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): Replace obsolete machine args with robot_type #13737

Merged
merged 12 commits into from
Oct 10, 2023

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Oct 6, 2023

Overview

Closes RSS-266 and RSS-267.

Test Plan

I will:

  • Test simulating an OT-2 protocol locally.
  • Test simulating an OT-2 protocol on an OT-2.
  • Test simulating a Flex protocol on an OT-2. This should fail with an error message describing the robot type mismatch.
  • Test simulating an OT-2 protocol on a Flex. This should fail with an error message describing the robot type mismatch.

Changelog

Main items, RSS-266 and RSS-267:

  • Remove the machine argument of the opentrons.simulate.simulate() function and opentrons_simulate CLI. Instead, we automatically infer it from the contents of the protocol.
  • Remove the machine argument of opentrons.simulate.get_protocol_api(). Replace it with robot_type, and make its values match the robotType key in the requirements dict of Python protocols.

Other stuff:

  • Move execute.py's find_jupyter_labware() helper to somewhere it can be shared between simulate.py and execute.py.
  • Fix bugs in the test cases that could cause false passes.
  • Add various notes and comments.

Review requests

None in particular.

Risk assessment

The removal of the machine arguments is technically a breaking change for any OT-2 user who's gone out of their way to discover and use them. I'm betting nobody has done this, and even if they have, the fix is trivial. But if anybody disagrees, we can add some kind of backwards compatibility shim.

@SyntaxColoring SyntaxColoring requested a review from a team October 6, 2023 19:53
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner October 6, 2023 19:53
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #13737 (a9cb3dc) into chore_release-7.0.1 (47d486c) will decrease coverage by 0.05%.
Report is 1 commits behind head on chore_release-7.0.1.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.1   #13737      +/-   ##
=======================================================
- Coverage                71.30%   71.25%   -0.05%     
=======================================================
  Files                     1588     2429     +841     
  Lines                    52809    68421   +15612     
  Branches                  3448     8043    +4595     
=======================================================
+ Hits                     37653    48754   +11101     
- Misses                   14621    17769    +3148     
- Partials                   535     1898    +1363     
Flag Coverage Δ
app 68.78% <ø> (+24.52%) ⬆️
components 60.73% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
labware-library 49.17% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 46.09% <ø> (ø)
step-generation 86.82% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/execute.py 54.09% <ø> (ø)
api/src/opentrons/hardware_control/types.py 96.55% <ø> (-0.03%) ⬇️
api/src/opentrons/simulate.py 63.57% <ø> (-0.35%) ⬇️
api/src/opentrons/util/entrypoint_util.py 90.69% <ø> (ø)

... and 842 files with indirect coverage changes

@SyntaxColoring SyntaxColoring added the api Affects the `api` project label Oct 9, 2023
Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

Looks good, just have one comment but other than that good to merge 🤖

Comment on lines +319 to +322
raise RuntimeError(
f'This robot is of type "{current_robot_type}",'
f' so it can\'t simulate protocols for robot type "{robot_type}"'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than expose that in this error message, until we fix this it might be better if we just say something along the lines of "robotType defined in protocol cannot be simulated on this machine."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeahhhhhhh, maybe.

I'm going to leave it as-is for now, to try to stay closer to robot-server, which provides these details and does it properly. Ideally, the logic to generate the human-readable error would be shared between them.

@SyntaxColoring SyntaxColoring merged commit 27e594a into chore_release-7.0.1 Oct 10, 2023
@SyntaxColoring SyntaxColoring deleted the remove_obsolete_machine_args branch October 10, 2023 19:20
SyntaxColoring added a commit that referenced this pull request Oct 19, 2023
* Fix bugs in Jupyter override test cases.

* Move find_jupyter_labware() to entrypoint_utils.

* Remove obsolete `machine` args.
  
  Replace `machine` arg of `simulate.get_protocol_api()` with `robot_type`.
  
  Remove `machine` arg of `simulate.simulate()`.
  
  Remove MachineType type.

* Various minor refactors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants