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

fix(shared-data): evotips v1 are schema 2 now #17428

Merged
merged 8 commits into from
Feb 6, 2025

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Feb 5, 2025

We had the evotips labware and adapter as schema 3 labware. This was probably necessary while this was a 3-element stack, but now it is not, and we need to avoid shipping schema 3 labware in 8.3 so that we retain the ability to change it.

Closes EXEC-1179

review

  • seem fine?

testing

  • smoke test. nothing checks schema identity, just whether data is there, and all the data that the code in 8.3 uses is still there.

@sfoster1 sfoster1 requested review from a team as code owners February 5, 2025 16:05
@sfoster1 sfoster1 requested review from shlokamin and CaseyBatten and removed request for a team February 5, 2025 16:05
@y3rsh y3rsh added the gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails label Feb 5, 2025
Copy link
Contributor

github-actions bot commented Feb 5, 2025

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17430

@sfoster1 sfoster1 force-pushed the exec-1179-downgrade-evotips branch from de30e52 to 994f8c4 Compare February 5, 2025 16:35
@sfoster1 sfoster1 added gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails and removed gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails labels Feb 5, 2025
@SyntaxColoring
Copy link
Contributor

SyntaxColoring commented Feb 5, 2025

There's a shared-data test failing now because the tips (wells[*].z + wells[*].depth) are higher than the bounding box (dimensions.z).

That test should have prevented the definition from being written like this in the first place, but it did not run on the schema 3 files. That was probably accidental, similar to #17425, and we can fix the test in a separate PR. EXEC-1193

But I'm not sure how the definition should be fixed here. The documented intent of dimensions is that it's the outer bounding box of the whole labware, which implies to me that it should envelop the tips, even if the tips extend beyond the "main body" of the labware. Can we just...do that, or is the existing zDimension value load-bearing?

@CaseyBatten @Laura-Danielle?

Base automatically changed from exec-1180-downgrade-tc-lids to chore_release-8.3.0 February 5, 2025 18:57
We had the evotips labware and adapter as schema 3 labware. This was
probably necessary while this was a 3-element stack, but now it is not,
and we need to avoid shipping schema 3 labware in 8.3 so that we retain
the ability to change it.

Closes EXEC-1179
@sfoster1 sfoster1 force-pushed the exec-1179-downgrade-evotips branch from 994f8c4 to 74295fc Compare February 5, 2025 18:58
@sfoster1 sfoster1 requested a review from a team as a code owner February 5, 2025 19:09
@CaseyBatten
Copy link
Contributor

There's a shared-data test failing now because the tips (wells[*].z + wells[*].depth) are higher than the bounding box (dimensions.z).

That test should have prevented the definition from being written like this in the first place, but it did not run on the schema 3 files. That was probably accidental, similar to #17425, and we can fix the test in a separate PR.

But I'm not sure how the definition should be fixed here. The documented intent of dimensions is that it's the outer bounding box of the whole labware, which implies to me that it should envelop the tips, even if the tips extend beyond the "main body" of the labware. Can we just...do that, or is the existing zDimension value load-bearing?

@CaseyBatten @Laura-Danielle?

I believe we can and probably should continue our pattern of the Z dimension representing the whole "naive block" of a labware's geometry, with the tips included "inside" that block, however I believe we will need to increase the stackingOffsetWithLabware values to reflect that new overall height. I think we probably want to remeasure the evotip length to double check accuracy, and then set the overall labware height to match that. At that point, we subtract the difference in height from the bottom of the metal component of the labware to the top of the evotips, and account for that difference in the stackingOffsetWithLabware values for the evotip adapter (96ch) and the deep well plate (8ch).

This means that the full height is the same as the well height, which is
a thing we need, and then we have to alter the stacking offsets
@sfoster1
Copy link
Member Author

sfoster1 commented Feb 6, 2025

Okay, turns out we don't need to change the stack offsets because the thing that would make us do that is changing the implied bottom location or changing the adapter geometry, neither of which we did. So, all good!

Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Looks good and should be good to go, some optional comments on the labware load on some of our test fixtures.

Comment on lines 39 to 45
return LabwareDefinition.model_validate(
json.loads(
load_shared_data(
"labware/definitions/3/evotips_opentrons_96_labware/1.json"
"labware/definitions/2/evotips_opentrons_96_labware/1.json"
)
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're no longer doing the V3 version of this labware we can probably stop referring to it by the entire load name and can just use the shared_data labware load_definition function with something like:

return LabwareDefinition.parse_obj(
        load_definition("evotips_opentrons_96_labware", 1)
)

This would mean when we update the whole system to use V3 definitions as the default the test cases will start using those by default as well.

@@ -47,7 +47,7 @@ def evotips_definition() -> LabwareDefinition:
return LabwareDefinition.model_validate(
json.loads(
load_shared_data(
"labware/definitions/3/evotips_opentrons_96_labware/1.json"
"labware/definitions/2/evotips_opentrons_96_labware/1.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here

@@ -91,7 +91,7 @@ def evotips_definition() -> LabwareDefinition:
return LabwareDefinition.model_validate(
json.loads(
load_shared_data(
"labware/definitions/3/evotips_opentrons_96_labware/1.json"
"labware/definitions/2/evotips_opentrons_96_labware/1.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

And here as well on this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll switch it to work like the other PE fixtures which loads from shared data python bindings but still specifies version

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.

LGTM if you two are satisfied with the stack offset / zDimension thing. Thank you!

@sfoster1 sfoster1 merged commit 71bfaf6 into chore_release-8.3.0 Feb 6, 2025
55 checks passed
@sfoster1 sfoster1 deleted the exec-1179-downgrade-evotips branch February 6, 2025 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants