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

Unpin Python requirements #11912

Closed
wants to merge 2 commits into from
Closed

Conversation

ecederstrand
Copy link

Overview

Fixes #11905

Changelog

  • Allow dependency versions up to the next major version when pinning is not strictly necessary. This makes it easier to combine opentrons with other Python packages that have shared dependencies.

@ecederstrand ecederstrand requested a review from a team as a code owner December 20, 2022 14:38
@ecederstrand ecederstrand changed the title Allow dependencies up to next major version Unpin Python requirements Dec 21, 2022
@sfoster1
Copy link
Member

This is a really good idea and I'd like to get it in.

For it to completely remove conflicts like what you're describing, we'll also have to do it for shared-data:

INSTALL_REQUIRES = [
"jsonschema==3.0.2",
"typing-extensions>=4.0.0,<5",
"pydantic==1.8.2",
]
since opentrons depends on shared-data.

@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Merging #11912 (b491578) into edge (98df3db) will not change coverage.
The diff coverage is n/a.

❗ Current head b491578 differs from pull request most recent head fe2e9a2. Consider uploading reports for the commit fe2e9a2 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #11912   +/-   ##
=======================================
  Coverage   74.33%   74.33%           
=======================================
  Files        2158     2158           
  Lines       59492    59492           
  Branches     6225     6225           
=======================================
  Hits        44224    44224           
  Misses      13800    13800           
  Partials     1468     1468           
Flag Coverage Δ
notify-server 89.13% <ø> (ø)

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

@y3rsh
Copy link
Member

y3rsh commented Dec 21, 2022

Thank you for the PR.
I have incorporated this into #11682 which we are iterating over for stability to improve python environment management and testing.

opentrons/api/setup.py

Lines 60 to 71 in c21cd55

INSTALL_REQUIRES = [
f"opentrons-shared-data=={VERSION}",
"aionotify>=0.2.0,<0.3",
"anyio>=3.3.0,<4",
"jsonschema>=3.0.2,<4",
"numpy>= 1.15.1, <=1.21.6",
"pydantic>=1.8.2,<2",
"pyserial>=3.5,<4",
"typing-extensions>=4.0.0,<5",
"click>=8.0.1,<9",
'importlib-metadata >= 1.0 ; python_version < "3.8"',
]

INSTALL_REQUIRES = [
"jsonschema>=3.0.2,<4",
"typing-extensions>=4.0.0,<5",
"pydantic>=1.8.2,<2",
]

@ecederstrand
Copy link
Author

Thanks! Feel free to close this PR as necessary.

@ecederstrand
Copy link
Author

ecederstrand commented Oct 11, 2023

@y3rsh #11682 was closed without merge: #11682 (comment)

Can you revisit this MR? We still have the need to unpin requirements.

@faruqsandi
Copy link

It is almost a year!

@sfoster1
Copy link
Member

sfoster1 commented Feb 2, 2024

Hey @ecederstrand , I'm sorry, I know it's been forever, but we're doing a big upgrade of a bunch of our dependencies now. Do version ranges like this: https://github.com/Opentrons/opentrons/pull/14420/files#diff-7bac1606de7de80231cd062b4bee4ba9e7874be65ce0dbdbfc20f31e4cc0dc31 work for you?

@ecederstrand
Copy link
Author

ecederstrand commented Feb 4, 2024

Yeah, any dependency that is a range instead of pinned would work for me.

sfoster1 added a commit that referenced this pull request Feb 7, 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.
@sfoster1
Copy link
Member

sfoster1 commented Feb 7, 2024

Okay, we've merged #14420 and it will be released in 7.2.0 - I'm going to close this PR. Thank you for bringing this to our attention, and sorry it took so long to fix it.

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.

feat: Unpin requirements
4 participants