refactor(step-generation, protocol-designer): create HydratedFormData & update atomic command args to match v10 params #17308

jerader · 2025-01-18T02:56:55Z

Overview

Was cleaning up StepArgs a bit so it matches the command params a bit better and then ended up creating HydratedFormData type and wired it up. This resulted in a bit of a refactor and changing some arg names.

Additionally, this PR updates some of the atomic command types to using the command's Param types to the latest command schema and updates the stepArgs accordingly. The reason why I did this is because some atomic command args were still using outdated params (from command schema V4, V6, etc. When we are currently on command schema V10)

I also deleted a bunch of stuff in generateSubsteps because we no longer have substeps for every step type now. only thermocycler and mix and move liquid have substeps

Test Plan and Hands on Testing

Smoke test creating protocols in PD, in particular make sure:

you can properly generate form errors in any form type
disabled fields properly work and dependent fields properly work as well in forms
test that the step-generation commands are what you would expect them to be, i especially changed the module compound commands so test TC, heater-shaker, magnetic module, plate reader, and temperature modules.

Changelog

create HydratedFormData and wire it in
make sure every hydrated form data type includes description and name and create the form data for the thermocycler which was previously never created for some reason.
update the atomic command args to match the command schema v10 command param types. This resulted in some changes like module to moduleId, temperature to celsius, and a few other things I think were changed that were more one-offs
fix all the type errors
update the tests

Review requests

lmk if anything seems weird to you!

Risk assessment

medium - this touches a bunch of code, if all tests pass (most importantly the migration files) then I am confident that these changes don't break anything.

codecov · 2025-01-18T13:51:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.84%. Comparing base (0e7b516) to head (9ded968).
Report is 4 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #17308   +/-   ##
=======================================
  Coverage   73.84%   73.84%           
=======================================
  Files          43       43           
  Lines        3304     3304           
=======================================
  Hits         2440     2440           
  Misses        864      864

Flag	Coverage Δ
shared-data	`73.84% <ø> (ø)`

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

jerader · 2025-01-21T15:40:08Z

protocol-designer/src/form-types.ts

@@ -175,19 +175,7 @@ export interface BlowoutFields {
 export interface ChangeTipFields {
  changeTip?: ChangeTipOptions
 }
-export type MixForm = AnnotationFields &


note: MixForm wasn't in use anywhere

So what's the deal with some of the form classes that are named "legacy," like HydratedMixFormDataLegacy?

the HydratedMoveLiquidFormDataLegacy is not in use. HydratedMixFormDataLegacy was in use, so i just removed Legacy because it matches the pattern we are using for the other form data types. Not sure why Legacy was added initially...

jerader · 2025-01-21T15:41:04Z

protocol-designer/src/form-types.ts

@@ -397,13 +396,7 @@ export interface HydratedAbsorbanceReaderFormData {
  referenceWavelength: number | null
  wavelengths: number[] | null
 }
-// TODO: Ian 2019-01-17 Moving away from this and towards nesting all form fields
-// inside `fields` key, but deprecating transfer/consolidate/distribute is a pre-req
-export type HydratedMoveLiquidFormDataLegacy = AnnotationFields &


this was also not in use anywhere, plus it was a legacy type

jerader · 2025-01-21T16:25:54Z

step-generation/src/commandCreators/atomic/absorbanceReaderCloseLid.ts

-  prevRobotState
-) => {
+export const absorbanceReaderCloseLid: CommandCreator<
+  AbsorbanceReaderOpenLidCreateCommand['params']


note: the reason why I updated the args type for a lot of atomic commands is because they didn't align with the command schema v10 command params. This pattern is established in a few atomic commands already, so I extended it to match more atomic commands (just realized i need to extend to match the pipetting atomic commands, maybe i'll do that in a follow up). With this update in pattern, it ensures the args match the latest command schema params exactly which makes atomic commands cleaner.

OK, to make sure I understand:

Params is the "bigger" object that contains the Args, among other things?

Why do you have named Params interfaces for things like AspirateInPlaceParams down below, but not for AbsorbanceReaderOpenLidCreateCommand['params'] here?

ddcc4 · 2025-01-21T19:00:47Z

protocol-designer/src/form-types.ts

-    pickUpTip_wellNames?: string[] | null
-    preWetTip?: boolean | null
-  }
-  description?: string | null


I think this PR makes a lot of sense for consistency. But how does this not break all the existing code that used fields like description? (and all the old saved JSON protocols)

Good question, i updated the HydratedFormData types which are only used for ...inArgs fns and various form utils such as getDisabledFields. the steps in a JSON file uses FormData, before it was hydrated. Here is the util that hydrates the certain fields, if a field is not hydrated, it is left as it was in form data.

An example of a hydrated field is labware in mix and move liquid. When a user selects it in a form, it is just the labwareId. When hydrated, that labware id is mapped against the labware entities for its match and is hydrated as a LabwareEntity.

So a JSON file shouldn't break when reimported because the fields are just the form data, not hydrated yet.

Ah! I wasn't aware that the Hydrated*Data was a separate thing from the data in the imported form.

In terms of description, it was never wired up in the form or FormData. so a JSON file shouldn't have that field. But also, i only edited the HydratedFormData for that, i think. 😄

ddcc4 · 2025-01-21T19:02:14Z

protocol-designer/src/form-types.ts

  times?: number | null
 }
 export type MagnetAction = 'engage' | 'disengage'
 export type HydratedMagnetFormData = AnnotationFields & {
  engageHeight: string | null
  id: string
  magnetAction: MagnetAction
-  moduleId: string | null
+  moduleId: string


Again, I don't understand how this doesn't break old files? Like, what was it possible to export a file that had the moduleId missing? What happens if you try to import it after this change?

ddcc4 · 2025-01-21T19:14:26Z

protocol-designer/src/form-types.ts

  absorbanceReaderFormType: AbsorbanceReaderFormType | null
-  fileName: string | null
+  fileName?: string | null


OK. So if this data structure is the form after hydration, what is the purpose of allowing both a missing fileName and a null fileName here?

oh ya good question! let me see if i can remove the undefined

ddcc4 · 2025-01-21T20:12:56Z

step-generation/src/commandCreators/atomic/absorbanceReaderCloseLid.ts

-  prevRobotState
-) => {
+export const absorbanceReaderCloseLid: CommandCreator<
+  AbsorbanceReaderOpenLidCreateCommand['params']


OK, to make sure I understand:

Params is the "bigger" object that contains the Args, among other things?

Why do you have named Params interfaces for things like AspirateInPlaceParams down below, but not for AbsorbanceReaderOpenLidCreateCommand['params'] here?

ddcc4 · 2025-01-21T20:17:20Z

protocol-designer/src/steplist/formLevel/stepFormToArgs/mixFormToArgs.ts

 import type { MixArgs } from '@opentrons/step-generation'
 type MixStepArgs = MixArgs
 export const mixFormToArgs = (
-  hydratedFormData: HydratedMixFormDataLegacy
+  hydratedFormData: HydratedMixFormData


This might be the wrong PR for this question, but we still need a verdict on what we do about form fields that are lost when we go from the form -> args, right?

Here for example, the form has fields like mix_wellOrder_first and mix_wellOrder_second that don't make it into the MixStepArgs. But that means that if we generate Python from the MixStepArgs, there's no way for the Python generator to know what the original values of mix_wellOrder_first/mix_wellOrder_second were, which means there's no way for the Python generator to emit them into the Python file so that we can re-hydrate them on import.

for sure, i'll think about that more in a follow up!

… & update atomic command args to match v10 params (#17308) Was cleaning up `StepArgs` a bit so it matches the command params a bit better and then ended up creating `HydratedFormData` type and wired it up. This resulted in a bit of a refactor and changing some arg names. Additionally, this PR updates some of the atomic command types to using the command's `Param` types to the latest command schema and updates the stepArgs accordingly. The reason why I did this is because some atomic command args were still using outdated params (from command schema V4, V6, etc. When we are currently on command schema V10)

jerader marked this pull request as ready for review January 21, 2025 13:37

jerader requested a review from a team as a code owner January 21, 2025 13:37

jerader requested review from koji, ddcc4 and ncdiehl11 and removed request for a team January 21, 2025 13:37

jerader commented Jan 21, 2025

View reviewed changes

jerader changed the title ~~refactor(step-generation, protocol-designer): create HydratedFormData~~ refactor(step-generation, protocol-designer): create HydratedFormData & update atomic command args to match v10 params Jan 21, 2025

jerader commented Jan 21, 2025

View reviewed changes

jerader added 9 commits January 21, 2025 12:19

clean up stepArgs and make hydratedFormData

901bf47

fix whitescreens in disabledFields

9e6f37e

cleanup

6fe1f94

fix prettier

5ea3bb6

fix lint

cdfb51e

remove fields from HydratedMoveLiquidFormData

ca58f2a

rename HydratedMixFormDataLegacy to HydratedMixFormData

54ad873

update inPlace atomic command args to use command params

84d33de

fix failed checks and clean up rebase

9ded968

jerader force-pushed the sg_step-args-cleanup branch from 871f486 to 9ded968 Compare January 21, 2025 19:02

ddcc4 reviewed Jan 21, 2025

View reviewed changes

ddcc4 approved these changes Jan 21, 2025

View reviewed changes

address feedback

a08219b

jerader merged commit 73bdb33 into edge Jan 21, 2025
49 checks passed

jerader deleted the sg_step-args-cleanup branch January 21, 2025 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(step-generation, protocol-designer): create HydratedFormData & update atomic command args to match v10 params #17308

refactor(step-generation, protocol-designer): create HydratedFormData & update atomic command args to match v10 params #17308

jerader commented Jan 18, 2025 •

edited

Loading

codecov bot commented Jan 18, 2025 •

edited

Loading

jerader Jan 21, 2025

ddcc4 Jan 21, 2025

jerader Jan 21, 2025 •

edited

Loading

jerader Jan 21, 2025

jerader Jan 21, 2025 •

edited

Loading

ddcc4 Jan 21, 2025

ddcc4 Jan 21, 2025

jerader Jan 21, 2025 •

edited

Loading

jerader Jan 21, 2025

ddcc4 Jan 21, 2025

jerader Jan 21, 2025 •

edited

Loading

ddcc4 Jan 21, 2025

ddcc4 Jan 21, 2025

jerader Jan 21, 2025

ddcc4 Jan 21, 2025

ddcc4 Jan 21, 2025

jerader Jan 21, 2025

refactor(step-generation, protocol-designer): create HydratedFormData & update atomic command args to match v10 params #17308

refactor(step-generation, protocol-designer): create HydratedFormData & update atomic command args to match v10 params #17308

Conversation

jerader commented Jan 18, 2025 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

codecov bot commented Jan 18, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader Jan 21, 2025 • edited Loading

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 Jan 18, 2025 •

edited

Loading

codecov bot commented Jan 18, 2025 •

edited

Loading

jerader Jan 21, 2025 •

edited

Loading

jerader Jan 21, 2025 •

edited

Loading

jerader Jan 21, 2025 •

edited

Loading

jerader Jan 21, 2025 •

edited

Loading