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

feat(app,protocol-designer,shared-data): Allow JSON protocols to mix labware schemas #17560

Draft
wants to merge 7 commits into
base: edge
Choose a base branch
from

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Feb 20, 2025

Overview

This allows JSON protocol files to contain a mixture of labware defined in labware schema 2 and labware defined in labware schema 3.

I think this is necessary because schemas 2 and 3 are mutually incompatible, and someone might want to use, for example, their existing schema-2 custom labware alongside our new schema-3 standard labware.

Closes EXEC-1236.

Test Plan and Hands on Testing

So far, just CI.

Changelog

Done:

  • In the underlying JSON schema, basically deprecate the labwareDefinitionSchemaId property.

    It was already accepting arbitrary strings, and that hasn't changed. However, in the human-readable description, we now say that it should just contain the empty string (""), instead of a labware schema ID like "opentronsLabwareSchemaV2".

  • Protocol Designer now emits JSON protocols with labwareDefinitionSchemaId set to "".

  • The on-device display's Quick Transfer code now emits JSON protocols with labwareDefinitionSchemaId set to "".

  • Update TypeScript types to reflect that a protocol's labware contains LabwareDefinition2 | LabwareDefinition3, not just LabwareDefinition2.

  • Update the JSON Schema validation code in shared-data to ignore labwareDefinitionSchemaId. Instead, it validates each labware individually into a <labware schema 2> | <labware schema 3> union.

Not done:

  • No treatment needed for Python in this PR. Our Python parsing code has never relied on labwareDefinitionSchemaId. It already accepts labware schemas 2 and 3 (though it doesn't yet handle schema 3 fully correctly).
  • Getting presentation-layer TypeScript code to correctly handle schema-3 labware—for example, rendering the wells in the correct place even though the location of the origin has changed—is out of scope for this PR.

Review requests

  • Fundamentally, does all of the above seem like a good idea, and is the "" thing a good approach?
  • I haven't worked with PD or Quick Transfer much, so I'm not aware of any gotchas. Is there anything in particular I need to manually test?
  • See comments below.

Risk assessment

Low?

@SyntaxColoring SyntaxColoring requested a review from a team February 20, 2025 17:27
Comment on lines +160 to +162
patternProperties: {
'.+': { anyOf: [labwareSchema2, labwareSchema3] },
},
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Feb 20, 2025

Choose a reason for hiding this comment

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

Is this an okay way to validate into a union with ajv?

Because JSON Schema doesn't have a way of specifying a union discriminator, I'm a little worried that this will be slow and cause bad error messages.

It's certainly possible to fix that by switching on schemaVersion ourselves. I'm just not sure whether that's worth the complexity.

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 41.66667% with 21 lines in your changes missing coverage. Please review.

Project coverage is 26.16%. Comparing base (2f3849d) to head (a3ae556).
Report is 12 commits behind head on edge.

Files with missing lines Patch % Lines
shared-data/js/protocols.ts 0.00% 13 Missing ⚠️
shared-data/js/helpers/index.ts 64.28% 5 Missing ⚠️
...QuickTransferFlow/utils/createQuickTransferFile.ts 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #17560      +/-   ##
==========================================
- Coverage   26.17%   26.16%   -0.02%     
==========================================
  Files        2838     2840       +2     
  Lines      217730   218328     +598     
  Branches     9280     9294      +14     
==========================================
+ Hits        56989    57120     +131     
- Misses     160724   161191     +467     
  Partials       17       17              
Flag Coverage Δ
protocol-designer 18.87% <38.88%> (+0.01%) ⬆️
step-generation 4.36% <25.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...ol-designer/src/file-data/selectors/fileCreator.ts 84.21% <100.00%> (ø)
protocol-designer/src/load-file/migration/8_0_0.ts 97.51% <100.00%> (ø)
...igration/utils/getEquipmentLoadInfoFromCommands.ts 85.00% <ø> (ø)
...ile/migration/utils/getMigrationPositionFromTop.ts 5.88% <ø> (ø)
shared-data/protocol/types/schemaV8/index.ts 100.00% <ø> (ø)
..._shared_data/protocol/models/protocol_schema_v8.py 100.00% <ø> (ø)
...QuickTransferFlow/utils/createQuickTransferFile.ts 0.00% <0.00%> (ø)
shared-data/js/helpers/index.ts 69.58% <64.28%> (-0.64%) ⬇️
shared-data/js/protocols.ts 83.55% <0.00%> (-1.57%) ⬇️

... and 8 files with indirect coverage changes

@SyntaxColoring SyntaxColoring marked this pull request as ready for review February 20, 2025 21:07
@SyntaxColoring SyntaxColoring requested review from a team as code owners February 20, 2025 21:07
@SyntaxColoring SyntaxColoring requested review from koji and jerader and removed request for a team February 20, 2025 21:07
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

I think we are going to want a new protocol schema version for this because labware definitions get embedded into the protocol document directly (as opposed to through commands).

This is because of basically what you're identifying in this PR and in your other work: there are many many places where everything involved assumes labware is schema 2. We're going to be significantly changing the geometry definitions in labware 3; code is going to have to change for it; and without having some way to discriminate them up front, older apps and PD are going to see schema 8 protocols that contain schema 3 labware as just protocols that they should understand that contain invalid labware. We need some way to trigger the new code paths up front.

@SyntaxColoring
Copy link
Contributor Author

Interesting—I've just realized that the validation functions in protocols.ts are only called by tests, not production code? So what does validate a JSON protocol when it's imported into the app or PD?

@SyntaxColoring SyntaxColoring marked this pull request as draft February 21, 2025 15:58
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