StackStorm / st2

StackStorm (aka "IFTTT for Ops") is event-driven automation for auto-remediation, incident responses, troubleshooting, deployments, and more for DevOps and SREs. Includes rules engine, workflow, 160 integration packs with 6000+ actions (see https://exchange.stackstorm.org) and ChatOps. Installer at https://docs.stackstorm.com/install/index.html
https://stackstorm.com/
Apache License 2.0
5.96k stars 741 forks source link

Pants requirements #6181

Closed cognifloyd closed 2 months ago

cognifloyd commented 2 months ago

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
nzlosh commented 2 months ago

This is going to cause a lot of conflicts with the pr for python3.10 https://github.com/StackStorm/st2/pull/6157. What are these changes needed for?

winem commented 2 months ago

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 commented 2 months ago

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.

nzlosh commented 2 months ago

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.

cognifloyd commented 2 months ago

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?

nzlosh commented 2 months ago

Much less scary! Thanks for aligning this with the pinnings thus far :heart:

winem commented 2 months ago

Really appreciate the effort and the solution!