dandi / dandi-schema

Schemata for DANDI archive project
Apache License 2.0
7 stars 10 forks source link

Statically bundle dandi/schema json #155

Open danlamanna opened 1 year ago

danlamanna commented 1 year ago

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.

satra commented 1 year ago

@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?

danlamanna commented 1 year ago

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
satra commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

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

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

:exclamation: 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 ```diff @@ 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](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/dandi/dandi-schema/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | Coverage Δ | | |---|---|---| | [dandischema/metadata.py](https://codecov.io/gh/dandi/dandi-schema/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#diff-ZGFuZGlzY2hlbWEvbWV0YWRhdGEucHk=) | `96.66% <100.00%> (+0.03%)` | :arrow_up: | | [dandischema/digests/zarr.py](https://codecov.io/gh/dandi/dandi-schema/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#diff-ZGFuZGlzY2hlbWEvZGlnZXN0cy96YXJyLnB5) | `98.71% <0.00%> (-0.02%)` | :arrow_down: | | [dandischema/model\_types.py](https://codecov.io/gh/dandi/dandi-schema/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#diff-ZGFuZGlzY2hlbWEvbW9kZWxfdHlwZXMucHk=) | `100.00% <0.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi)

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

danlamanna commented 1 year ago

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 commented 1 year ago

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

jwodder commented 1 year ago

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

yarikoptic commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.