refactor(api,robot-server,shared-data): Split LabwareDefinition Pydantic model for labware schemas 2 and 3
#17563
Conversation
3e67f83 to
d994928
Compare
| innerLabwareGeometry={ | ||
| "welldefinition1111": InnerWellGeometry( | ||
| sections=[ | ||
| CuboidalFrustum( | ||
| shape="cuboidal", | ||
| topXDimension=7.6, | ||
| topYDimension=8.5, | ||
| bottomXDimension=5.6, | ||
| bottomYDimension=6.5, | ||
| topHeight=45, | ||
| bottomHeight=20, | ||
| ), | ||
| CuboidalFrustum( | ||
| shape="cuboidal", | ||
| topXDimension=5.6, | ||
| topYDimension=6.5, | ||
| bottomXDimension=4.5, | ||
| bottomYDimension=4.0, | ||
| topHeight=20, | ||
| bottomHeight=10, | ||
| ), | ||
| SphericalSegment( | ||
| shape="spherical", | ||
| radiusOfCurvature=6, | ||
| topHeight=10, | ||
| bottomHeight=0.0, | ||
| ), | ||
| ], | ||
| ) | ||
| }, |
There was a problem hiding this comment.
This test definition had a mixture of schema 2 and 3 stuff, and it was unclear to me which direction to take it. I've deleted the schema 3 stuff because it seemed like it was unused in this file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17563 +/- ##
==========================================
- Coverage 63.45% 63.45% -0.01%
==========================================
Files 2840 2840
Lines 218427 218412 -15
Branches 18123 18123
==========================================
- Hits 138605 138590 -15
Misses 79629 79629
Partials 193 193
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| f" using nominal fallback value of {nominal_fallback}" | ||
| ) | ||
| log.debug(message, exc_info=e) | ||
| return nominal_fallback |
There was a problem hiding this comment.
Heads up for this temporary hack that I'm doing for OT-2 tip length calibration.
The ticket I linked, EXEC-1230, currently talks about totally blocking these calibration flows from dealing with labware schema 3. I'm changing my mind on that—I think we have to get them to deal with labware schema 3 correctly somehow—and I'll rewrite EXEC-1230 when I know more about the approach.
There was a problem hiding this comment.
Why do you think we have to get them to deal with labware schema 3 correctly? We could instead not ever issue OT-2 tiprack or calibration block definitions in schema 3. We possibly lock out loading tipracks in schema 3 when the robot is an OT-2, for this reason. This means that OT-2 tipracks wouldn't get the benefit of the more-correct geometry loading, but this is an ancient and known problem with OT-2 tipracks that people have been handing for years.
There was a problem hiding this comment.
Why do you think we have to get them to deal with labware schema 3 correctly?
Basically because I didn't feel it was acceptable to abandon OT-2 tip racks like that. If our goal is to improve labware positioning, but we fail at standard OT-2 tip racks, then that's a pretty big bummer. They're certainly not edge cases. I think it's worth trying to resolve even if it means us (or me) pushing a little harder.
Locking OT-2 tip racks and calibration blocks to schema 2 sounds like a good escape hatch, if it comes down to it, but I hope it doesn't come down to it.
I can deprioritize this and revisit the question later, but mark it as something we must resolve somehow before v8.4.0—either get it to work with schema 3, or delete the schema 3 defs and lock it to schema 2.
Instead of sharing attributes between schema 2 and 3, share attributes between rectangular and circular wells. In other words, separate the schema versions a little bit more. Fix a bug where the type of geometryDefinitionId was inconsistent between rectangular and circular wells.
523528f to
df90055
Compare
df90055 to
9c1f0e4
Compare
Resolve conflicts in shared-data/python/opentrons_shared_data/labware/labware_definition.py. Add the new $otSharedSchema property to LabwareDefinition3.
| # $otSharedSchema deliberately omitted for now. See `"extra": "allow"` in model_config. | ||
| schemaVersion: Literal[1, 2, 3] | ||
| class LabwareDefinition2(BaseModel): | ||
| # todo(mm, 2025-02-18): Is it correct to accept schemaVersion==1 here? |
There was a problem hiding this comment.
uh it almost certainly shouldn't be, this code may have started out trying to handle schema 1 labware but i almost guarantee it does not currently do that.
| f" using nominal fallback value of {nominal_fallback}" | ||
| ) | ||
| log.debug(message, exc_info=e) | ||
| return nominal_fallback |
There was a problem hiding this comment.
Why do you think we have to get them to deal with labware schema 3 correctly? We could instead not ever issue OT-2 tiprack or calibration block definitions in schema 3. We possibly lock out loading tipracks in schema 3 when the robot is an OT-2, for this reason. This means that OT-2 tipracks wouldn't get the benefit of the more-correct geometry loading, but this is an ancient and known problem with OT-2 tipracks that people have been handing for years.
Overview
Closes EXEC-1206.
Test Plan and Hands on Testing
Just automated tests.
Changelog
Main changes:
Split the
LabwareDefinitionPydantic model intoLabwareDefinition2andLabwareDefinition3, for labware schemas 2 and 3.Many minor updates across our backend to account for the above.
Try to be mindful about which things in our backend should deal with
LabwareDefinition2 | LabwareDefinition3and which things should now limit themselves toLabwareDefinition2.For example, our Python types for JSON protocol schema 8 now accept
LabwareDefinition2 | LabwareDefinition3, whereas for schemas ≤7 they continue to only supportLabwareDefinition2.Plus a couple of smaller fixes:
WellDefinitionmodel intoRectangularWellDefinitionandCircularWellDefinition.TypedDict-based bindings wheregeometryDefinitionIdwas omittable for rectangular wells but not circular ones.Review requests
Can you think of any other places in our backend that should deliberately limit themselves to
LabwareDefinition2, instead of acceptingLabwareDefinition2 | LabwareDefinition3?Risk assessment
Low. This shouldn't be any riskier than what we had before.