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: bump python deps for 3.10 and robot system updates #13865

Merged
merged 60 commits into from
Jan 4, 2024

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Oct 27, 2023

This PR contains updates required to handle Opentrons/oe-core#110 and Opentrons/buildroot#213 which update the flex operating system and ot-2 operating system respectively to more recent versions of, well everything. Perhaps most significantly to us, both PRs update the python that runs on the machine to 3.10, which requires a lot of changes to a lot of our code.

This PR makes those changes by updating the python versions used in our test harnesses to 3.10; setting those dependencies explicitly captured by the system builders to their new versions from the new versions of the system builders; and bumping other dependencies the minimum required to make it all work. Further work may increase dependency versions further.

The big ones are updating pydantic to 1.9, which is something we've been putting off for a while and probably unlocks further updates. This update is because of the following, which is typical of the process:

  • system builders updated numpy, which is locked in because it must be cross-compiled, to 1.17
  • numpy 1.17 includes type annotations via .pyi
  • mypy must be updated to 0.981 because previous versions interpreted .pyi files as-if the python version was 3.7, but the .pyis had some >3.7-only features
  • pydantic must be updated to 1.9 because 1.8 doesn't work with that updated mypy because of a change to internal mypy hooks
  • a lot of server code and tests have to be slightly altered because pydantic 1.9 doesn't elide None keys, which is (IMO) good anyway

This should hopefully free us to update mypy to the latest again, and hopefully the changes to explicit handling of none keys will let us update pydantic further.

Anyway, there's a fair amount of "just run the robot" style testing to do here, and we have to decide what to do about loading pickles; handling user-installed dependencies; and further support of 3.7. But all the tests should now pass.

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (5733f66) 68.05% compared to head (5a41e94) 58.78%.
Report is 10 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13865      +/-   ##
==========================================
- Coverage   68.05%   58.78%   -9.28%     
==========================================
  Files        2509     1979     -530     
  Lines       71342    47067   -24275     
  Branches     9062     8737     -325     
==========================================
- Hits        48555    27667   -20888     
+ Misses      20684    17370    -3314     
+ Partials     2103     2030      -73     
Flag Coverage Δ
api ?
app 65.35% <ø> (+0.03%) ⬆️
components 50.34% <ø> (-0.03%) ⬇️
g-code-testing 96.48% <ø> (+0.04%) ⬆️
hardware 57.19% <47.05%> (+0.34%) ⬆️
hardware-testing ∅ <ø> (∅)
notify-server 89.13% <ø> (ø)
ot3-gravimetric-test ?
protocol-designer 34.93% <ø> (ø)
robot-server ?
shared-data 75.05% <100.00%> (-0.03%) ⬇️
step-generation ∅ <ø> (∅)
system-server 96.04% <100.00%> (+0.01%) ⬆️
update-server 63.58% <39.39%> (+0.07%) ⬆️
usb-bridge 76.94% <ø> (+0.06%) ⬆️

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

Files Coverage Δ
...pentrons_hardware/drivers/can_bus/can_messenger.py 82.94% <ø> (-0.78%) ⬇️
...dware/opentrons_hardware/drivers/can_bus/driver.py 97.36% <ø> (+16.11%) ⬆️
...are/firmware_bindings/utils/binary_serializable.py 96.96% <100.00%> (+0.37%) ⬆️
...are/hardware_control/motion_planning/move_utils.py 92.57% <100.00%> (ø)
...hardware/hardware_control/motion_planning/types.py 92.00% <100.00%> (+0.54%) ⬆️
hardware/opentrons_hardware/scripts/can_comm.py 68.59% <100.00%> (ø)
hardware/opentrons_hardware/sensors/sensor_abc.py 78.12% <100.00%> (ø)
...ared-data/python/opentrons_shared_data/_version.py 61.53% <100.00%> (-7.70%) ⬇️
system-server/system_server/_version.py 61.53% <100.00%> (ø)
...server/otupdate/common/name_management/__init__.py 100.00% <100.00%> (ø)
... and 8 more

... and 660 files with indirect coverage changes

@sfoster1 sfoster1 changed the title chore(hardware): bump python-can for new OE chore: bump python deps for 3.10 and robot system updates Dec 20, 2023
@@ -27,6 +27,6 @@ runs:
- shell: bash
run: |
npm install --global shx@0.3.3
$OT_PYTHON -m pip install pipenv==2021.5.29
Copy link
Member

Choose a reason for hiding this comment

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

among other things! note that we'll need to downgrade this to v2023.10.13 if we want to a) keep 3.7 support and b) ensure that by having i guess separate pipfiles (we shouldn't do this, if we want to do something like that we should set up a tox environment just for the api stack)

@@ -75,17 +75,13 @@ jobs:
os: ['windows-2022', 'ubuntu-22.04', 'macos-latest']
# TODO(mc, 2022-02-24): expand this matrix to 3.8 and 3.9,
# preferably in a nightly cronjob on edge or something
python: ['3.7', '3.10']
python: ['3.10']
Copy link
Member

Choose a reason for hiding this comment

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

see above re: keeping 3.7 support and testing for it

exclude:
- os: 'macos-latest'
python: '3.10'
python: ['3.10']
Copy link
Member

Choose a reason for hiding this comment

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

see above re: python 3.7 support

@@ -66,7 +66,7 @@ PYTHON_SETUP_TARGETS := $(addsuffix -py-setup, $(PYTHON_DIRS))

.PHONY: setup-py
setup-py:
$(OT_PYTHON) -m pip install pipenv==2021.5.29
$(OT_PYTHON) -m pip install pipenv==2023.11.15
Copy link
Member

Choose a reason for hiding this comment

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

see above re: tested versions and pipenv versions

@@ -7,24 +7,24 @@ name = "pypi"
# atomicwrites and colorama are pytest dependencies on windows,
# spec'd here to force lockfile inclusion
# https://github.com/pypa/pipenv/issues/4408#issuecomment-668324177
atomicwrites = { version = "==1.4.0", sys_platform = "== 'win32'" }
colorama = { version = "==0.4.4", sys_platform = "== 'win32'" }
atomicwrites = { version = "==1.4.0", markers="sys_platform=='win32'" }
Copy link
Member

Choose a reason for hiding this comment

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

one annoying subtle pipenv change was they altered how you spell markers in pipfiles

@@ -6,43 +6,40 @@ name = "pypi"
[packages]
fastapi = "==0.68.1"
uvicorn = "==0.14.0"
anyio = "==3.3.0"
anyio = "==3.6.1"
Copy link
Member

Choose a reason for hiding this comment

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

from system builder

pydantic = "==1.8.2"
sqlalchemy = "==1.4.32"
systemd-python = { version = "==234", sys_platform = "== 'linux'" }
pydantic = "==1.9.2"
Copy link
Member

Choose a reason for hiding this comment

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

for mypy

return web.json_response( # type: ignore[no-untyped-call,no-any-return]
data={"message": msg}, status=400
)
return web.json_response(data={"message": msg}, status=400)
Copy link
Member

Choose a reason for hiding this comment

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

typed aiohttp? the future is now!

@@ -97,6 +97,8 @@ async def status(request: web.Request, session: UpdateSession) -> web.Response:
async def _save_file(part: BodyPartReader, path: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

all this stuff wasn't type safe at all and now it should be

@sfoster1 sfoster1 marked this pull request as ready for review December 20, 2023 20:37
@sfoster1 sfoster1 requested review from a team as code owners December 20, 2023 20:37
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Hallelujah.

Here are some preliminary comments and questions. I'll take a closer look when we return in January, but I don't anticipate anything that would block this from merging.

@y3rsh
Copy link
Collaborator

y3rsh commented Jan 3, 2024

Are we addressing #11905 request here?

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

I've reviewed this in detail and it looks great to merge whenever you're ready with Opentrons/oe-core#110 and Opentrons/buildroot#213. Thank you so much, @caila-marashaj and @sfoster1, for pulling this off!

@sfoster1
Copy link
Member

sfoster1 commented Jan 3, 2024

Are we addressing #11905 request here?

No, but I want to do the following:

@sfoster1 sfoster1 merged commit 4f8a983 into edge Jan 4, 2024
57 of 58 checks passed
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.

6 participants