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

chore(hardware): add opentrons hardware package to ot 2 build #13770

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Oct 12, 2023

Overview

This PR, in conjunction with Opentrons/buildroot#209 adds the opentrons_hardware package to OT-2 builds. This is the beginning of plans to port most of the hardware-dependent code in the opentrons package over to opentrons_hardware. As of now, the package cannot actually be imported on the OT-2.

Test Plan

Uploaded the build that was created from both this and the corresponding buildroot PR to an OT-2. Everything started up as normal, the app could still talk to the robot, and HTTP endpoints responded as normal. Hitting a Flex-specific endpoint still gets filtered out, with the error detail "No module named 'can'". SSH'd in and checked that the opentrons_hardware package was on the filesystem as expected.

Changelog

  • Mark python-can as an optional dependency of the hardware package in setup.py
  • Add buildroot configuration to install opentrons_hardware and generate a version like other packages
  • All of the setup-ot2 targets remove python-can instead of the entire opentrons_hardware package

Review requests

Risk assessment

* Make "python-can" an optional dependency for "opentrons_hardware"
* Add "CAN" as an extra dependency for opentrons-hardware from the base opentrons pipfile
* All "setup-ot2" targets just move python-can instead of the entire hardware package
@fsinapi fsinapi requested review from a team as code owners October 12, 2023 14:37
@fsinapi fsinapi requested a review from a team October 12, 2023 14:38
@fsinapi fsinapi self-assigned this Oct 12, 2023
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #13770 (a81ef47) into edge (e30c1b5) will not change coverage.
Report is 6 commits behind head on edge.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #13770   +/-   ##
=======================================
  Coverage   70.96%   70.96%           
=======================================
  Files        2454     2454           
  Lines       69033    69033           
  Branches     8258     8258           
=======================================
  Hits        48986    48986           
  Misses      18095    18095           
  Partials     1952     1952           
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
hardware 56.58% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

Copy link
Contributor

@vegano1 vegano1 left a comment

Choose a reason for hiding this comment

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

This is awesome,
left 2 simple copypasta errors
but otherwise this looks good

@@ -0,0 +1,17 @@
config BR2_PACKAGE_PYTHON_OPENTRONS_HARDWARE
bool "python-opentrons-system-server"
Copy link
Contributor

Choose a reason for hiding this comment

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

hardware server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed

select BR2_PACKAGE_PYTHON_CLICK # runtime

help
Opentrons system HTTP server. Provides access to an OT-2 robot.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • hardware server
  • OT-2 and Flex or maybe Opentrons robot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤖

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.

I'd love to see if we can bubble up the extras to something like "install hardware support for flex" on the opentrons package.

Also we should check that the OE build still works since that consumes the pipfile I believe.

api/setup.py Outdated
@@ -60,6 +60,7 @@ def get_version():
PACKAGES = find_packages(where="src")
Copy link
Member

Choose a reason for hiding this comment

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

can we see if we can make the extras thing transitive? like, add an extra to api that is 'flex-hardware' that requires opentrons_hardware["CAN"], an extra that is ot2-hardware that requires opentrons_hardware (maybe eventually opentrons_hardware["SERIAL"] or whatever). Then the pipfile for api installs opentrons["flex-hardware"].

The reason for this is that ideally we don't require hardware support for default in the api server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this worked pretty nicely. Tested running all of the following and they seem to filter the packages correctly:

  • pipenv run pip install . --> no hardware package
  • pipenv run pip install ".[ot2-hardware]" --> opentrons_hardware without python-can
  • pipenv run pip install ".[flex-hardware]" --> opentrons_hardware with python_can

- Updated the can-related extra requirement in opentrons_hardware to "flex"
- Added extra requirements to API for either "ot2-hardware" or "flex-hardware"
@fsinapi fsinapi added the ot3-build Do an OT-3 system build for this branch. label Oct 12, 2023
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 is awesome!

@fsinapi
Copy link
Contributor Author

fsinapi commented Oct 12, 2023

Tested installing the Openembedded build on Flexy, everything is still in the right place and the server seems to be chugging along fine

@fsinapi fsinapi merged commit e3e82e8 into edge Oct 19, 2023
30 checks passed
@fsinapi fsinapi deleted the RET-1380-Add-opentrons_hardware-package-to-OT-2-build branch October 19, 2023 18:24
y3rsh added a commit that referenced this pull request Oct 25, 2023
* edge: (77 commits)
  fix(app): update move gantry text in change pipette flow (#13712)
  fix(app): fix CI after release merge back (#13816)
  chore(hardware): add opentrons hardware package to ot 2 build (#13770)
  feat(protocol-designer): correct step count in create file wizard (#13807)
  feat(protocol-designer): error handling in create file wizard (#13804)
  fix(app): invalidate OT2 calibration queries when calibration flows complete (#13809)
  always jog (#13806)
  chore(app): point updates at dns not s3 (#13798)
  docs(api): updated Flex default flow rates (#13796)
  fix(api): Flag pipette as not ready to aspirate after pushing out air (#13728)
  fix(api, hardware): allow OT3Controller to disable tip motors (#13805)
  more blank trials; longer scale stabilize wait; submerge 2.5mm (#13788)
  fix(app): unload adapters after checking position of labware on adapter on heater shaker module (#13803)
  feat(app): wire up location conflict modal for ODD (#13797)
  feat(app): add Deck configuration page component (#13784)
  fix(robot-server): add default to robot serial (#13802)
  feat(protocol-designer, components): prep work for deck config in PD (#13775)
  feat(app, protocol-designer): plug in waste chute asset (#13800)
  fix(app): do not check mag block in calibration status (#13799)
  feat(protocol-designer): edit additional items staging area support (#13752)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ot3-build Do an OT-3 system build for this branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants