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): Support the new pipette configurations and 96 channel in the hardware controller #11830

Merged

Conversation

Laura-Danielle
Copy link
Contributor

@Laura-Danielle Laura-Danielle commented Dec 6, 2022

Overview

This PR is an initial pass at supporting the new pipette configurations from shared data in the hardware controller. As a result of this, we also get to control the 96 channel in the hardware controller. Another benefit is that the ot3 api can now switch between pipetting functions depending on the working volume of the pipette.

A lot of the work in this PR will require refactoring once we have the ot2 pipettes ported over to the new pipette configuration. I hope to combine a lot of repeat code at that time. The shape of the configurations is very different, so I did not bother trying to combine it at this time.

A separate PR will be put up to support full tip rack pick up with the 96 channel.

A few things that need more thought in the follow-ups:

  • Are we satisfied with how we handle major/minor version?
  • How can we make the caching more effective for the pipette information in shared data?
  • How should we actually store multiple tip types with their configurations?
  • What should we do about the pipette "name" and "model" given that they're very confusing and we're moving away from loading in pipettes by string name.
  • Should we keep track of the right mount's "position" while the 96 channel is moving?

Changelog

  • Block pipettes not supported on the OT3 from being loaded in an OT3 hardware controller
  • Separate the new configurations and use them in the Pipette object / hardware controller
  • Changed how pipettes are reset. Rather than reloading an object (which is a roundabout way of resetting state/offsets), we should actually reset state/offsets/configurations
  • Modified current/speeds for high throughput gantry load
  • Modified the gantry load function to not rely on the pipette dict and modified the functionality to only return HIGH_THROUGHPUT when a 96 channel is attached to the left (the only supported configuration)
  • Marked all tests that use non-ot3 pipettes as ot2_only for now. A lot of them were old protocol api tests which I'm assuming we plan to delete at some point as well.

Planned fast follows

  • Support the correct "versions" for the pipettes
  • Add the OT2 pipettes to the new pipette configuration format (will also need to change locations this is ported in for js as well)
  • Support updating pipette configurations for the OT2 pipettes
  • Re-combine pipette_handler and separate out pipette objects only where functionality diverges
  • Separate out the concept of "static" and "stateful" pipette information that consumers of the HC would be interested in. This will effectively remove the pipette state "dict" that we keep track of on top of the attributes stored in the Pipette object class.

Risk assessment

High. This changes where configurations are loaded in for pipettes. It should be thoroughly tested for all pipette types.

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #11830 (9c19a98) into RLIQ-131-support-96-channel (e59f558) will increase coverage by 0.04%.
The diff coverage is 80.00%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           RLIQ-131-support-96-channel   #11830      +/-   ##
===============================================================
+ Coverage                        74.14%   74.19%   +0.04%     
===============================================================
  Files                             2106     2108       +2     
  Lines                            58491    58636     +145     
  Branches                          6218     6218              
===============================================================
+ Hits                             43371    43504     +133     
- Misses                           13654    13666      +12     
  Partials                          1466     1466              
Flag Coverage Δ
app 72.60% <ø> (ø)
hardware-testing ∅ <ø> (∅)
notify-server 88.26% <ø> (ø)
protocol-designer 45.86% <ø> (ø)
shared-data 85.53% <78.00%> (+0.72%) ⬆️
step-generation 88.46% <ø> (ø)

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

Impacted Files Coverage Δ
api/src/opentrons/config/defaults_ot3.py 87.27% <0.00%> (ø)
...entrons/hardware_control/backends/ot3controller.py 64.63% <58.33%> (ø)
api/src/opentrons/hardware_control/ot3api.py 80.66% <73.33%> (ø)
.../python/opentrons_shared_data/pipette/load_data.py 93.10% <75.00%> (ø)
...pentrons_shared_data/pipette/pipette_definition.py 91.37% <78.57%> (ø)
api/src/opentrons/hardware_control/__init__.py 100.00% <100.00%> (ø)
...pentrons/hardware_control/backends/ot3simulator.py 88.57% <100.00%> (ø)
api/src/opentrons/hardware_control/dev_types.py 100.00% <100.00%> (ø)
...ardware_control/protocols/instrument_configurer.py 63.15% <100.00%> (ø)
api/src/opentrons/protocols/api_support/util.py 90.80% <100.00%> (ø)
... and 10 more

@Laura-Danielle Laura-Danielle force-pushed the RLIQ-60-support-96-channel-hc branch from 0e86423 to 63a8e4a Compare December 6, 2022 20:44
@Laura-Danielle Laura-Danielle marked this pull request as ready for review December 7, 2022 16:19
@Laura-Danielle Laura-Danielle requested a review from a team as a code owner December 7, 2022 16:19
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Couple inline changes; some are critical, like using floats for versions - we really shouldn't do that.

There's also a whole lot of tests that effectively get mark-skipped for the ot3. Is there any way at all we can avoid that? IMO we really shouldn't check in PRs that disable swathes of tests like this.

Are the ot3 variants of pipette and pipette_handler straight copies of the ot2 variants? if not, can you point at where they're different?

api/src/opentrons/config/ot3_pipette_config.py Outdated Show resolved Hide resolved
api/src/opentrons/config/ot3_pipette_config.py Outdated Show resolved Hide resolved
api/src/opentrons/hardware_control/dev_types.py Outdated Show resolved Hide resolved
)
new_p.act_as(p.acting_as)
self._attached_instruments[m] = new_p
if isinstance(m, OT3Mount):
Copy link
Member

Choose a reason for hiding this comment

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

the linter will also be satisfied if you make reset_pipette_offset generic across mount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the code is separated -- including calibration stuff right now. Would rather address this when I'm recombining all the instruments code.

api/src/opentrons/hardware_control/ot3api.py Outdated Show resolved Hide resolved
@@ -670,26 +671,50 @@ async def test_blowout_flow_rate(sim_and_instr):


async def test_reset_instruments(monkeypatch, sim_and_instr):
sim_builder, dummy_instruments = sim_and_instr
instruments = {
Copy link
Member

Choose a reason for hiding this comment

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

we should have a version of this for ot2 and ot3, since those are now separate pieces of code

@Laura-Danielle
Copy link
Contributor Author

Are the ot3 variants of pipette and pipette_handler straight copies of the ot2 variants? if not, can you point at where they're different?

The main differences include:

  1. some new static properties to make is easier to look up things like plunger positions and pick up configurations
  2. the shape of the pipette configurations passed in

Other than that both of those files are straight copies that I hope to be able to merge back together once the OT2 pipette configurations are ported over.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

This looks good as a first step to me, yeah. I think your refactor plans will help the other stuff I like less.

return PipetteVersionType(major, minor)


def version_from_int(version: int) -> PipetteVersionType:
Copy link
Member

Choose a reason for hiding this comment

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

When do we end up using this?

@Laura-Danielle Laura-Danielle merged commit 3a3035b into RLIQ-131-support-96-channel Dec 8, 2022
@Laura-Danielle Laura-Danielle deleted the RLIQ-60-support-96-channel-hc branch December 8, 2022 19:37
Laura-Danielle added a commit that referenced this pull request Dec 16, 2022
…he hardware controller (#11830)

* feat(api): change pipette functions based on the tip working volume 
* feat(api): do not use "pipette name" and "pipette model" to look up pipettes via configurations
Laura-Danielle added a commit that referenced this pull request Dec 27, 2022
…he hardware controller (#11830)

* feat(api): change pipette functions based on the tip working volume 
* feat(api): do not use "pipette name" and "pipette model" to look up pipettes via configurations
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.

2 participants