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

Upgrade to django-ninja v1 / pydantic v2 #540

Closed
wants to merge 9 commits into from

Conversation

mvandenburgh
Copy link
Member

@mvandenburgh mvandenburgh commented Nov 17, 2024

This is a major upgrade that requires changes to many of our Pydantic-based schemas. See https://docs.pydantic.dev/latest/migration/.

A lot of the changes in this PR were automated by the bump-pydantic tool, with some manual cleanup and fixes after.

Note, pydantic 2 also claims backwards compatibility with pydantic 1 code via the nested pydantic.v1 package. However, django-ninja does not support it so we can not use it here.

This is a major upgrade that requires changes to all the Pydantic-based
schemas. See https://docs.pydantic.dev/latest/migration/
Fields that are explicitly *optional* (i.e. can be missing entirely,
as opposed to being present with a value of `None`) now need to be
assigned `None` by default.
See https://docs.pydantic.dev/2.9/migration/#required-optional-and-nullable-fields
These schemas had incorrect types. It seems like pydantic 1 didn't care,
but pydantic 2 does.
This is required because rio-tiler v5 is incompatible with pydantic 2.
Note, v7 is the latest version, but it contains a substantial number
of breaking changes. To keep things simple, we can upgrade to v6 for
now and revisit v7 later.
@mvandenburgh mvandenburgh marked this pull request as ready for review November 18, 2024 18:40
@mvandenburgh
Copy link
Member Author

mvandenburgh commented Nov 18, 2024

@BryonLewis @floryst this is ready for review. I tried testing the entire feature set of the application with these changes, and I fixed all the issues I came across. But, it's very possible that I missed something considering the scope of changes, so any further stress testing of the whole system would be welcome 🙂

Also, I would suggest we postpone merging this PR until v1.14, so no particular urgency here.

Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most things involving the endpoints touched seemed to work properly including downloading and the downloading stats.

Not approval yet but just a checklist of things I went over:

  • Scoring - image fetching and visualization for WV and S2

  • Scoring - visualization of Sites/Observations, Scoring Mode

  • Scoring - Selection of site and downloading of Animations

  • RGD - image fetching and visualization in map for WV and S2

  • RGD - Site/Observation vector tile visualization

  • RGD - Selection of objects and exporting animations

  • RGD - Downloading JSON file

Not Working:

  • Image Browser in both RGD and Scoring - I think a merge to main will fix it. Forrest I believe fixed a CSS issue for this
  • Scrolling the ModelRun list - I believe this again is a merge from main that will fix it.

@BryonLewis BryonLewis self-requested a review December 6, 2024 13:40
Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after the merge from main I believe the remaining issues I had are now fixed.

@mvandenburgh
Copy link
Member Author

Closing after discussing with @aashish24. We might revisit this in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants