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

build(api,shared-data): replace static versioning with intervals #12839

Closed
wants to merge 1 commit into from

Conversation

marcosgdiaz
Copy link

@marcosgdiaz marcosgdiaz commented Jun 2, 2023

Overview

Change static dependencies versioning with intervals. This allows opentrons to be compatible with third-party software.

Test Plan

We are waiting for the OT-2 to arrive and therefore we could not test it on the hardware yet. On the other hand, test will be run on the CI when creating the pull request.

Changelog

  • Replace static dependencies versions with intervals

Review requests

Risk assessment

The dependency jsonschema follows semantic versioning and not breaking changes were introduced in 4.X.X. Hereby we included up until version 5 in jsonschema>=3.0.2,<5

@marcosgdiaz marcosgdiaz requested a review from a team as a code owner June 2, 2023 12:13
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #12839 (1b501b9) into edge (c5102c5) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12839      +/-   ##
==========================================
- Coverage   73.39%   73.39%   -0.01%     
==========================================
  Files        2283     2304      +21     
  Lines       62669    63196     +527     
  Branches     6802     6918     +116     
==========================================
+ Hits        45997    46382     +385     
- Misses      15055    15189     +134     
- Partials     1617     1625       +8     
Flag Coverage Δ
app 71.60% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 46.35% <ø> (ø)
shared-data 75.43% <ø> (-0.93%) ⬇️
step-generation 88.64% <ø> (ø)

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

see 21 files with indirect coverage changes

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.

@marcosgdiaz Thank you for the pull request!

I think we need to modify this PR slightly to add the specific versions that were previously in INSTALL_REQUIRES to the pipenv files for this module; for shared-data; and for robot-server. We really want to use exact versions to test because those exact versions are the ones we use on the OT-2, as set in buildroot. Currently, the reason the exact versions are in the setup.py here is that we delegate installing those dependencies from pipenv to here.

If we move the exact dependency specifications to the pipfiles for these modules and for the modules that use these as a dependency, I think we should be able to get the setup pys to specify ranges.

@sfoster1
Copy link
Member

sfoster1 commented Feb 2, 2024

@marcosgdiaz Hey, I'm sorry, I know it's been a really long time. Do version ranges like this: https://github.com/Opentrons/opentrons/pull/14420/files#diff-7bac1606de7de80231cd062b4bee4ba9e7874be65ce0dbdbfc20f31e4cc0dc31 work for you?

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

We just merged #14420 and will release it in 7.2.0, so I'm going to close this PR. Thank you for bringing this to our attention, and sorry we took so long to fix it.

@sfoster1 sfoster1 closed this Feb 7, 2024
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.

2 participants