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

Allow dictionary representation of Dandiset to have extra attributes #218

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

candleindark
Copy link
Member

@candleindark candleindark commented Jan 26, 2024

This PR allows the dictionary representation of Dandiset to have extra attributes.

ATM, packages such as dandi-archive construct Dandiset objects using Dandiset.model_construct() to allow extra attributes to be passed to the resulting Dandiset objects. While extra attributes can be passed to the resulting Dandiset objects this way, the dictionary representations of these Dandiset objects will not contain the extra attributes unless Dandiset is explicitly configured to allow extra attributes.

dandi-archive requires extra attributes passed in constructing a Dandiset object to be present in the dictionary representation of the object. This change in this PR explicitly configures Dandiset to allow extra attributes to meet this requirement, and meeting this requirement will eliminate some of the failures encountered in dandi/dandi-archive#1823.

Please note, the change in this PR will modify the JSON schemata for Dandiset and PublishedDandiset slightly. Both schemata will contain the additional key of "additionalProperties" with the value of true. Please consider increase the version number of the JSON schemas to 0.6.6.

Note: This PR is a follow up to #203 to make a new version of dandi-schema with Pydantic 2.0 working for dandi-archive.

So that the extra attributes passed to
constructing a `Dandiset` object will be
included in its dictionary representation
produced by `BaseModel.model_dump()`
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4423b41) 97.66% compared to head (6ac7c8d) 91.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage   97.66%   91.99%   -5.67%     
==========================================
  Files          18       18              
  Lines        1798     1799       +1     
==========================================
- Hits         1756     1655     -101     
- Misses         42      144     +102     
Flag Coverage Δ
unittests 91.99% <100.00%> (-5.67%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yarikoptic
Copy link
Member

ATM, packages such as dandi-archive construct Dandiset objects using Dandiset.model_construct() to allow extra attributes to be passed to the resulting Dandiset objects.

could you please elaborate/point to what you mean since I find no model_construct in dandi-archive codebase and if there is some custom use like that I wonder if that is something we should aim to avoid.

@candleindark
Copy link
Member Author

candleindark commented Jan 27, 2024

Here is the reason why this PR is needed.

Before the deprecation of the json_dict method, the method was a workaround to generate a dictionary representation of DandiBaseModel objects that's JSON serializable. I found that such workaround was not necessary and deprecated it in #203. However, in resolving the failures in dandi/dandi-archive#1823, I learned that dandi-archive attaches extra attributes to Dandiset objects (extra in the sense that these attributes are not defined as fields in the Dandiset model). To have these extra attributes included in the dictionary representation of the Dandiset objects generated by the facilities provided by Pydantic, the model has to be marked explicitly to allow extra attributes, and the change in this PR is the explicit marking.

@candleindark
Copy link
Member Author

ATM, packages such as dandi-archive construct Dandiset objects using Dandiset.model_construct() to allow extra attributes to be passed to the resulting Dandiset objects.

could you please elaborate/point to what you mean since I find no model_construct in dandi-archive codebase and if there is some custom use like that I wonder if that is something we should aim to avoid.

The two deprecated methods in this line is the cause of the problem.

unvalidated calls model_construct() and json_dict calls model_dump().

@yarikoptic
Copy link
Member

@jwodder and @satra please review on either you bless such changes.

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Jan 30, 2024
@jwodder
Copy link
Member

jwodder commented Jan 30, 2024

If the JSON Schema has changed, we need to bump the schema version.

@yarikoptic
Copy link
Member

we would need to release again , so labeling accordingly

The JSON schemata for `Dandiset` and
`PublishedDandiset` now contain the additional
key of "additionalProperties" with the value of
"true"
@candleindark
Copy link
Member Author

Once #221 is merged to master. This PR should be ready for merge.

@candleindark
Copy link
Member Author

@yarikoptic This one is ready to merge.

@yarikoptic
Copy link
Member

ok, let's see where it would get us

@yarikoptic yarikoptic merged commit 5b50a11 into dandi:master Jan 30, 2024
34 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants