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

chore(pydantic): V1->V2 migration #14805

Closed
wants to merge 0 commits into from
Closed

Conversation

ahiuchingau
Copy link
Contributor

@ahiuchingau ahiuchingau commented Apr 4, 2024

Superseded by #14871.

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 96.25000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 67.20%. Comparing base (65885b2) to head (00a4299).
Report is 55 commits behind head on edge.

❗ Current head 00a4299 differs from pull request most recent head e83d1b5. Consider uploading reports for the commit e83d1b5 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14805      +/-   ##
==========================================
- Coverage   67.24%   67.20%   -0.05%     
==========================================
  Files        2495     2495              
  Lines       71254    71396     +142     
  Branches     8937     8984      +47     
==========================================
+ Hits        47918    47979      +61     
- Misses      21235    21302      +67     
- Partials     2101     2115      +14     
Flag Coverage Δ
hardware 55.57% <100.00%> (ø)
shared-data 75.95% <96.10%> (+0.01%) ⬆️

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

Files Coverage Δ
...are/opentrons_hardware/drivers/can_bus/settings.py 95.77% <100.00%> (ø)
...pentrons_shared_data/gripper/gripper_definition.py 95.23% <100.00%> (-0.22%) ⬇️
...pentrons_shared_data/labware/labware_definition.py 100.00% <100.00%> (ø)
...a/python/opentrons_shared_data/pipette/__init__.py 94.73% <100.00%> (ø)
...python/opentrons_shared_data/protocol/dev_types.py 100.00% <100.00%> (ø)
..._shared_data/protocol/models/protocol_schema_v6.py 100.00% <100.00%> (ø)
..._shared_data/protocol/models/protocol_schema_v7.py 100.00% <100.00%> (ø)
..._shared_data/protocol/models/protocol_schema_v8.py 100.00% <100.00%> (ø)
...trons_shared_data/protocol/models/shared_models.py 100.00% <100.00%> (ø)
...pentrons_shared_data/pipette/pipette_definition.py 94.23% <96.55%> (-0.10%) ⬇️
... and 1 more

... and 55 files with indirect coverage changes

offset: Optional[OffsetVector]
profile: Optional[List[ProfileStep]]
radius: Optional[float]
slotName: Optional[str] = None
Copy link
Member

Choose a reason for hiding this comment

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

we sure about these changes? slotName: Optional[str] without the =None means "you have to explicitly specify it but you can specify it as None/null" which I think is what we want

Copy link
Contributor

Choose a reason for hiding this comment

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

In Pydantic v1, slotName: Optional[str] defaulted to None when the field was missing from the source JSON:

>>> class M(pydantic.BaseModel):
...     slotName: typing.Optional[str]
...
>>> M.parse_raw("{}")
M(slotName=None)

In Pydantic v2, that becomes a validation error. If you want a default, you have to specify a default. This is a good, but breaking, change, so I guess the upgrade script applies this fixup liberally.

In this particular case, with the Params class, I think it's correct for these to default to None. In other cases of f: Optional[T], though, we'll definitely need to be careful to cross-check against our underlying JSON schemas.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

This mostly makes sense to me so far, thanks!

Comment on lines 362 to 385
class Config:
json_encoders = {
# TODO[pydantic]: The following keys were removed: `json_encoders`.
# Check https://docs.pydantic.dev/dev-v2/migration/#changes-to-config for more information.
model_config = ConfigDict(
json_encoders={
pip_types.PipetteChannelType: lambda v: v.value,
pip_types.PipetteModelType: lambda v: v.value,
pip_types.PipetteGenerationType: lambda v: v.value,
pip_types.Quirks: lambda v: v.value,
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is saying json_encoders isn't doing anything anymore; this is dead code.

According to the migration guide, there's some other way to do this now. But it isn't clear to me what this was accomplishing before. These lambdas look like straight passthroughs to me.

Comment on lines 34 to 35
diameter: Optional[float]
yDimension: Optional[float]
xDimension: Optional[float]
diameter: Optional[float] = None
yDimension: Optional[float] = None
xDimension: Optional[float] = None
Copy link
Contributor

@SyntaxColoring SyntaxColoring Apr 5, 2024

Choose a reason for hiding this comment

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

This whole WellDefinition model is apparently unused. I think we can delete it.

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.

3 participants