feat(protocol-designer): load module in python file #17483

jerader · 2025-02-10T19:43:04Z

addresses a bit of AUTH-1092

Overview

First, I added to the createPythonFile so modules get added. Then I cleaned up createFile for JSON so we use getInvariantContext instead of 4 separate selectors for the entities.

Test Plan and Hands on Testing

Smoke test a protocol by exporting python. Should work as expected

Changelog

clean upcreateFile()
create getLoadModules to return an array of each load module for python

Review requests

I removed what we changed in the reselect.d.ts - i think that's ok since we now only need the 12 selectors in createFile

Risk assessment

low, behind ff

jerader · 2025-02-10T20:11:49Z

protocol-designer/src/file-data/__tests__/createFile.test.ts

@@ -70,6 +70,12 @@ describe('createFile selector', () => {
  afterEach(() => {
    vi.restoreAllMocks()
  })
+  const entities = {
+    moduleEntities: v7Fixture.moduleEntities,


turns out the v7Fixture doesn't have a module - should i add one to the fixture so we can test that it works in the unit test?

If you want to! But like the comment below says, the point of the tests in this file is just to make sure createFile()/createPythonFile() spit out something that looks like a protocol. We'll have other unit tests in other files to make sure that the generated Python code is actually correct for all possible types of modules/labware/etc.

codecov · 2025-02-10T20:16:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.17%. Comparing base (455774e) to head (9e063ce).
Report is 2 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #17483   +/-   ##
=======================================
  Coverage   26.16%   26.17%           
=======================================
  Files        3191     3191           
  Lines      230143   230174   +31     
  Branches     9808     9815    +7     
=======================================
+ Hits        60220    60251   +31     
  Misses     169898   169898           
  Partials       25       25

Flag	Coverage Δ
protocol-designer	`17.43% <100.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	`83.04% <100.00%> (+0.14%)`	⬆️
...col-designer/src/file-data/selectors/pythonFile.ts	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

ddcc4

Hey! So my plan for testing was that in pythonFile.test.ts, we would have fine-grained tests for each of the Python generator functions.

For for this PR, we would add tests for getLoadModules() to pythonFile.test.ts, which would show that the Python code looks like for loading modules.

ddcc4 · 2025-02-10T21:02:14Z

protocol-designer/src/file-data/__tests__/createFile.test.ts

@@ -70,6 +70,12 @@ describe('createFile selector', () => {
  afterEach(() => {
    vi.restoreAllMocks()
  })
+  const entities = {
+    moduleEntities: v7Fixture.moduleEntities,


If you want to! But like the comment below says, the point of the tests in this file is just to make sure createFile()/createPythonFile() spit out something that looks like a protocol. We'll have other unit tests in other files to make sure that the generated Python code is actually correct for all possible types of modules/labware/etc.

ddcc4 · 2025-02-10T21:05:25Z

protocol-designer/src/file-data/selectors/pythonFile.ts

@@ -51,9 +56,32 @@ export function pythonRequirements(robotType: RobotType): string {
  return `requirements = ${formatPyDict(requirements)}`
 }

-export function pythonDefRun(): string {
+function getLoadModules(


Question: Do you prefer your files to go from high-level to low-level functions (so pythonDefRun() comes before getLoadModules()), or low-level functions to high-level functions?

I just want to pick a consistent order so that the file makes sense if someone reads it from top to bottom.

i usually automatically do low-level functions to high-level functions, but i don't think we have a pattern. i can also make a new file for the load functions to live in too.

ddcc4 · 2025-02-10T21:07:43Z

protocol-designer/src/file-data/selectors/pythonFile.ts

+function getLoadModules(
+  moduleEntities: ModuleEntities,
+  moduleRobotState: TimelineFrame['modules']
+): string[] {


Hm, I was hoping that each of these sections like getLoadModules() would return a string instead of an array, decorated with a comment like `# Load modules." to make the Python file easier to read.

oh ya, i can do that! i wasn't sure.

ddcc4 · 2025-02-10T21:09:11Z

protocol-designer/src/file-data/selectors/pythonFile.ts

+    (acc, moduleEntity) => [
+      ...acc,
+      `${moduleEntity.pythonName} = ${PROTOCOL_CONTEXT_NAME}.load_module("${
+        moduleEntity.model


I need to confirm that moduleEntity.model is the correct Python identifier (https://docs.opentrons.com/v2/modules/setup.html#available-modules). Do you know if it is?

some of them are, some of them are not. i'll fix it

the newer modules's python identifier is the module model. so maybe i can map out the conversion for older modules and then assume that all of the unreleased future modules will follow the same module model pattern?

Heh, so we can check with the PAPI team, but I think when I tried it, the API actually accepts a wider range of names than what's in the doc. So moduleEntity.model might actually work -- I'd just need to trace through the Python code to confirm.

oh that is nice to know! i made a fn to map out the old ones but can easily remove it

ddcc4 · 2025-02-10T21:10:43Z

protocol-designer/typings/reselect.d.ts

@@ -1,6 +1,5 @@
 import type { OutputSelector, Selector } from 'reselect'
 declare module 'reselect' {
-  // declaring type for createSelector with 15 selectors because the reselect types only support up to 12 selectors


lol, I had to hack up this file too.

But can you check to see if you need to touch this file at all? My understanding is that for 12 selectors or fewer, the base library already supports that, and you don't need to provide this override.

ddcc4 · 2025-02-10T21:59:03Z

protocol-designer/src/file-data/selectors/pythonFile.ts

+      const pythonIdentifier = getModulePythonIdentifier(module.model)
+      return `${
+        module.pythonName
+      } = ${PROTOCOL_CONTEXT_NAME}.load_module("${pythonIdentifier}", "${


Hey, don't "quote" strings yourself -- could you use formatPyStr() instead? It'll work correctly just in case pythonIdentifier contains quotes or backslashes or other funny characters.

oops, ya will update it!

ddcc4 · 2025-02-10T21:59:49Z

protocol-designer/src/file-data/__tests__/pythonFile.test.ts

+    expect(pythonDefRun(mockInvariantContext, mockInitialRobotState)).toBe(
+      `
+def run(protocol: protocol_api.ProtocolContext):
+


Nit: Could you get rid of the blank line at here before the code in the function?

ddcc4 · 2025-02-10T22:01:15Z

protocol-designer/src/file-data/selectors/utils.ts

@@ -237,3 +238,25 @@ export const getLabwareLoadInfo = (
    {}
  )
 }
+
+export const getModulePythonIdentifier = (model: ModuleModel): string => {


Let's check if this function is really necessary.

But if it is, could you implement it with a {key: value} map instead of switch/case, just to make it more concise?

ddcc4 · 2025-02-10T22:04:50Z

protocol-designer/src/file-data/__tests__/pythonFile.test.ts

+  it('should generate the commands section', () => {
+    expect(pythonDefRun(mockInvariantContext, mockInitialRobotState)).toBe(
+      `
+def run(protocol: protocol_api.ProtocolContext):


Hm, the tests here could become a bit repetitive if we include the def run() in all of them. It might be better to write more targeted unit tests and just test getLoadModules() directly. (We already know that the def run() line gets generated from the higher-level tests.)

ddcc4 · 2025-02-10T22:09:37Z

protocol-designer/src/file-data/selectors/pythonFile.ts

+): string {
+  const pythonModules = Object.values(moduleEntities)
+    .map(module => {
+      const pythonIdentifier = getModulePythonIdentifier(module.model)


Hey, take a look here: https://github.com/Opentrons/opentrons/blob/edge/api/src/opentrons/protocol_api/validation.py#L373

It looks like any of the names in _MODULE_ALIASES or _MODULE_MODELS are acceptable as arguments to protocol.load_module(). Are all of our model names on one of those lists?

Also, could you add a comment with a link to that file so that we remember where we got the names from?

ddcc4 · 2025-02-11T15:46:08Z

protocol-designer/src/file-data/selectors/pythonFile.ts

+          return `${
+            module.pythonName
+          } = ${PROTOCOL_CONTEXT_NAME}.load_module(${formatPyStr(
+            pythonIdentifier


I think just using module.model directly is fine now if we don't need the lookup function.

ddcc4 · 2025-02-11T15:48:11Z

Tried it on your branch. Looks nice!

def run(protocol: protocol_api.ProtocolContext):
    # Load Modules:
    heater_shaker_module_1 = protocol.load_module("heaterShakerModuleV1", "C1")
    temperature_module_1 = protocol.load_module("temperatureModuleV2", "D1")
    thermocycler_module_1 = protocol.load_module("thermocyclerModuleV2", "B1")

jerader added 2 commits February 10, 2025 14:38

feat(protocol-designer): load module in python file

3b3f39c

remove changes to reselect

0184ec0

jerader requested a review from ddcc4 February 10, 2025 19:43

jerader marked this pull request as ready for review February 10, 2025 19:43

jerader requested review from a team as code owners February 10, 2025 19:43

jerader requested review from mjhuff and removed request for a team and mjhuff February 10, 2025 19:43

jerader added 2 commits February 10, 2025 14:51

fix import error

3bb3a87

fix unit test

a08634a

jerader commented Feb 10, 2025

View reviewed changes

ddcc4 reviewed Feb 10, 2025

View reviewed changes

address feedback, add to test

4082495

jerader requested a review from ddcc4 February 10, 2025 21:52

ddcc4 reviewed Feb 10, 2025

View reviewed changes

account for no modules

a7ba157

ddcc4 reviewed Feb 10, 2025

View reviewed changes

refine loadModules fn and test

e84bed5

jerader requested a review from ddcc4 February 11, 2025 13:31

ddcc4 approved these changes Feb 11, 2025

View reviewed changes

call module.model inline

9e063ce

jerader merged commit 875346d into edge Feb 11, 2025
16 checks passed

jerader deleted the pd_py-load-module branch February 11, 2025 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol-designer): load module in python file #17483

feat(protocol-designer): load module in python file #17483

jerader commented Feb 10, 2025

jerader Feb 10, 2025

ddcc4 Feb 10, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

ddcc4 left a comment

ddcc4 Feb 10, 2025

ddcc4 Feb 10, 2025

jerader Feb 10, 2025

ddcc4 Feb 10, 2025

jerader Feb 10, 2025

ddcc4 Feb 10, 2025

jerader Feb 10, 2025

jerader Feb 10, 2025

ddcc4 Feb 10, 2025

jerader Feb 10, 2025

ddcc4 Feb 10, 2025

ddcc4 Feb 10, 2025

jerader Feb 10, 2025

ddcc4 Feb 10, 2025

ddcc4 Feb 10, 2025 •

edited

Loading

ddcc4 Feb 10, 2025

ddcc4 Feb 10, 2025

ddcc4 Feb 11, 2025

jerader Feb 11, 2025

ddcc4 commented Feb 11, 2025

feat(protocol-designer): load module in python file #17483

feat(protocol-designer): load module in python file #17483

Conversation

jerader commented Feb 10, 2025

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcc4 Feb 10, 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

ddcc4 commented Feb 11, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

ddcc4 Feb 10, 2025 •

edited

Loading