Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Statically bundle dandi/schema json #155

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

danlamanna
Copy link
Contributor

This is a minimal PR to show an example of what was discussed in #153.

The git submodule approach seemed to be the simplest way to ensure the schema files were present in every environment: development, testing, and packaging. It should be easy to programatically "bump" this when new versions of dandi/schema are released as well.

@danlamanna danlamanna marked this pull request as ready for review January 23, 2023 18:13
@satra
Copy link
Member

satra commented Jan 31, 2023

@danlamanna - the release workflow for this project generates the schema and pushes to the repo during an automated github action (https://github.com/dandi/dandi-schema/blob/master/.github/workflows/release.yml#L76) and then builds the package for distribution via pypi and uploads there.

will this change then pick the latest release from the schema repo?

@satra satra added the minor Increment the minor version when merged label Jan 31, 2023
@danlamanna
Copy link
Contributor Author

the release workflow for this project generates the schema and pushes to the repo during an automated github action (master/.github/workflows/release.yml#L76) and then builds the package for distribution via pypi and uploads there.

will this change then pick the latest release from the schema repo?

It doesn't, I just made this to get feedback on the approach. If it looks good then I can adjust the release.yml to:

  1. Generate the new schema files and commit/push them to dandi/schema
  2. Update the submodule revision and commit it in dandi/dandi-schema

setup.cfg Outdated Show resolved Hide resolved
@satra
Copy link
Member

satra commented Feb 1, 2023

the approach looks reasonable. i would add a github action test that checks that a package built from this repo with the submodule enabled can be installed and run, and can be used to access all acceptable versions of the schema.

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Base: 96.67% // Head: 96.67% // No change to project coverage 👍

Coverage data is based on head (f754432) compared to base (969c359).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head f754432 differs from pull request most recent head e20aadd. Consider uploading reports for the commit e20aadd to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #155   +/-   ##
=======================================
  Coverage   96.67%   96.67%           
=======================================
  Files          18       18           
  Lines        1654     1654           
=======================================
  Hits         1599     1599           
  Misses         55       55           
Flag Coverage Δ
unittests 96.67% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandischema/metadata.py 96.66% <100.00%> (+0.03%) ⬆️
dandischema/digests/zarr.py 98.71% <0.00%> (-0.02%) ⬇️
dandischema/model_types.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@danlamanna
Copy link
Contributor Author

danlamanna commented Feb 16, 2023

I attempted to update the release action in a64e806, but I don't have a way of testing it. I'm also unfamiliar with auto so I'm not sure how to grab the version that will be released (see the TODO). @jwodder It looks like you wrote the release action originally, would you mind taking a look? The new workflow should be 1) generate new schema versions and commit/push them to dandi/schema 2) update the submodule on dandi-schema to include the new schema versions and 3) commit/tag/push the new version of dandi-schema with the updated submodule.

i would add a github action test that checks that a package built from this repo with the submodule enabled can be installed and run, and can be used to access all acceptable versions of the schema.

I'm not sure I follow. The nice thing about this approach is that the package is always used with the submodule enabled, so the existing tests using tox already build such a package.

@jwodder
Copy link
Member

jwodder commented Feb 16, 2023

@danlamanna You can get the to-be-released version via ${{ needs.release-check.outputs.auto-version }}.

@jwodder
Copy link
Member

jwodder commented Feb 16, 2023

@danlamanna Wait, no, that returns the bump level, not the actual version. I'll get back to you.

@yarikoptic
Copy link
Member

FWIW, I agree that it is a good approach. The only usecase I see this would disallow for is validation against a "future" (to the used older dandi-schema package version) version of schema. So it would mean to need to update dandi-schema package every time we release a new version. If nobody sees a problem with that, as "we need to do that now anyways already in dandi-archive", then all good.

@jwodder
Copy link
Member

jwodder commented Feb 16, 2023

@danlamanna Unfortunately, auto does not expose the new version that it's releasing, so you'll have to take the version bump level from ${{ needs.release-check.outputs.auto-version }} and apply it to the most recent prior version to find that out. Here's some code to get you started.

@danlamanna danlamanna force-pushed the statically-bundle-schemas branch 2 times, most recently from 37d745e to 44587d6 Compare February 17, 2023 18:21
@danlamanna
Copy link
Contributor Author

I think this latest revision makes sense, but I'm skeptical it will work whenever the next release is cut since I don't have a proper sandbox for testing it.

yarikoptic added a commit that referenced this pull request Dec 18, 2023
Transpired by looking at never finalized
#155
as a much less intrusive and simpler approach to avoid repetative requests
for the same schema files.
yarikoptic added a commit that referenced this pull request Dec 18, 2023
Transpired by looking at never finalized
#155
as a much less intrusive and simpler approach to avoid repetative requests
for the same schema files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants