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(api, app, shared-data, pd): use new flex pipette names in backend & clients #13082

Merged
merged 10 commits into from
Jul 13, 2023

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Jul 11, 2023

Overview

#12966 updated the Flex's pipette names from .._gen3 to .._flex which created a conflict with pipette names expected by the engine & PAPI. This PR updates the engine to switch to using the new names, while adding a temporary backwards compatible shim to python API to convert the old names to new ones.

Ticket to remove the backwards compatibility shim before launch: RSS-284

Update after PR reviews:

  • also added new PAPI-only pipette names from this comment.
  • replaced gen3 references in v1 pipette definitions to flex
  • added an integration test on robot server to test a minimal flex protocol

Test Plan

  • Test on a flex bot and make sure:
  • you can calibrate pipettes and previously calibrated pipettes show the correct calibration
  • you can analyze & run python protocols that load pipettes with both the old and the new names
  • protocol setup is able to detect & match the correct pipette attached
  • LPC runs correctly

Review requests

This approach would cause existing analyses & run logs on the robot to raise errors. It would require users to reset their database. Is it okay to have to do that?

Risk assessment

This is basically a fix for the breaking change introduced in the above linked PR so that way it reduces risk, but it also fixes it by switching the engine over to use the new names so we should be careful with merging it in.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #13082 (3ed3374) into edge (5394203) will decrease coverage by 0.08%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13082      +/-   ##
==========================================
- Coverage   72.20%   72.13%   -0.08%     
==========================================
  Files        1559     1543      -16     
  Lines       51750    51379     -371     
  Branches     3229     3146      -83     
==========================================
- Hits        37366    37061     -305     
+ Misses      13875    13812      -63     
+ Partials      509      506       -3     
Flag Coverage Δ
app 46.07% <ø> (+2.37%) ⬆️
components 64.42% <100.00%> (-2.47%) ⬇️
g-code-testing 96.44% <ø> (ø)
labware-library 49.61% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 45.84% <100.00%> (ø)
shared-data 79.62% <100.00%> (+1.33%) ⬆️
step-generation 87.17% <0.00%> (ø)

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

Impacted Files Coverage Δ
api/src/opentrons/protocol_api/protocol_context.py 91.80% <ø> (ø)
components/src/instrument/InstrumentDiagram.tsx 91.66% <ø> (ø)
...-generation/src/commandCreators/atomic/aspirate.ts 93.02% <0.00%> (ø)
...-generation/src/commandCreators/atomic/dispense.ts 86.48% <0.00%> (ø)
...rc/commandCreators/atomic/heaterShakerOpenLatch.ts 81.81% <0.00%> (ø)
...eneration/src/commandCreators/atomic/moveToWell.ts 92.85% <0.00%> (ø)
...eneration/src/commandCreators/atomic/replaceTip.ts 46.15% <0.00%> (ø)
components/src/instrument/PipetteSelect.tsx 49.01% <100.00%> (-11.96%) ⬇️
protocol-designer/src/components/modals/utils.ts 64.70% <100.00%> (ø)
...a/python/opentrons_shared_data/pipette/__init__.py 94.73% <100.00%> (ø)
... and 1 more

... and 36 files with indirect coverage changes

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.

Code changes make sense to me. Thanks for your quick response on this.

This approach would cause existing analyses & run logs on the robot to raise errors. It would require users to reset their database. Is it okay to have to do that?

👍 Yeah, this is the same sort of thing as with #12571. If we have to have a breaking API change like this, we may as well rip off the band-aid while it's still internal.

Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

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

when running analysis on a protocol that uses a pipette load name of p1000_single_flex i get the following analysis error:

ProtocolEngineError [line 28]: Error 4000 GENERAL_ERROR (ProtocolEngineError): UnexpectedProtocolError: 'p1000_single_flex_v1'

@sanni-t sanni-t marked this pull request as ready for review July 12, 2023 00:13
@sanni-t sanni-t requested review from a team as code owners July 12, 2023 00:13
Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

Since you already have this PR open, can you go ahead and do the requirements listed in this ticket?

Make sure the "gen3" version of the pipettes get passed into the hardware controller.

@@ -21,16 +21,16 @@
"p20_multi_gen2",
"p50_single",
"p50_multi",
"p50_single_gen3",
"p50_multi_gen3",
"p50_single_flex",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the gen3 in these dev types for now and also test on a robot w/ app to see if we need to additionally add the _flex models to schema v1 for pipettes (hopefully we do not).

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

Approving for now, but see comment I added.

@jerader jerader requested a review from a team as a code owner July 13, 2023 13:41
@jerader jerader requested review from smb2268 and removed request for a team July 13, 2023 13:41
Copy link
Member Author

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Successfully tested pipette attach, calibration, LPC & running python protocol with flex_8channel_1000 & p50_single_gen3 pipettes without any issues!

@sanni-t sanni-t changed the title feat(api): use new '_flex' pipette names in engine feat(api, app, shared-data, pd): use new flex pipette names in backend & clients Jul 13, 2023
@sanni-t sanni-t merged commit cb2a7be into edge Jul 13, 2023
@smb2268 smb2268 deleted the api-accept_new_flex_pipette_names branch July 13, 2023 18:28
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.

6 participants