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: update python deps to breaking change breakpoints #14420

Merged
merged 23 commits into from
Feb 7, 2024

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Feb 2, 2024

This is a PRs to update python dependencies to modern versions. This one stops at the major breaking change point of pydantic 2.0. While Pydantic 2.0 is apparently much much faster (it's written in rust if you can believe it) it also makes a bunch of breaking api changes and in theory provides back-compat patches. Similarly, fastapi 0.100.0 switches internally to depend on pydantic 2, with similarly theoretical back compat patches. So, we've updated to,

Dependency Version Changes

In prod, major deps (full list is essentially entirely in the robot-server pipfile, see below for why):

  • fastapi 0.99.0 (from 0.68.1)
  • pydantic 1.10.12 (from 1.9.2)
  • uvicorn 0.27.0 (from 0.14.0)
  • sqlalchemy 1.4.51 (from 1.4.32 - bigger change than you think, and breaking changes above this)

In dev tooling,

  • mypy 1.8.0 (from 0.981)
  • flake8 7 (from like 3 or something we never updated this)
  • mock 5 (from 4)
  • decoy 2 (from 1)
  • pytest 7.4.4 except robot-server, which is limited by tavern to 7.3 (from various)
  • tavern 2.9.1 (from 1.6)

Version Definition Changes

Some people want to use our python libraries that are published on pypi. The problem with this is that our libraries - opentrons and opentrons_shared_data have explicit version pinning in their setup.py install_requires, which makes it incredibly hard for them to coexist with other python packages that aren't comaintained by us (see #11912 , #12839 ). One way to fix this would be version ranges in the setup.py and explicit versions (that are contained within those ranges, and that match or define what's present on the robot) in the pipfiles. We couldn't do this because pipenv had problems with it.

Now, however, we've upgraded pipenv, and that strategy works! And since I was going around bumping all the deps anyway, I could figure out what the actual functional dependency version boundaries were. So as part of this, opentrons (api/setup.py) and opentrons_shared_data (shared-data/python/setup.py) now have version ranges for all of their install_requires that aren't other opentrons packages, and I'm pretty sure about those version ranges. They may be smaller than would be ideal, but they're real.

Testing (and how to get this out of draft)

This PR should stay in draft until the system builders are known to have the right dependency versions.

  • Flex build with appropriate deps
  • OT-2 build with appropriate deps
  • usb-bridge works on flex
  • robot-server works on flex
  • robot-server works on ot-2
  • update-server works on flex
  • update-server works on ot-2
  • jupyter works on ot-2
  • jupyter works on flex

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

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

Comparison is base (c77a53b) 68.12% compared to head (c8813d1) 67.77%.
Report is 2 commits behind head on chore_release-7.2.0.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.2.0   #14420      +/-   ##
=======================================================
- Coverage                68.12%   67.77%   -0.36%     
=======================================================
  Files                     2518     2518              
  Lines                    72031    71972      -59     
  Branches                  9243     9245       +2     
=======================================================
- Hits                     49073    48779     -294     
- Misses                   20755    20991     +236     
+ Partials                  2203     2202       -1     
Flag Coverage Δ
app 64.58% <ø> (-0.07%) ⬇️
g-code-testing 92.43% <0.00%> (-4.06%) ⬇️
hardware 57.30% <42.85%> (ø)
protocol-designer 37.93% <ø> (ø)
shared-data 75.26% <75.00%> (ø)
step-generation 86.90% <ø> (ø)
system-server 96.04% <100.00%> (ø)
update-server 50.56% <ø> (-13.03%) ⬇️
usb-bridge 76.94% <100.00%> (ø)

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

Files Coverage Δ
api/src/opentrons/calibration_storage/helpers.py 95.45% <ø> (-0.55%) ⬇️
...rons/drivers/asyncio/communication/async_serial.py 97.72% <ø> (-0.06%) ⬇️
api/src/opentrons/drivers/rpi_drivers/gpio.py 4.65% <ø> (-0.56%) ⬇️
api/src/opentrons/drivers/serial_communication.py 28.76% <ø> (-2.82%) ⬇️
...c/opentrons/drivers/smoothie_drivers/driver_3_0.py 80.69% <ø> (-0.03%) ⬇️
...rc/opentrons/drivers/smoothie_drivers/simulator.py 80.76% <ø> (+1.13%) ⬆️
.../opentrons/hardware_control/backends/controller.py 72.89% <ø> (-0.17%) ⬇️
...entrons/hardware_control/backends/ot3controller.py 68.48% <ø> (ø)
...pentrons/hardware_control/backends/ot3simulator.py 86.23% <ø> (ø)
...rc/opentrons/hardware_control/execution_manager.py 89.39% <ø> (-0.16%) ⬇️
... and 53 more

... and 22 files with indirect coverage changes

In 4.18 which you may note is not a major version change jsonschema
split off all of its reference management stuff into an independent
library and kept some backwards compatibility shims which... aren't. So
limit it to <4.18 and set 4.17.3 in the pipfile to prevent having to
change a bunch of stuff about how we do internal referencing.
Some typing changes, a couple test changes. protocol api old tests
seemed broken. The command schema weirdly needs to be updated.
The difference here is the switch from anyOf to oneOf for the command
create, which seems to make no difference since only one element can be present.
- pydantic 1.10 (last before 2's breaking changes)
- fastapi 0.99 (last before it focused on pydantic 2)
- mypy 1.8
- pytest 7.2 (tavern can't tolerate >7.3, even latest tavern)

Fixes from all the above. Pretty minor, but there were two annoying
ones and a weird one.
Annoying:
- Fastapi started trying to shove all responses into pydantic fields if
you didn't provide a response= value to tis route method. pydantic
doesn't want to put our PydanticResponses in fields. Fix: specify the
response model _contained_ in the PydanticResponse in the route handler.
This is annoying, and luckily we now have paramspec, so make a decorator
decorator that encompasses both the actual endpoint method and the route
method call so you don't have to remember.
- Fastapi's openapi integration got more specific about requiring
examples to be exactly jsonschema example keywords, which are lists of
objects that don't have associated docs when you're putting examples in
a schema component

Weird:
Fastapi started properly noting that filenames are optional in file
upload bodies, and also specifies BytesIO, and mypy finally noticed that
mutable attributes of Protocols must be invariant per the PEP, so we
needed to change some api-side type descriptors in the protocol file
readers. No functinoality change, just wanted to mention because that's
why this commit touches api.
Mypy 1.8, pytest 7.4.4. Minimal changes.
aiohttp 3.9, mypy 1.8, pytest 7.4
mypy 1.8, diff match 2023, pytest 7.4.4
@sfoster1
Copy link
Member Author

sfoster1 commented Feb 5, 2024

@sfoster1 sfoster1 changed the base branch from edge to chore_release-7.2.0 February 7, 2024 13:33
@sfoster1 sfoster1 marked this pull request as ready for review February 7, 2024 14:55
@sfoster1 sfoster1 requested review from a team as code owners February 7, 2024 14:55
@sfoster1
Copy link
Member Author

sfoster1 commented Feb 7, 2024

Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Works well on Flex during run and no issues with analysis on app or robot. FastAPI changelog might want to be appraised to be sure that there isn't anything that might conflict with how we've used it so far.

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.

Sweet,
once we start using this more then that might expose specific issues.
until then, this looks good to me!

Comment on lines -77 to +79
if self._state == ExecutionState.CANCELLED:
# type-ignore needed because this is a reentrant function and narrowing cannot
# apply
if self._state == ExecutionState.CANCELLED: # type: ignore[comparison-overlap]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, this seems like bad mypy behavior.

api/src/opentrons/protocols/models/json_protocol.py Outdated Show resolved Hide resolved
@sfoster1 sfoster1 merged commit ae4e2e3 into chore_release-7.2.0 Feb 7, 2024
51 of 52 checks passed
@sfoster1 sfoster1 deleted the bump-py-deps branch February 7, 2024 21:29
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.

4 participants