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

Pants requirements #6181

Merged
merged 6 commits into from
Apr 12, 2024
Merged

Pants requirements #6181

merged 6 commits into from
Apr 12, 2024

Conversation

cognifloyd
Copy link
Member

Using the pants venv, the tests all pass with these requirements. I hacked the Makefile to test that in CI in #6130. This extracts just the requirements bits from that PR.

Lockfile diff: lockfiles/st2.lock [st2]

==                    Upgraded dependencies                     ==

  amqp                           5.0.6        -->   5.2.0
  bcrypt                         3.2.0        -->   4.1.2
  cffi                           1.14.6       -->   1.16.0
  eventlet                       0.30.3       -->   0.36.1
  filelock                       3.13.3       -->   3.13.4
  kombu                          5.2.2        -->   5.3.6
  oslo-utils                     4.13.0       -->   7.1.0
  stevedore                      2.0.1        -->   5.2.0
  tenacity                       6.3.1        -->   8.2.3
  vine                           5.0.0        -->   5.1.0

==                      Added dependencies                      ==

  typing-extensions              4.11.0

Using the pants venv, the tests all pass with these requirements.
I hacked the Makefile to test that in CI in #6130.
This extracts just the requirements bits from that PR.

Lockfile diff: lockfiles/st2.lock [st2]

==                    Upgraded dependencies                     ==

  amqp                           5.0.6        -->   5.2.0
  bcrypt                         3.2.0        -->   4.1.2
  cffi                           1.14.6       -->   1.16.0
  eventlet                       0.30.3       -->   0.36.1
  filelock                       3.13.3       -->   3.13.4
  kombu                          5.2.2        -->   5.3.6
  oslo-utils                     4.13.0       -->   7.1.0
  stevedore                      2.0.1        -->   5.2.0
  tenacity                       6.3.1        -->   8.2.3
  vine                           5.0.0        -->   5.1.0

==                      Added dependencies                      ==

  typing-extensions              4.11.0
@cognifloyd cognifloyd added this to the pants milestone Apr 10, 2024
@cognifloyd cognifloyd requested review from winem, nzlosh, mamercad and a team April 10, 2024 20:31
@cognifloyd cognifloyd self-assigned this Apr 10, 2024
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Apr 10, 2024
@nzlosh
Copy link
Contributor

nzlosh commented Apr 10, 2024

This is going to cause a lot of conflicts with the pr for python3.10 #6157. What are these changes needed for?

@winem
Copy link
Contributor

winem commented Apr 10, 2024

This PR looks good to me but I'd like to clarify what @nzlosh just mentioned before approving it. Let's try to align on the way to go and reduce the number of conflicts to a minimum.

@cognifloyd
Copy link
Member Author

This is the set of requirements that allows us the tests to pass in a venv built by pants on both py3.8 and py3.9. This is the next step towards switching from nosetest to pytest. With a known good set of locked requirements, most of the changes required should be in the tests.

I'm trying very hard to minimize the version constraints we have to define manually. If we need to put in a minimum or maximum version, that's fine. But in general I think we should should avoid == requirements to allow pip+pex as much flexibility as possible in resolving dependencies.

Looking over #6157, I'm scared by the manual pins (== requirements) in lockfiles/st2-constraints.txt and requirements-pants.txt. Can we loosen them at all to list a range of possible versions?

What if we copied the versions that pip+pex selected from lockfiles/st2.lock into fixed-requirements.txt? And maybe copy parts of #6157 into smaller PRs that can be merged now before #6157 is ready? That would hopefully reduce the merge conflicts.

@pull-request-size pull-request-size bot removed the size/L PR that changes 100-499 lines. Requires some effort to review. label Apr 10, 2024
@nzlosh
Copy link
Contributor

nzlosh commented Apr 10, 2024

I'm working across py3.8, py3.9, py3.10 to find a common working combination of st2 core dependencies. I've opted for very specific pinning of dependencies during this phase to avoid surprises in module behaviour. Having the same module revision on all python versions makes for less variability and simpler debugging. This way I know I'm comparing apples with apples and not inadvertently hitting a bug on py3.10 because the pinning was too loose, there was a regression or other sorts of things. Sometimes, version bumps require code revision in the test and/or core so having the same module version makes reasoning about these changes straight forward as well.

I do agree with allowing flexibility in the pinning but my concern is the PRs being created are only against py3.8 and py3.9, which doesn't take into account py3.10 constraints. Would you be OK with loosening pinning once I've found a working baseline between our targeted python versions (I think I'm getting close).

We might be able to copy some of the changes into smaller PRs, but #6157 changes were done in an iterative fashion, as I worked on repairing test units, deprecation warnings or updating modules to have py3.10 support as well as dropping py3.6 constraints. So I don't see a clear separation between these changes to allow a PR that would yield a passing set of tests. That said, I'll take a look at the commits to see if there's some way to break it up cleanly.

@pull-request-size pull-request-size bot added the size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. label Apr 10, 2024
This should support the rest of the updated requirements copied from the lockfile.
@cognifloyd
Copy link
Member Author

cognifloyd commented Apr 10, 2024

I went through the requirements in fixed-requirements.txt and test-requirements.txt and copied the locked version from lockfiles/st2.lock using == pins. I left the source files used to generate lockfiles/st2.lock (like lockfiles/st2-constraints.txt and requirements-pants.txt) with the broad requirements used to generate the lockfile.

Looking through that PR, it looks like most of the locked versions match what you changed, with a few exceptions where you've made code changes to support a bumped version.

With the updated requirements, rstcheck and pip-compile were failing, so I copied your updates for those.

@nzlosh Does this looks like a less-scary merge conflict with #6157?

This should support the rest of the updated requirements copied from the lockfile.
@nzlosh
Copy link
Contributor

nzlosh commented Apr 11, 2024

Much less scary! Thanks for aligning this with the pinnings thus far ❤️

@winem
Copy link
Contributor

winem commented Apr 12, 2024

Really appreciate the effort and the solution!

@cognifloyd cognifloyd merged commit 911e172 into master Apr 12, 2024
29 checks passed
@cognifloyd cognifloyd deleted the pants-requirements branch April 12, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependency pantsbuild size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants