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(api): remove version suffixes from pipette models in JSON protocols #1825

Closed

Conversation

IanLondon
Copy link
Contributor

overview

Let's make JSON protocols agnostic to pipette version!

Users are not aware of the differences between a "v1" and a "v1.3" pipette versions. They aren't written on the pipettes.

Protocol Designer & JSON protocols shouldn't need to tie pipettes to a particular pipette version due to differing capabilities to accomplish a protocol. On the protocol level, a p10_single_v1 is identical to a p10_single_v1.3 and so on, for all versions. If p10_single_v42 has some differing capabilities, it shouldn't be a "p10_single".

If JSON protocols have a pipette version hard-coded into the protocol, we would have to deal with the case where the user's robot has a p10_single_v1.3 but the protocol is written for a p10_single_v1. It seems like the best thing to do is to ignore the hard-coded pipette version -- so it seems like it shouldn't be there in the first place.

changelog

  • JSON protocols now follow Python protocol convention of dealing with pipettes: version-less model, with version suffix added during simulation/runtime and defaulting to _v1

review requests

If p10_single_v42 has some differing capabilities, it shouldn't be a "p10_single".

Is that true?

  • This would be a breaking change, but since PD + JSON protocols are still pre-beta, is it OK if we don't treat it like one? The schema had a "TBD" note in it for the pipette model field.
  • Needs parallel update in Protocol Designer, or protocols won't work (and will no longer conform to the JSON protocol schema)! I didn't attempt it yet in this PR b/c we have a huge PD PR that touches pipettes still open at the moment, so after that is closed I will make those updates here. Until then, DO NOT MERGE.

@IanLondon IanLondon added api Affects the `api` project protocol designer Affects the `protocol-designer` project RFC Software proposal ("request for comment") json protocol schema DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available labels Jul 6, 2018
@codecov
Copy link

codecov bot commented Jul 6, 2018

Codecov Report

Merging #1825 into edge will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #1825   +/-   ##
=======================================
  Coverage   34.22%   34.22%           
=======================================
  Files         370      370           
  Lines        6069     6069           
=======================================
  Hits         2077     2077           
  Misses       3992     3992

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c803f1...79de26d. Read the comment docs.

@btmorr
Copy link
Contributor

btmorr commented Jul 9, 2018

I think this is a good solution, and I agree with the logic regarding breaking changes (that this is not a breaking change since it's solely for a non-released product). We should definitely communicate it to folks who have had an interest in the JSON protocol schema, but we definitely don't want protocols to require an exact model of pipette.

@btmorr
Copy link
Contributor

btmorr commented Jul 9, 2018

I also agree that a pipette version with different capabilities should have a different name, even if the volume and number of tips are the same. I'm struggling to imagine the scenario, but let's say it's at a 45 degree angle, then it should be something like P10_Single_45_deg_v1.0 rather than P10_Single_v4.2

@IanLondon IanLondon removed the RFC Software proposal ("request for comment") label Jul 16, 2018
@mcous
Copy link
Contributor

mcous commented Sep 4, 2018

It's been a while since we talked about this one, but if my memory serves me correctly, we decided:

  • model should remain the exact model number of a given pipette
    • model ties to specific configuration, like motor current settings and dimensional offsets
    • model is a manufacturing concern, and should be treated like it may be specified arbitrarily
  • a name field should be added to the pipette
    • Two pipettes of the same name have the same external capabilities, but may differ in under-the-hood configuration (e.g. motor current settings)
    • name can be used by the app to check proper pipettes are attached
    • The values for name will be "versionless" models, but this similarity should be treated as coincidental (see above about model being a factory concern)

This should line up with what we've implemented for modules

@IanLondon
Copy link
Contributor Author

@btmorr & I came up with this: 2 file for pipette configs. One is "public" and one is "under the hood", IDK what to name them, but here's the idea:

pipette-config-public.json - PD pulls purely from this. This is the "versionless" name - properties that do not vary across models

{
  "p10_single": {
    "displayName": "P10 Single-Channel",
    "nominalMaxVolumeUl": 10,

    "aspirateFlowRate": 5,
    "dispenseFlowRate": 10,
    "channels": 1,
  },
  ...other pipette names
}

pipette-config-internals.json - PD never looks at this. API merges the 2 via the name key.

{
  "p10_single_v1.3": {
    "name": "p10_single", // acts as FK to keys in "public" json
    "plungerPositions": {
      "top": 19.5,
      "bottom": 0.5,
      "blowOut": -2.5,
      "dropTip": -6
    },
    "pickUpCurrent": 0.1,
    "modelOffset": [0.0, 0.0, -13],
    "plungerCurrent": 0.3,
    "dropTipCurrent": 0.5,
    "maxVolume": 10,
    "tipLength": 33
  },
  ... other models here like p10_single_v1, etc
}

@IanLondon
Copy link
Contributor Author

More comprehensive PR coming soon

@IanLondon IanLondon closed this Nov 1, 2018
@IanLondon IanLondon deleted the api_versionless-pipette-models-in-json-protocols branch November 2, 2018 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available json protocol schema protocol designer Affects the `protocol-designer` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants