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

Boost dandischema and DANDI_SCHEMA_VERSION #1074

Merged
merged 4 commits into from
May 3, 2022
Merged

Conversation

yarikoptic
Copy link
Member

@satra have mentioned the need to upgrade dandischema library switching dandischema library from 0.6. to 0.7. series. Since there just happened 0.7.1 bugfix release -- made it the target version

  • is this enough and safe to do?
  • why don't we use 'dandischema=~0.7.1' i.e. to allow for further versions within 0.7 series?
  • whenever upgrade propagates, we would need to merge/release Upgrade dandischema to 0.7.x series dandi-cli#987 to do similar upgrade on client side
  • related ref: dandischema alignment with releases #1012
  • why do we hardcode DANDI_SCHEMA_VERSION (diff here to switch to 0.6.3) and not just import it from dandischema? (dandischema/consts.py:DANDI_SCHEMA_VERSION = "0.6.3")

this is pretty much a "motivational PR". Dear @waxlamp , I am assigning it to you to reassign/change/finalize/etc.

@jjnesbitt
Copy link
Member

@satra have mentioned the need to upgrade dandischema library switching dandischema library from 0.6. to 0.7. series. Since there just happened 0.7.1 bugfix release -- made it the target version

  • is this enough and safe to do?

If the tests pass, I don't see why not.

  • why don't we use 'dandischema=~0.7.1' i.e. to allow for further versions within 0.7 series?

If dandi-schema verisons comply with semantic versioning then pinning to ~=0.7.1 would be ideal.

  • why do we hardcode DANDI_SCHEMA_VERSION (diff here to switch to 0.6.3) and not just import it from dandischema? (dandischema/consts.py:DANDI_SCHEMA_VERSION = "0.6.3")

I think we should keep the setting just in case it ever needs to be overridden, but I think the default value should be pulled from dandischema.

I can push a change that fixes CI and updates this behavior.

@yarikoptic
Copy link
Member Author

dandi-cli tests are failing because we still install dandischema-0.6.0 in those tests since dandi-cli is awaiting for dandi-archive to get this upgrade. So, let's proceed and possibly adjust dependencies etc later per discussion above. Filed a dedicated #1078 for that

@yarikoptic yarikoptic merged commit 89b35a0 into master May 3, 2022
@yarikoptic yarikoptic deleted the rf-dandischema branch May 3, 2022 14:23
@dandibot
Copy link
Member

dandibot commented May 5, 2022

🚀 PR was released in v0.2.20 🚀

@dandibot dandibot added the released This issue/pull request has been released. label May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants