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

ci(api,shared-data): run test matrix on Python 3.7 thru 3.10 #9497

Merged
merged 6 commits into from
Feb 28, 2022

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Feb 17, 2022

Overview

This PR adds a CI matrix for Python 3.7 through 3.10 for our pip-installable packages: opentrons and opentrons_shared_data.

Takes over for the CI portion of #9368 for review and git history purposes.

Changelog

  • Bump pytest to v7 for Python 3.10 support
  • Ensure Pipfile.lock behaves on Python 3.7 all the way to Python 3.10
  • Add unit test runs to the CI matrix for Python >= 3.7 for these packages
  • Mark setup.py with support for Python >= 3.7

Review requests

  • Changes look good
  • This PR does not affect your local 3.7 dev environment
  • This PR allows you to set up a local 3.10 dev environment

With this PR and a copy of Python 3.10 installed somewhere on your system (e.g. via pyenv), to set up a venv in a new Python version:

# tear down the old environment
make -C app teardown

# set up a new dev environment with the given Python version
make -C app setup pipenv_opts="--dev --python 3.10"

Risk assessment

No risk to production environment. However, for our dev environment, runs the following risks:

  • CI runs for api and shared-data will likely take more time
  • Most developers will continue to run local 3.7 environments, so the risk of introducing changes that break in >= 3.8 is high
    • However, CI should catch these failures
    • So, this turns into a risk of time wasted rather than a risk of breaking stuff

@mcous mcous added DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). Robot Tech Debt For review in robot guild meetings labels Feb 17, 2022
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #9497 (28591d6) into edge (add3ff1) will increase coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #9497      +/-   ##
==========================================
+ Coverage   75.61%   75.84%   +0.22%     
==========================================
  Files        1916     1916              
  Lines       50916    51388     +472     
  Branches     4895     4895              
==========================================
+ Hits        38502    38974     +472     
  Misses      11492    11492              
  Partials      922      922              
Flag Coverage Δ
api 85.50% <ø> (+0.35%) ⬆️
app 73.12% <ø> (ø)
components 52.41% <ø> (ø)
labware-library 49.90% <ø> (ø)
notify-server 89.17% <ø> (ø)
protocol-designer 44.14% <ø> (ø)
react-api-client 86.51% <ø> (ø)
robot-server 92.96% <ø> (ø)
shared-data 82.02% <ø> (+0.05%) ⬆️
step-generation 90.33% <ø> (ø)
update-server 70.44% <ø> (ø)

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

Impacted Files Coverage Δ
...c/opentrons/hardware_control/emulation/tempdeck.py 94.73% <0.00%> (-2.57%) ⬇️
...rc/opentrons/hardware_control/emulation/magdeck.py 90.00% <0.00%> (-2.31%) ⬇️
api/src/opentrons/drivers/rpi_drivers/gpio.py 5.17% <0.00%> (-0.03%) ⬇️
api/src/opentrons/config/types.py 100.00% <0.00%> (ø)
api/src/opentrons/drivers/types.py 100.00% <0.00%> (ø)
api/src/opentrons/algorithms/graph.py 100.00% <0.00%> (ø)
api/src/opentrons/motion_planning/types.py 100.00% <0.00%> (ø)
api/src/opentrons/protocol_api/load_info.py 100.00% <0.00%> (ø)
api/src/opentrons/calibration_storage/types.py 100.00% <0.00%> (ø)
api/src/opentrons/protocol_engine/state/state.py 100.00% <0.00%> (ø)
... and 75 more

@mcous mcous removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Feb 17, 2022
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 think this is going to make these tests take a) forever by wall clock and b) use way more github actions runners, which is something we're already fairly frequently rate limited on.

Can we investigate using something like tox to do it inside a single runner?

@mcous mcous assigned mcous and y3rsh Feb 18, 2022
@y3rsh
Copy link
Member

y3rsh commented Feb 18, 2022

@sfoster1 @mcous

I think this is going to make these tests take a) forever by wall clock and b) use way more github actions runners, which is something we're already fairly frequently rate limited on.

Can we investigate using something like tox to do it inside a single runner?

I am not sure if tox solves our problem. We would still need 3 OS runners customized to have the 4 versions of python installed making 3 long running runners. Other folks seem to use the matrix even with tox.

Regardless of this, I would like to know how we are hitting rate limits. I see usage of runners increasing so I would like to spearhead solutions. Besides @bndo who else might be good pull into conversation on this?

@mcous
Copy link
Contributor Author

mcous commented Feb 18, 2022

For reference, here's a full matrix run with everything passing from an earlier iteration of this branch: https://github.com/Opentrons/opentrons/actions/runs/1792647885

Each job in the matrix took 5 to 10 minutes, depending on OS

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.

Looks good to me, then! Love to see the changes

@mcous mcous marked this pull request as ready for review February 24, 2022 22:10
@mcous mcous requested review from a team as code owners February 24, 2022 22:10
# Conflicts:
#	.github/workflows/api-test-lint-deploy.yaml
#	.github/workflows/shared-data-test-lint-deploy.yaml
@mcous mcous merged commit af3dfd0 into edge Feb 28, 2022
@mcous mcous deleted the ci_python-3.10 branch February 28, 2022 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Robot Tech Debt For review in robot guild meetings robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants