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): Allow omitting the mount when loading a 96-channel #14187

Merged
merged 6 commits into from
Dec 14, 2023

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 13, 2023

Overview

The 96-Channel Pipette physically occupies both mounts. But a quirk (not ticketed) of our current Python Protocol API is that you need to specify mount="left" or mount="right". This PR fixes that quirk.

Changelog

Before this PR:

  • The Python Protocol API's load_instrument() method takes a mount argument that's required to be "left" or "right".
  • The documentation says to use "left" if you're loading a 96-Channel.
  • The implementation ignores whichever value you pass in; "left" and "right" do the same thing in practice.

With this PR:

  • The mount argument is allowed to be omitted or None if you're loading a 96-channel. For all other pipettes, it's still required to be "left" or "right".
  • The documentation says to omit mount if you're loading a 96-channel.
  • The implementation continues to ignore whichever value you pass in. This is for compatibility with existing protocols.

This is roughly analogous to how we treat the Thermocycler Module, which can only be loaded into one location.

Test Plan

  • Try out some different load syntaxes and make sure they raise errors, or don't raise them, according to your expectations.

Review requests

Is this a good idea to do now, or were we deliberately making people do mount="<something>"?

Risk assessment

Low.

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Merging #14187 (4f0845e) into chore_release-7.1.0 (165a607) will decrease coverage by 0.01%.
Report is 12 commits behind head on chore_release-7.1.0.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.1.0   #14187      +/-   ##
=======================================================
- Coverage                70.45%   70.45%   -0.01%     
=======================================================
  Files                     2512     2512              
  Lines                    71237    71232       -5     
  Branches                  8982     8982              
=======================================================
- Hits                     50189    50184       -5     
  Misses                   18849    18849              
  Partials                  2199     2199              
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/protocol_api/protocol_context.py 92.07% <ø> (-0.24%) ⬇️

@SyntaxColoring SyntaxColoring added the papi-v2 Python API V2 label Dec 13, 2023
Comment on lines -834 to -835
:param str instrument_name: The name of the instrument model, or a
prefix. For instance, 'p10_single' may be
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure when this prefix thing was true, but it is not true now and it's weird, so I'm removing it from the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

🚮

@SyntaxColoring SyntaxColoring marked this pull request as ready for review December 13, 2023 17:14
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner December 13, 2023 17:14
@SyntaxColoring SyntaxColoring requested review from ecormany and a team and removed request for a team December 13, 2023 17:14
Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

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

From docs side, go ahead with this. The docstrings will carry along no problem. I'm doing a huge refactor of the Pipettes page (#14189), to the point it's not compatible with rebasing, so I'll incorporate those changes by copy-paste into that branch.

api/docs/v2/new_pipette.rst Outdated Show resolved Hide resolved
Comment on lines -834 to -835
:param str instrument_name: The name of the instrument model, or a
prefix. For instance, 'p10_single' may be
Copy link
Contributor

Choose a reason for hiding this comment

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

🚮

api/src/opentrons/protocol_api/protocol_context.py Outdated Show resolved Hide resolved
@SyntaxColoring SyntaxColoring merged commit ae8f637 into chore_release-7.1.0 Dec 14, 2023
25 checks passed
@SyntaxColoring SyntaxColoring deleted the fix_96_channel_load_mounts branch December 14, 2023 17:07
ncdiehl11 pushed a commit that referenced this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
papi-v2 Python API V2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants