fix(api): Remove obsolete "_gen3" pipette names #13514

SyntaxColoring · 2023-09-11T21:16:53Z

Overview

Removes obsolete pipette names like p50_single_gen3. Closes RSS-284.

Changelog

Remove some transitional code in various places that was accepting strings like p50_single_gen3.
Update a whole bunch of tests to use the new official strings, which are either like flex_1channel_50 for the Python Protocol API, or p50_single_flex for HTTP and the back-end.

Test Plan

Analyze a protocol that does protocol.load_instrument("p50_single_gen3", mount="left") and confirmed that it errored out.
With the robot software, ODD, and local app on this branch, play around with uploading and running protocols and make sure nothing has broken. Also attach and detach instruments.

Review requests

I did a global find+replace, but I'm not sure whether it's appropriate to change all of these places. I'm vaguely aware that there's some distinction now between Python Protocol API load names and the names used by things deeper in the back-end, but I'm not sure exactly where that division is drawn. See my review comments.

Risk assessment

Medium. We need to be careful that this doesn't somehow affect the HTTP API.

codecov · 2023-09-11T21:20:47Z

Codecov Report

Merging #13514 (1cfa332) into chore_release-7.0.0 (bcf6cbd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           chore_release-7.0.0   #13514   +/-   ##
====================================================
  Coverage                71.29%   71.30%           
====================================================
  Files                     2419     2419           
  Lines                    68096    68096           
  Branches                  7913     7913           
====================================================
+ Hits                     48552    48557    +5     
+ Misses                   17690    17685    -5     
  Partials                  1854     1854

Flag	Coverage Δ
app	`68.90% <ø> (ø)`
g-code-testing	`96.44% <ø> (ø)`
hardware-testing	`∅ <ø> (∅)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`46.09% <ø> (ø)`
react-api-client	`69.30% <ø> (ø)`
shared-data	`73.91% <100.00%> (ø)`
step-generation	`87.18% <ø> (ø)`

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

Files Changed	Coverage Δ
.../python/opentrons_shared_data/pipette/dev_types.py	`100.00% <ø> (ø)`
...a/python/opentrons_shared_data/pipette/__init__.py	`94.73% <100.00%> (ø)`

... and 2 files with indirect coverage changes

robot-server/simulators/test-flex.json

protocol-designer/src/components/modals/CreateFileWizard/__tests__/ModulesAndOtherTile.test.tsx

api/tests/opentrons/hardware_control/test_instruments.py

api-client/src/pipettes/__fixtures__/index.ts

…names I'm getting CI errors in what looks like an unrelated gripper test. Maybe this will fix them?

Laura-Danielle

If you change the tsx files, you'll need to modify pipetteModelSpecs.json names as well from p1000_single_flex to the new name.

SyntaxColoring · 2023-09-12T19:37:41Z

If you change the tsx files, you'll need to modify pipetteModelSpecs.json names as well from p1000_single_flex to the new name.

@Laura-Danielle Sorry, I'm confused. I understand we want to change names like p1000_single_gen3 to flex_1channel_1000 in the Python Protocol API. I thought names like p1000_single_flex were still the modern names used by the backend. Is that not true?

sanni-t

All the clients should use names like p1000_single_flex, etc.. since those are the names we are using in backend everywhere except in the user-facing API.
Maybe these client tests don't interface with actual backend and shared data and that's why they might not be erroring out when using the _gen3 names..? And looks like I might have missed them when I changed all .._gen3 instances to .._flex in this PR

As for the use in hardware control/ hardware testing, those too should change to .._flex but that's less critical. You can check how many tests you'll need to update by removing gen3 from this line and running the tests. That is the line that looks up the actual pipette model from the names.

Laura-Danielle · 2023-09-13T13:13:47Z

@SyntaxColoring the front-end never ported over to getting its info from the new shared-data format. I think someone from JS could better confirm what relies on the old config and what is comparing things directly with the backend (if that makes sense).

…names More hope for CI fixes.

shared-data/python/opentrons_shared_data/pipette/dev_types.py

SyntaxColoring · 2023-09-14T18:58:43Z

api-client/src/instruments/__fixtures__/index.ts

+
+export const instrumentsResponseLeftPipetteFixture = {
+  data: { channels: 1, min_volume: 1, max_volume: 50 },
+  instrumentModel: 'p1000_single_v3.0',
+  instrumentName: 'p1000_single_flex',
+  instrumentType: 'pipette',
+  mount: 'left',
+  serialNumber: 'abc',
+  subsystem: 'pipette_left',
+  ok: true,
+}
+
+export const instrumentsResponseRightPipetteFixture = {
+  data: { channels: 1, min_volume: 1, max_volume: 50 },
+  instrumentModel: 'p1000_single_v3.0',
+  instrumentName: 'p1000_single_flex',
+  instrumentType: 'pipette',
+  mount: 'right',
+  serialNumber: 'cba',
+  subsystem: 'pipette_right',
+  ok: true,
+}


These are test fixtures for the /instruments endpoints, but they were in the directory for the /pipettes endpoints. I'm moving them here and correcting their instrumentName from p1000_single_gen3 to p1000_single_flex.

I supposed we all felt that, one way or another.

SyntaxColoring · 2023-09-14T19:19:21Z

@SyntaxColoring the front-end never ported over to getting its info from the new shared-data format. I think someone from JS could better confirm what relies on the old config and what is comparing things directly with the backend (if that makes sense).

I've taken a closer look at the TypeScript files I'm modifying here. They're used for modeling HTTP responses, which return names like p50_single_flex. So I've changed the TypeScript files to match.

I'm not sure if anything in the front-end is still relying on the old shared-data format, but if there is, this PR doesn't touch it.

TamarZanzouri · 2023-09-14T20:33:09Z

robot-server/simulators/test-flex.json

@@ -4,19 +4,11 @@
  "attached_instruments": {
    "right": {
      "model": "p1000_single_3.4",
-      "id": "321",


are these changes necessary for this pr?

I think so. See #13514 (comment).

TamarZanzouri

LGTM! Thank you for clarifying it :-)

CaseyBatten · 2023-09-15T18:31:38Z

Ran tests on this branch utilizing each naming convention on RSS Flex in the office. Good results overall, code samples for protocols to run can be found at the following comment under the RSS-284 Issue:
https://opentrons.atlassian.net/browse/RSS-284?focusedCommentId=88046

Note that tests for the following are commented out: p50_single_flex, p1000_single_flex, p50_multi_flex, p1000_multi_flex

Upon running the Single channel test script, the following executions occur:

call to p50_single_gen3 denied - this is due to the name not existing and the try-catch entering an exception
Load Flex 1-Channel 50 uL in Left mount succeeds, lifts a pipette, and returns it
call to p1000_single_gen3 denied - this is due to the name not existing and the try-catch entering an exception
Load Flex 1-Channel 1000 uL in Right Mount succeeds, lifts a pipette and returns it

This is the correct behavior, old naming conventions fail their attempts to assign to the Left mount.
If the file is modified to remove the p50_single_flex and p1000_single_flex try-catch statements from their commeted out status, the following executions occur:

call to p50_single_gen3 denied - this is due to the name not existing and the try-catch entering an exception
Load Flex 1-Channel 50uL in left mount succeeds, lifts a pipette, and returns it. This time this is happening with the "p50_single_flex" name being assigned to the hardware.
call to flex_1channel_50 denied - This is an unusual case, but is correct. The Left mount is now already occupied as the name "p50_single_flex" was used in the device setup and already accepted.
call to p1000_single_gen3 denied -this is due to the name not existing and the try-catch entering an exception
Load Flex 1-Channel 1000uL in Right mount succeeds, lifts a pipette, and returns it. This time this is happening with the "p1000_single_flex" name being assigned to the hardware.
call to flex_1channel_1000 denied - This is the same unusual case, once again correct. The Right mount is now already occupied as the name "p50_single_flex" was used in the device setup and already accepted.

When running the attached 8 channel protocol the same results are encountered at each equivalent step, including the intermediate exception cases when the p50_multi_flex and p1000_multi_flex names are re-enabled causing the standard flex naming conventions to be denied on the next step.

Overall, each step of this worked as intended on a live flex system, and the edge case naming conventions in the middle can be handled in other ways if at all.

…names Mmmmaybe this fixes G-code testing?

Implemented these changes. Discussed in Slack and in-person.

sanni-t

🙌

Remove _gen3 pipette names.

d1e04b2

SyntaxColoring changed the base branch from edge to chore_release-7.0.0 September 11, 2023 21:17

Global search and replace.

a59fa7e

SyntaxColoring commented Sep 11, 2023

View reviewed changes

SyntaxColoring marked this pull request as ready for review September 11, 2023 22:19

SyntaxColoring requested review from a team as code owners September 11, 2023 22:19

SyntaxColoring requested review from sanni-t and Laura-Danielle September 12, 2023 14:00

SyntaxColoring added 2 commits September 12, 2023 10:16

Update load name construction in hardware_testing.

dbfbadc

Merge branch 'chore_release-7.0.0' into remove_obsolete_flex_pipette_…

bbf7d29

…names I'm getting CI errors in what looks like an unrelated gripper test. Maybe this will fix them?

This comment was marked as resolved.

Sign in to view

Laura-Danielle reviewed Sep 12, 2023

View reviewed changes

sanni-t previously requested changes Sep 12, 2023

View reviewed changes

SyntaxColoring marked this pull request as draft September 13, 2023 14:22

SyntaxColoring added 4 commits September 13, 2023 11:34

Remove a todo comment that we no longer think we're going to pursue?

f41f211

Remove "gen3" from dummy_model_for_name

1b44739

Update comment.

5e7d7fe

Merge branch 'chore_release-7.0.0' into remove_obsolete_flex_pipette_…

7bc790c

…names More hope for CI fixes.

SyntaxColoring commented Sep 13, 2023

View reviewed changes

shared-data/python/opentrons_shared_data/pipette/dev_types.py Show resolved Hide resolved

SyntaxColoring added 4 commits September 14, 2023 14:52

Delete obsolete fields from simulator configs.

6616fa2

Make TS mock more obvious.

98119b3

Move /instruments fixtures to the /instruments fixtures folder.

b402ede

/instruments fixtures should use HTTP strings, not PAPI strings.

f040a0d

SyntaxColoring commented Sep 14, 2023

View reviewed changes

Now I am become lint, destroyer of worlds.

14c468b

I supposed we all felt that, one way or another.

SyntaxColoring marked this pull request as ready for review September 14, 2023 19:14

SyntaxColoring requested a review from a team as a code owner September 14, 2023 19:14

SyntaxColoring requested review from jerader and sanni-t and removed request for a team September 14, 2023 19:14

TamarZanzouri reviewed Sep 14, 2023

View reviewed changes

TamarZanzouri approved these changes Sep 14, 2023

View reviewed changes

SyntaxColoring added 2 commits September 15, 2023 14:07

Flesh out ensure_pipette_name() test cases.

f946d1b

Fix accidental acceptance of names like p1000_single_flex.

930ca15

Merge branch 'chore_release-7.0.0' into remove_obsolete_flex_pipette_…

1cfa332

…names Mmmmaybe this fixes G-code testing?

SyntaxColoring merged commit d9509cd into chore_release-7.0.0 Sep 15, 2023
60 of 63 checks passed

SyntaxColoring deleted the remove_obsolete_flex_pipette_names branch September 15, 2023 19:56

sanni-t approved these changes Sep 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): Remove obsolete "_gen3" pipette names #13514

fix(api): Remove obsolete "_gen3" pipette names #13514

SyntaxColoring commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

This comment was marked as resolved.

Laura-Danielle left a comment

SyntaxColoring commented Sep 12, 2023

sanni-t left a comment •

edited

Loading

Laura-Danielle commented Sep 13, 2023

SyntaxColoring Sep 14, 2023

SyntaxColoring commented Sep 14, 2023

TamarZanzouri Sep 14, 2023

SyntaxColoring Sep 14, 2023

TamarZanzouri left a comment

CaseyBatten commented Sep 15, 2023

sanni-t left a comment

fix(api): Remove obsolete "_gen3" pipette names #13514

fix(api): Remove obsolete "_gen3" pipette names #13514

Conversation

SyntaxColoring commented Sep 11, 2023 • edited Loading

Overview

Changelog

Test Plan

Review requests

Risk assessment

codecov bot commented Sep 11, 2023 • edited Loading

Codecov Report

This comment was marked as resolved.

Laura-Danielle left a comment

Choose a reason for hiding this comment

SyntaxColoring commented Sep 12, 2023

sanni-t left a comment • edited Loading

Choose a reason for hiding this comment

Laura-Danielle commented Sep 13, 2023

SyntaxColoring Sep 14, 2023

Choose a reason for hiding this comment

SyntaxColoring commented Sep 14, 2023

TamarZanzouri Sep 14, 2023

Choose a reason for hiding this comment

SyntaxColoring Sep 14, 2023

Choose a reason for hiding this comment

TamarZanzouri left a comment

Choose a reason for hiding this comment

CaseyBatten commented Sep 15, 2023

sanni-t left a comment

Choose a reason for hiding this comment

SyntaxColoring commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

sanni-t left a comment •

edited

Loading