feat(protocol-designer): introduce new -locationUpdates #17414

jerader · 2025-02-04T15:54:59Z

Overview

The goal of this PR is so when importing JSON/python protocols back into PD, we can determine the additionalEquipment entities from stepForms instead of reading the commands.

This PR introduces a few new -locationUpdate keys in savedStepForms's INITIAL_DECK_SETUP_STEP step. The locations include:

trashBinLocationUpdate
wasteChuteLocationUpdate
gripperLocationUpdate
stagingAreaLocationUpdate

Each of them include the entity's id as the key and the location as the value. This is for pd/py interop so we can easily grab these additional equipment entities from the step forms instead of reading from the commands

Test Plan and Hands on Testing

Test this out with different variations of deleting/adding gripper, staging areas, trash bin/waste chute. and then import old versions of PD and test that the migration works as expected.

I did smoke test this a lot so i think everything is working as expected.

Changelog

add the new locationUpdate keys in stepForms for py/pd interop and wire it up in the reducer
fix fixtures for the migration cypress test

Risk assessment

low-ish, will be well tested before merging

closes AUTH-1396

codecov · 2025-02-04T16:54:36Z

Codecov Report

Attention: Patch coverage is 42.75956% with 419 lines in your changes missing coverage. Please review.

Project coverage is 17.34%. Comparing base (3d78c1f) to head (4b8fc7e).
Report is 174 commits behind head on edge.

Files with missing lines	Patch %	Lines
...tion/utils/getAdditionalEquipmentLocationUpdate.ts	1.92%	204 Missing ⚠️
protocol-designer/src/step-forms/reducers/index.ts	7.03%	119 Missing ⚠️
...ile/migration/utils/getMigrationPositionFromTop.ts	3.03%	32 Missing ⚠️
protocol-designer/src/load-file/migration/8_5_0.ts	0.00%	28 Missing ⚠️
protocol-designer/cypress/support/import.ts	0.00%	25 Missing ⚠️
...organisms/EditInstrumentsModal/PipetteOverview.tsx	0.00%	7 Missing ⚠️
...designer/src/step-forms/actions/additionalItems.ts	25.00%	3 Missing ⚠️
...rotocol-designer/src/step-forms/selectors/index.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             edge   #17414       +/-   ##
===========================================
- Coverage   63.62%   17.34%   -46.29%     
===========================================
  Files         303     3130     +2827     
  Lines       16164   226253   +210089     
  Branches        0     6889     +6889     
===========================================
+ Hits        10285    39242    +28957     
- Misses       5879   187011   +181132

Flag	Coverage Δ
g-code-testing	`?`
hardware	`?`
protocol-designer	`17.34% <42.75%> (?)`
shared-data	`?`
system-server	`?`
update-server	`?`
usb-bridge	`?`

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

Files with missing lines	Coverage Δ
protocol-designer/src/constants.ts	`99.31% <100.00%> (ø)`
protocol-designer/src/file-types.ts	`71.42% <ø> (ø)`
...rotocol-designer/src/step-forms/test/utils.test.ts	`100.00% <100.00%> (ø)`
protocol-designer/src/step-forms/utils/index.ts	`78.50% <100.00%> (ø)`
...r/src/steplist/formLevel/getDefaultsForStepType.ts	`92.89% <100.00%> (ø)`
...list/formLevel/test/getDefaultsForStepType.test.ts	`100.00% <100.00%> (ø)`
...rotocol-designer/src/step-forms/selectors/index.ts	`47.73% <0.00%> (ø)`
...designer/src/step-forms/actions/additionalItems.ts	`16.66% <25.00%> (ø)`
...organisms/EditInstrumentsModal/PipetteOverview.tsx	`1.09% <0.00%> (ø)`
protocol-designer/cypress/support/import.ts	`0.00% <0.00%> (ø)`
... and 4 more

... and 3415 files with indirect coverage changes

ddcc4 · 2025-02-04T23:12:06Z

protocol-designer/src/load-file/migration/utils/getAdditionalEquipmentLocationUpdate.ts

    trashBinLocationUpdate =
-      unoccupiedSlotForTrash === WASTE_CHUTE_CUTOUT ? {} : hardcodedTrashFlex
+      unoccupiedSlotForTrash === WASTE_CHUTE_CUTOUT


Hey, what does unoccupiedSlotForTrash mean?

all this logic was copied over from the reducer. This is that bug you noticed where trashbins are not generated back to where the user had them if the user doesn't include any pipetting steps in their protocol. So unoccupiedSlotForTrash looks for the first available slot to place the trash bin. It includes the waste chute as well because there was a bug one time where a customer had a labware in every slot except for D3 which had a staging area. trash bins can not go into D3 with a staging area so it had to be a waste chute auto-generated instead.

