-
Notifications
You must be signed in to change notification settings - Fork 15
Add ability to downgrade schema to 0.6.10 #342
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #342 +/- ##
=======================================
Coverage 97.84% 97.84%
=======================================
Files 16 16
Lines 2042 2042
=======================================
Hits 1998 1998
Misses 44 44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@waxlamp here we are to get into that unpleasant situation to need to coordinate releasing since otherwise if client has this release more recent dandi-schema installed (possible with or without us pinning in client): we would get so the question(s)
|
How could this happen if you are pinning (For the record, it seems that the CLI does indeed pin
No problem, given that both the archive and the CLI are pinning to a specific version of This does mean we need to coordinate releasing versions of both that do use the new version, but I'm not sure how we can avoid that without solving the bigger problem of enabling automatic migration between schema versions.
I imagine it would be part of dandi/dandi-archive#2606 (see this comment). |
Simply because we are testing against these changes, which will give new version 0.7.0 -- that's the whole point. And it just reveals fragility of our tight binding of versions here and there and that pinning just a workaround for "user deployment" while developers, who need to test components where one is ahead of release, would get out of sync! What is critical here is that actually this new version would be just fine for dandi-cli... so I might reconsider and
then this would
well, I think there is overall misunderstanding as we actually/apparently do not auto migrate metadata at all ATM! Server does not really care much, and client started to demand exact match to avoid missing recently added new metadata to be extracted/uploaded. Since here we talk about releaseNotes which client is not concerned about, it should not be concerned about being 'outdated' from client's perspective, so no boost of
thanks for the pointers! Indeed, would make sense to boost in that PR to this new version simply to allow for this new feature (since you hard-pin). |
Server might need then to either wait until upgrade it becomes capable of
validating them or we might in the future to allow client to downgrade them (on
client; loselessly only) since server cannot yet handle them.
This behavior would facilitate testing of new releases of dandi-schema which now
would fail since we require 1-to-1 correspondence. See e.g.
dandi/dandi-schema#342 (comment)
Server might need then to either wait until upgrade it becomes capable of
validating them or we might in the future to allow client to downgrade them (on
client; loselessly only) since server cannot yet handle them.
This behavior would facilitate testing of new releases of dandi-schema which now
would fail since we require 1-to-1 correspondence. See e.g.
dandi/dandi-schema#342 (comment)
|
@waxlamp here is my solution on dandi-cli side which I am planing to merge/release to largely decouple back client and server in terms of dandi-schema: please chime in! With that PR, if I revert here to upgrade within 0.6.x -- dandi-cli tests must not be affected! for 0.7.x indeed would keep failing ATM but in principle we could relax there too ;-) |
ecd02f9 to
5fd99be
Compare
candleindark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment. Will provide more.
candleindark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more comments.
|
|
||
| # Simple downgrades that just require removing fields, which is totally fine | ||
| # if they are empty | ||
| SIMPLE_DOWNGRADES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables in function should be lowercase generally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just following convention of const variables to use CAPITALS (which we do extensively generally at module level e.g. in models.py) -- this variable is not expected to change.
dandischema/metadata.py
Outdated
| if to_version_tuple < version2tuple(ver_added) <= obj_version_tuple: | ||
| for field in fields: | ||
| if field in obj_migrated: | ||
| if val := obj_migrated.get(field): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a falsy field value is actually a valid value?
Additionally, this assumes that a None value is the default of the field. I.e., having a None value is considered as the value not provided, though I think this assumption is mostly true in our models if not entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point! I was aiming for an "empty value" check but indeed what is needed is to check if it matches the default... or may be more relaxed would be just to check if field there at all, and hence remove this check, but then it would become dependent on it being optional and people do not fill up optional field.
Overall, given current distraction at sfn 2025, I will postpone the "downgrade" portion of this PR. Since you provided good comments, I will craft a separate short one for releaseNotes so we could proceed with merging both that + devendorization in one go. I will deal with downgrades later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slept on it . "We are fine" since we are operating only on the fields we are to add and thus they would need to conform to our expectation of what "empty" constitutes. But I will push now commit adding relevant comment and logic to treat only some set of containers and None specifically here.
| # before migration | ||
| if not skip_validation: | ||
| _validate_obj_json(obj, _get_jsonschema_validator(obj_ver, "Dandiset")) | ||
| _validate_obj_json(obj, _get_jsonschema_validator(obj_version, "Dandiset")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This migration function seems to expect the metadata instance to be a Dandiset instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validating PublishedDandiset instance may create a problem if #266 is put in. However, I guess we can have it specifically validate against the PublishDandiset schema if a PublishedDandiset instance is provided.
Add pytest marker registration for 'ai_generated' to avoid PytestUnknownMarkWarning when running tests marked with @pytest.mark.ai_generated. This marker is used to identify tests generated by AI assistants, allowing them to be filtered or selected separately if needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This change introduces the ability to downgrade metadata schemas to recent versions, allowing for more flexible schema version management. Key changes: - Allow migration to version 0.6.10 in addition to the current version - Implement SIMPLE_DOWNGRADES mechanism for safe field removal during downgrade - Remove restriction preventing downgrade to lower schema versions - Add validation to prevent data loss when downgrading with populated fields - Add comprehensive tests for downgrade functionality with releaseNotes field The downgrade mechanism ensures data integrity by raising an error if a field being removed during downgrade contains a non-empty value. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
fcc2a99 to
43cb3f6
Compare
Reminder to myself: we run checks on no changes to schema only on PRs with "release" tag. Thus we did not run checks when we actually adjusted the schema in #185
by @bendichter. Then I rushed merging
#341
as I was not expecting surprises from adjusting labels (forgot about above).
The moral again, do not rush merging - let CI finish! ;)
Boosting "patch" for both schema and library since just a "minor addition of a new feature" (mostly to schema).
TODOs