Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(protocol-designer): add load liquid commands #9923

Merged
merged 6 commits into from
Apr 13, 2022

Conversation

shlokamin
Copy link
Member

@shlokamin shlokamin commented Apr 8, 2022

Overview

This PR adds load liquid command to PD migrations, as well as the create file function which actually generates the final JSON file that users export.

closes #9702

Changelog

  • Add load liquid commands

Review requests

Add a few different liquids to different labware in PD. Export the protocol, you should see load liquid commands generated in the commands list that conform to the LoadLiquidCreateCommand interface.

Risk assessment

Low

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #9923 (fef525c) into edge (3c64516) will decrease coverage by 0.00%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #9923      +/-   ##
==========================================
- Coverage   74.70%   74.69%   -0.01%     
==========================================
  Files        2048     2054       +6     
  Lines       54034    54251     +217     
  Branches     5359     5421      +62     
==========================================
+ Hits        40366    40525     +159     
- Misses      12600    12644      +44     
- Partials     1068     1082      +14     
Flag Coverage Δ
app 70.42% <ø> (+0.06%) ⬆️
protocol-designer 44.77% <93.33%> (+0.47%) ⬆️
shared-data 85.02% <ø> (ø)
step-generation 85.70% <ø> (+0.52%) ⬆️

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

Impacted Files Coverage Δ
...neration/src/getNextRobotStateAndWarnings/index.ts 56.92% <ø> (ø)
...load-file/migration/utils/getLoadLiquidCommands.ts 91.30% <91.30%> (ø)
...ol-designer/src/file-data/selectors/fileCreator.ts 86.95% <100.00%> (+0.59%) ⬆️
protocol-designer/src/load-file/migration/6_0_0.ts 97.36% <100.00%> (+0.07%) ⬆️
robot-server/robot_server/app_setup.py 95.65% <100.00%> (ø)
...obot-server/robot_server/protocols/dependencies.py 100.00% <100.00%> (ø)
robot-server/robot_server/service/logging.py 100.00% <100.00%> (ø)
robot-server/robot_server/settings.py 100.00% <100.00%> (ø)
app/src/organisms/Devices/PipettesAndModules.tsx 85.71% <0.00%> (-14.29%) ⬇️
app/src/App/NextGenApp.tsx 71.42% <0.00%> (-7.15%) ⬇️
... and 43 more

@shlokamin shlokamin marked this pull request as ready for review April 11, 2022 14:44
@shlokamin shlokamin requested a review from a team as a code owner April 11, 2022 14:47
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

"4d23ffe0-b759-11ec-81e8-7fa12dc3e861": "opentrons/opentrons_96_filtertiprack_20ul/1"
},
"dismissedWarnings": { "form": {}, "timeline": {} },
"ingredients": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is quite the ingredients list 😆

@@ -18,7 +18,7 @@ import type {

export type CommandStatus = 'queued' | 'running' | 'succeeded' | 'failed'
export interface CommonCommandRunTimeInfo {
key: string
key?: string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this become optional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question! two reasons

  1. in the v6 spec key is optional
  2. in order for RunTimeCommands to extend CreateCommands, all properties must match if there are any that overlap. key was marked as optional in CreateCommand (which is correct), but required in RunTimeCommand, so TypeScript errors out.

designerApplication?.data?.ingredLocations
)) {
Object.values(liquidsByWellName).forEach(volumeByLiquidId => {
if (liquidId in volumeByLiquidId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a lot of logic, I've seen in other files with a lot of logic have comments sprinkled in to help people understand what is going on easier 😄 (ex: see useModuleMatchResults)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol yes that is a good call, ill add some comments

@shlokamin shlokamin merged commit 5b003f5 into edge Apr 13, 2022
@shlokamin shlokamin deleted the pd_add-load-liquid-commands branch April 13, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema v6 migration: add liquids key and load liquid commands
2 participants