ddcc4 · 2025-02-04T23:17:18Z

protocol-designer/src/load-file/migration/utils/getAdditionalEquipmentLocationUpdate.ts

@@ -252,11 +222,27 @@ export const getAdditionalEquipmentLocationUpdate = (
    trashBinLocationUpdate = {
      [trashBinId]: trashCutoutId as string,
    }
+    //  in case the user has no pipetting steps, auto-generate a trashBin or wasteChute entity for Flex
  } else if (isFlex && !hasWasteChuteCommands) {


Hm, I'm trying to understand the logic here. From your comment "in case the user has no pipetting steps, auto-generate a trashBin or wasteChute," it sounds like you want this section to run if there is no waste chute commands and no garbage bin commands, but this if-statement only tests for !hasWasteChuteCommands. Should this section run if there is no waste chute commands, but there IS a garbage bin command?

the first if in the statement is checking for if there is a trash bin command in the protocol and populates the trashBinLocationUpdate based off of those commands. This else if is auto-generating a trash bin if there were no pipetting steps for a Flex protocol. It's specifically checking for a flex protocol and making sure there is no waste chute because its possible to have a waste chute instead of a trash bin. You must have 1 though.

ddcc4 · 2025-02-04T23:24:18Z

protocol-designer/fixtures/protocol/8/doItAllV8.json

-          "pythonName": "liquid_0",
-          "liquidGroupId": "1"
+          "liquidGroupId": "0",
+          "pythonName": "liquid_1",


Hey, what's happening over here? I thought the liquid changes happened in the previous PR, so why is the ingredients getting updated here now?

when i updated these fixtures, i imported them back into PD and then exported (so i wouldn't have to manually add in the -locationUpdate info. I think the liquid information doesn't have a set pattern in which it generates the keys, which is why this changed. i don't think that's an issue, right?

It shouldn't be a problem.

I was also suspicious because I saw "pythonName": "liquid_0", and I thought we had decided that the numbering should start from liquid_1, so I was curious how the _0 got there in the first place.

ahh looking at this now, i see that the displayNames didn't get copied over and you're right that the pythonName is wrong... weird. i didn't change the liquidEntities in this pr 🤔 i'll look into it. it seems like a bug with my liquidEntities pr

hmm what's weird is this is the only migration fixture with this issue... hmmmmm

ok i think the actual issue was the migration fixture file was a bit corrupted. i fixed it by using the file from a much older commit (before i made the ingredient changes) and importing that into the app and exporting 😄

ddcc4

Hey, still working through this, but wanted to send you some more questions I had:

ddcc4 · 2025-02-05T16:57:10Z

protocol-designer/fixtures/protocol/8/doItAllV8.json

-          "pythonName": "liquid_0",
-          "liquidGroupId": "1"
+          "liquidGroupId": "0",
+          "pythonName": "liquid_1",


It shouldn't be a problem.

I was also suspicious because I saw "pythonName": "liquid_0", and I thought we had decided that the numbering should start from liquid_1, so I was curious how the _0 got there in the first place.

ddcc4 · 2025-02-05T17:21:10Z

protocol-designer/src/load-file/migration/8_5_0.ts

+          },
+        }
+      }
+      return acc


Hm, I'm confused: Is updatedInitialStep supposed to be the entire savedStepForms with __INITIAL_DECK_SETUP_STEP__ replaced, or is it supposed to be just the __INITIAL_DECK_SETUP_STEP__ entry?

In the base case down here, where you just return acc, won't that just discard any entries that are NOT __INITIAL_DECK_SETUP_STEP__? So I don't understand why this should use a reducer in the first place.

its only updating the __INITIAL_DECK_SETUP_STEP__ entry! If you look at the return at the end of the function, i'm spreading in the remaining stepForms:

savedStepForms: { ...designerApplication.data.savedStepForms, ...updatedInitialStep, ...savedStepsWithUpdatedMoveLiquidFields, ...savedStepsWithUpdatedMixFields, },

ddcc4 · 2025-02-05T17:35:56Z

protocol-designer/src/load-file/migration/utils/getAdditionalEquipmentLocationUpdate.ts

+    return null
+  }
+
+  for (const stepForm of Object.values(savedStepForms)) {


Hey, how did you decide on the approach to extract the trashbin from the savedStepForms instead of the commands?

the commands don't have the trashBinId that is associated with the step forms. If we generated new trashBinIds, the protocol would error on the trash bin steps because it would be looking for a different id. Same goes for waste chute.

ddcc4 · 2025-02-05T17:37:57Z

protocol-designer/src/load-file/migration/utils/getAdditionalEquipmentLocationUpdate.ts

+      if (stepForm.dropTip_location.toLowerCase().includes('trash')) {
+        return stepForm.dropTip_location
+      }
+      if (stepForm.blowout_location?.toLowerCase().includes('trash')) {


Why is blowout_location special -- why does it need blowout_location?.toLowerCase()?

you can blowout in the source well, destination well, or trash!

ddcc4 · 2025-02-05T17:40:57Z

protocol-designer/src/load-file/migration/utils/getAdditionalEquipmentLocationUpdate.ts

+
+  for (const stepForm of Object.values(savedStepForms)) {
+    if (stepForm.stepType === 'moveLiquid') {
+      if (stepForm.dispense_labware.toLowerCase().includes('trash')) {


Hm, should we add some more structure to the includes string to make sure we don't get false matches? Like, do trashbins always have the ID :trash?

(Hehe, I searched through my dictionary, and found words like intrashop and ultrashrewd.)

I guess the ids should always end in :trashBin so i can extend this to look for trashBin. Alternatively, i could see if dispense_labware is listed in the load labware commands (since labwareId is a load labware command param. If it is not, then we know the dispense is happening in a trashBin or a wasteChute. But to narrow down if it is a trashBin or wasteChute, I don't think there is any other way since we have no idea what the trashBin or wasteChute id is without searching for it like this.

ddcc4 · 2025-02-05T19:23:23Z

protocol-designer/src/step-forms/reducers/index.ts

+          return {
+            ...form,
+            [locationUpdate]:
+              Object.keys(updatedLocation).length > 0 ? updatedLocation : {},


Is the length > 0 necessary? Wouldn't [locationUpdate]: updatedLocation be correct in all cases?

I think it is since you can have multiple staging areas!

jerader added 6 commits February 4, 2025 09:32

start to add additionalEquipmentLocationUpdate

90f2792

update migration

ab26970

fix lint

ada684f

feat(protocol-designer): introduce new -locationUpdates

b3f6dd4

closes AUTH-1396

remvoe console.log

edfdbb7

fix unit test

0950225

remove unnused type export

c2ce56d

jerader marked this pull request as ready for review February 4, 2025 17:10

jerader requested a review from a team as a code owner February 4, 2025 17:10

jerader requested a review from ddcc4 February 4, 2025 17:10

clean up logic for hard-coded trashBin and wasteChute

1f240e0

ddcc4 reviewed Feb 4, 2025

View reviewed changes

jerader mentioned this pull request Feb 5, 2025

feat(protocol-designer): add designerApplication labware, pipettes, m… #17432

Merged

ddcc4 reviewed Feb 5, 2025

View reviewed changes

jerader added 3 commits February 5, 2025 13:12

fix load-file ingredients type

6b63aae

fix lint

07c74a1

fix doItAllV8

4b8fc7e

ddcc4 approved these changes Feb 5, 2025

View reviewed changes

jerader merged commit 9d28971 into edge Feb 5, 2025
14 of 16 checks passed

jerader deleted the pd_additional-equipment-location-update branch February 5, 2025 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol-designer): introduce new -locationUpdates #17414

feat(protocol-designer): introduce new -locationUpdates #17414

jerader commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

ddcc4 Feb 4, 2025

jerader Feb 5, 2025

ddcc4 Feb 4, 2025

jerader Feb 5, 2025

ddcc4 Feb 4, 2025

jerader Feb 5, 2025

ddcc4 Feb 5, 2025

jerader Feb 5, 2025 •

edited

Loading

jerader Feb 5, 2025

jerader Feb 5, 2025

ddcc4 left a comment

ddcc4 Feb 5, 2025

ddcc4 Feb 5, 2025

jerader Feb 5, 2025

ddcc4 Feb 5, 2025

jerader Feb 5, 2025

ddcc4 Feb 5, 2025

jerader Feb 5, 2025

ddcc4 Feb 5, 2025

jerader Feb 5, 2025

ddcc4 Feb 5, 2025

jerader Feb 5, 2025

feat(protocol-designer): introduce new -locationUpdates #17414

feat(protocol-designer): introduce new -locationUpdates #17414

Conversation

jerader commented Feb 4, 2025 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Risk assessment

codecov bot commented Feb 4, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcc4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

jerader Feb 5, 2025 •

edited

Loading