-
Notifications
You must be signed in to change notification settings - Fork 13
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 to 0.9.* series so we get support for pydantic 2.0 and schema 0.6.5 #1823
Conversation
there seems to be some changes in behavior with the new dandischema to address , some look like
@candleindark does it ring a bell may be right away? |
Yes. Pydantic V2 constructs error objects, including error messages, differently in comparison to Pydantic V1. This particular error is resulting from the failure to pass the validator for the To adapt to the changes in error generations brought by Pydantic V2, I made changes in the tests in You may have to adjust some of the tests in dandi-archive to adapt to the new error generations in Pydantic V2 if these tests assess errors originate from Pydantic. |
Some of the dandi-archive code should also be updated to no longer use now-deprecated dandischema methods; see the warnings emitted at the end of the tests. |
@candleindark could you take over this PR and adjust those exception msgs, and see to @jwodder's comment about deprecated invocations, etc? |
OK |
@yarikoptic @jwodder I have solved the two failures that are related to the new format of error messages (to be pushed). There are four failures remaining. They are all related. I am suspecting the cause of the remaining failures is that in POSTing to create a new dandiset with metadata, the metadata is not written to the database. I am not familiar with Django and how a Django app accesses and organizes the DB, and I having trouble to locate the tables in the DB for storing dandisets and related metadata. In fact, there were no table in the result of the following SQL script.
|
I am able to see the tables in running the DB service in "production" mode. I don't have to execute the steps in https://github.com/dandi/dandi-archive/blob/master/DEVELOPMENT.md#initial-setup for testing, do I? Do you use SQLite or an equivalent for testing? If you do, where is that DB file? |
@dandi/dandiarchive could you please guide us here? or may be @kabilar already got hands dirty enough to know this? (otherwise we will need to try to get into chatting to various GPTs to align with my knowledge of testing django apps ;-) ) |
I think it is relying on a DB since https://github.com/dandi/dandi-archive/blob/master/.github/workflows/backend-ci.yml creates one. DB would leave in the docker (volume) I guess after compose-up ... if no experts follow up, I will dig it up tomorrow. |
Yeah, it seems to be a Postgres DB. I found out more using the following code in a test.
It is a database under the name of Additionally, the access to DB in a test is provided by the However, for |
Because of the transactional nature of @pytest.mark.django_db(transaction=True)
def test_dandiset_rest_create(api_client, user, transactional_db): |
I can now confirm that the metadata was not written correctly to the database in the first place. The cause of the problem is in
I will continue the play tomorrow. |
@yarikoptic The remaining failures in tests for the backend are resolved by this PR. However, for that solution to work, we have to wait for the merge of this PR, dandi/dandi-schema#218, for dandischema and possibly a new version of dandishcema because of it. @yarikoptic There are failures in the frontend tests as well. Do you have someone else to handle that already? |
@waxlamp @mvandenburgh - I have adjusted PR description with pointers about json schema changes |
@jwodder -- I am concerned about our dandi-cli testing here -- all green sounds too good to be true... looked into a sample run log and it says
whenever our runs in dandi-cli repo say |
@yarikoptic The CLI integration tests are run with the |
I pushed two fixes to this branch - one to fix the e2e CI test failures (cd5cd04), and the other to fix the Meditor showing a tab for "Schema Key" (b3db231). I included further explanations in the commit descriptions. I did some testing locally and things appear to be working after those fixes. I think we should still do extensive testing in staging before cutting a new release. |
Great, thank you @mvandenburgh ! |
And who would be to review & potentially approve? meanwhile I will request multiple candidates. |
b3db231
to
8413451
Compare
Thank you @mvandenburgh ! Now we need I guess review and a blessing/merge by @waxlamp ? |
So, @waxlamp should we merge and test it in staging? |
The validator for the `assetsSummary` field in the `PublishedDandiset` model in dandischema 0.9.0 generates an error with a slightly different message
The validator for the `digest` field in the `PublishedAsset` model in dandischema 0.9.0 generates an error with a slightly different message than before
This seems to be a breaking change in pydantics json schema export function.
Pydantic 2.0's JSON schema export no longer specifies a `type` field for the `schemaKey` property, and leaves it undefined instead. This change makes it so an undefined type also delegates it as a "basic schema", meaning it will not be rendered as its own tab in the meditor.
8413451
to
dd2c7b3
Compare
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.
I rebased this after merging #1865 , looks like everything is passing as expected 👍
🚀 PR was released in |
Closes #1820
TODOs
Dandiset
to have extra attributes dandi-schema#218). Extra notes from @candleindark :#2
in "Additional notes regarding changes entailed by Pydantic V2" in the top post of Update to supporting Pydantic V2 dandi-schema#203.