feat: Add script for provisioning Flex pipettes in emulation #12897

DerekMaggio · 2023-06-12T16:08:00Z

Overview

Add script for provisioning Flex pipettes in emulation. This script is run inside the ot3-firmware-builder container from opentrons-emulation

Script Flow:

Read from environment variables: LEFT_OT3_PIPETTE_DEFINITION and RIGHT_OT3_PIPETTE_DEFINITION
Convert env var content to OT3PipetteEnvVar dataclass object
Generate serial code by using opentrons_hardware.instruments.pipettes.serials.serial_val_from_parts
Create eeprom files for each pipette inside of that pipette's volume folder. Note that the volume folders are created inside the emulation scope.

Note:
It is out of scope for this PR to actually ensure that the pipettes run correctly in emulation. They currently do not, and I cannot figure out why. Going to circle up with the wizards in embedded land to see if they can help me figure it out. A followup PR will address them actually working

Test Plan

Add unit test to validate that exercising the above flow creates the file
Validate against emulation repo
- Using opentrons-emulation local dev, bind mount the latest version of this branch into the emulation system.
- Validate that framework automatically creates correct env vars for script to parse
- Validate that script is automatically called with no required user inpur
- Validate that eeprom files are created on ot3-firmware-builder
- Validate that eeprom files are pushed to the pipette containers through the volume connection

Changelog

Add script
Add test

Review requests

Do you think it is okay to not validate that the content of the eeprom file is correct?
I am not doing this for a few reasons:

Figuring out what the content is actually supposed to be sounds like a pain
I am using a monorepo utility function to do the actual serial number provisioning. This functionality should be tested in the monorepo and not in the context of emulation
The new functionality is parsing the env vars and pushing to the file. Which testing was added for

Risk assessment

Low

Script is not used by this repo
Tests do not affect this repo
No dependencies or existing source code were modified

codecov · 2023-06-12T16:17:48Z

Codecov Report

Merging #12897 (be944f2) into edge (8a30869) will increase coverage by 0.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             edge   #12897      +/-   ##
==========================================
+ Coverage   73.01%   73.02%   +0.01%     
==========================================
  Files        1516     1517       +1     
  Lines       49686    49727      +41     
  Branches     3037     3037              
==========================================
+ Hits        36277    36315      +38     
- Misses      12943    12946       +3     
  Partials      466      466

Flag	Coverage Δ
hardware	`58.72% <94.11%> (+0.19%)`	⬆️
notify-server	`89.13% <ø> (ø)`

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

Impacted Files	Coverage Δ
...ns_hardware/scripts/emulation_pipette_provision.py	`94.11% <94.11%> (ø)`

... and 1 file with indirect coverage changes

fsinapi

Yeah this looks correct to me, couple of comments you can ignore 👍

fsinapi · 2023-06-12T19:32:56Z

hardware/opentrons_hardware/scripts/emulation_pipette_provision.py

+LEFT_PIPETTE_ENV_VAR_NAME = "LEFT_OT3_PIPETTE_DEFINITION"
+RIGHT_PIPETTE_ENV_VAR_NAME = "RIGHT_OT3_PIPETTE_DEFINITION"
+
+LEFT_PIPETTE_EEPROM_DIR_PATH = "/volumes/left-pipette-eeprom"


Part of me thinks that these should be parameters instead of hard coded, but this is such a specialized script that I don't think it matters.

I can easily make these part of the parsed environment variable.
Would that suffice?

💯 sounds good to me

fsinapi · 2023-06-12T19:39:44Z

hardware/tests/test_scripts/test_emulation_pipette_provision.py

+    assert not os.path.exists(left_eeprom_path)
+    assert not os.path.exists(right_eeprom_path)
+    emulation_pipette_provision.main()
+    assert os.path.exists(left_eeprom_path)


It might be worth it to verify that the contents of these files is correct by calling serial_val_from_parts and reading the files. Unfortunate that we can't deserialize the encoded serial values though!

Just to clarify, I am calling that function under the hood to generate the serial number.
Is it redundant to call the same function to compare that value to?

I think what makes it worth testing is that you want to verify that the script, viewed as a black box, is generating the right contents in the right files. I agree that it isn't the most thorough way to test it, but it at least makes sure you're 1) getting all of the info from the environment variable 2) putting the left/right info in the correct respective files.

Well good thing you suggested this.
I, being an idiot, was only ever evaluating the left pipette and storing it to both left and right.
Thanks @fsinapi. You saved me from myself

Laura-Danielle

This looks good to me.

fsinapi

LGTM !

* feat: Add script for provisioning pipettes in emulation * chore: Change to support full eeprom path being passed * Linting * remove unused constants

feat: Add script for provisioning pipettes in emulation

85c131b

DerekMaggio changed the title ~~feat: Add script for provisioning pipettes in emulation~~ feat: Add script for provisioning Flex pipettes in emulation Jun 12, 2023

DerekMaggio requested review from y3rsh, sfoster1 and Laura-Danielle June 12, 2023 17:41

DerekMaggio self-assigned this Jun 12, 2023

DerekMaggio marked this pull request as ready for review June 12, 2023 17:41

DerekMaggio requested a review from a team as a code owner June 12, 2023 17:41

DerekMaggio requested a review from fsinapi June 12, 2023 18:36

DerekMaggio mentioned this pull request Jun 12, 2023

chore: Monorepo script compatability Opentrons/opentrons-emulation#263

Merged

fsinapi approved these changes Jun 12, 2023

View reviewed changes

Laura-Danielle approved these changes Jun 13, 2023

View reviewed changes

Derek Maggio added 2 commits June 13, 2023 17:54

chore: Change to support full eeprom path being passed

73ea94d

Linting

c6cd8d6

DerekMaggio requested a review from fsinapi June 13, 2023 17:57

remove unused constants

be944f2

fsinapi approved these changes Jun 13, 2023

View reviewed changes

DerekMaggio merged commit 14ca121 into edge Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add script for provisioning Flex pipettes in emulation #12897

feat: Add script for provisioning Flex pipettes in emulation #12897

DerekMaggio commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

fsinapi left a comment

fsinapi Jun 12, 2023

DerekMaggio Jun 12, 2023

fsinapi Jun 12, 2023

DerekMaggio Jun 13, 2023

fsinapi Jun 12, 2023

DerekMaggio Jun 12, 2023

fsinapi Jun 12, 2023

DerekMaggio Jun 13, 2023

DerekMaggio Jun 13, 2023

Laura-Danielle left a comment

fsinapi left a comment

feat: Add script for provisioning Flex pipettes in emulation #12897

feat: Add script for provisioning Flex pipettes in emulation #12897

Conversation

DerekMaggio commented Jun 12, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

fsinapi 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

Laura-Danielle left a comment

Choose a reason for hiding this comment

fsinapi left a comment

Choose a reason for hiding this comment

DerekMaggio commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading