Feature/eng 2423 migrate to pydantic v2#60
Conversation
jamie-sgro
requested review from
ShengpeiWang,
jtanwk and
lmcindewar
as code owners
E TypeError: 'type' object is not subscriptable
| has_transforms: bool | ||
| has_variables: bool | ||
| orbit_parameters: dict | ||
| orbit_parameters: Dict[Any, Any] |
There was a problem hiding this comment.
Just for my information: other than being more explicit, is there any benefit to the new typing syntax?
There was a problem hiding this comment.
Similar question:
- why explicitly type with a
typing.Dictinstead of justdict? Can you point to where it recommends this in the migration docs - is the purpose of
Anyjust a placeholder to show that we haven't explicitly typed it yet? From glancing at the downstream code, we definitely have expectations for what the contents of the lists and dicts are, so I'd recommend we add the explicit typing that either now or in a second step
There was a problem hiding this comment.
I take back my Any comment re: doing it now. Adding in Any lets you get pylance off your back while still retaining a signal to explicitly type things later
There was a problem hiding this comment.
Great question. It's a push and pull between supporting python v3.8 and pydantic v2.
- New pylance/pydantic wants content filling in the type generic (dict/Dict is a generic)
- The newer dict[...] syntax doesn't play nice with Python versions <3.9 (ref)
ShengpeiWang
left a comment
ShengpeiWang
left a comment
There was a problem hiding this comment.
Changes make sense and CICD passed!
jtanwk
left a comment
jtanwk
left a comment
There was a problem hiding this comment.
Code itself looks like it should broadly work, but some open stylistic questions below
| has_transforms: bool | ||
| has_variables: bool | ||
| orbit_parameters: dict | ||
| orbit_parameters: Dict[Any, Any] |
There was a problem hiding this comment.
Similar question:
- why explicitly type with a
typing.Dictinstead of justdict? Can you point to where it recommends this in the migration docs - is the purpose of
Anyjust a placeholder to show that we haven't explicitly typed it yet? From glancing at the downstream code, we definitely have expectations for what the contents of the lists and dicts are, so I'd recommend we add the explicit typing that either now or in a second step
| @@ -1,5 +1,5 @@ | |||
| requests==2.* | |||
| pydantic==1.* | |||
| pydantic==2.* | |||
There was a problem hiding this comment.
Did we also want to specify >=2.12.4 here (as you wanted to do in rp)?
There was a problem hiding this comment.
We have external consumers. Given this is a parent node in the dependency DAG, the semver range should be broadly specified
3 participants
Description
We're upgrading pydantic across the board to V2. This means updating any deprecated syntax from V1
Type of change
Please delete options that are not relevant.
Checklist:
Next Steps
Assign a reviewer based on the code owner document.
Once your review is approved, merge and delete the feature branch
On behalf of the Modis Tools Dev Team, thank you for your hard work! ✨