-
-
Notifications
You must be signed in to change notification settings - Fork 745
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
Update Orquesta to v1.6.0 #6050
Changes from all commits
633ec10
880e5d2
4e13147
aa4e19e
7f47b36
abf9a80
6cd25e8
f0e0bfa
0e73c4c
e1b0938
cbd0ee0
16f27aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
orquesta@ git+https://github.com/StackStorm/orquesta.git@v1.5.0 | ||
orquesta@ git+https://github.com/StackStorm/orquesta.git@v1.6.0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ mongoengine | |
# networkx version is constrained in orquesta. | ||
networkx | ||
orjson | ||
orquesta @ git+https://github.com/StackStorm/orquesta.git@v1.5.0 | ||
orquesta @ git+https://github.com/StackStorm/orquesta.git@v1.6.0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to play a bit with pants. @cognifloyd For
the dependency conflict made no sense to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remove
weird 🤔 |
||
# NOTE: Recent version substantially affect the performance and add big import time overhead | ||
# See https://github.com/StackStorm/st2/issues/4160#issuecomment-394386433 for details | ||
oslo.config>=1.12.1,<1.13 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@armab what happens if you change networkx here to match orquesta, e.g. >=2.6 & < 3
and then re-generate the requirements files
and then re-generate pants.
Wondering if that is why it complains that it can't rectifiy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though also orquesta looks a bit more complicated...
requirements.txt:networkx>=2.5.1,<2.6; python_version < '3.7'
requirements.txt:networkx>=2.6,<3; python_version >= '3.7'
So we have different dependencies for different python versions, I wonder if the pants generate lockfiles copes with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amanda11 Yeah, I tried that in my first approach. See the build failures here: #6050 (comment)
Outside of issues in
st2
with fixate-requirements script not allowing duplicate records ofnetworkx
, I've found later that pip under EL7 didn't work as expected with the environment markers during thes2-packages
build: #6050 (comment)After all the tries in
st2-packages
andst2
the current approach is the way of less resistance and at least I got the packages build working (with additional #6050 (comment) fix)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the pants lockfile side, I saw
st2/lockfiles/st2.lock
Line 134 in 8f6cb46
so technically it may support things, but I experienced issues with pip too, so 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pants does not support using the legacy resolver. I'm not sure why the legacy resolver is required in this case, as it seems clear to me. I'll have to play with it, hopefully this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cognifloyd Thanks for looking into it 👍 Can't wait and I'm looking forward to merge this PR so I can continue updating the dependencies for the
v3.8.1
patch.Some context on the issue with dependencies.
For some unknown reason, the new pip resolver went into an infinite loop resolving the dependencies: https://app.circleci.com/pipelines/github/StackStorm/st2/4158/workflows/11838dac-5adc-4554-9a87-f7bf506bedb6/jobs/15940?invite=true#step-109-978547_123 and couldn't process that
networkx
with conditionalpython_version
env markers failing the EL7 build:In this ^^ case
orquesta 1.6.0 depends on networkx<3 and >=2.6
is a wrong condition chosen, asEL7
runs on py3.6 and should choose a different networkx for dependency. So that might be a bug in pip.I've tried to use the latest pip version available for py3.6, but it didn't help.
From the pip changelog there might be a relevant fix to try: (When checking for conflicts in the build environment, correctly skip requirements containing markers that do not match the current environment.) available in pip
22.1
, but we can't use it because py3.6 support was dropped in pip22.0
.For
pants
build, something pip buggy is happening as well, but more 👀 would be appreciated.My thinking that's the worst case, for the upcoming st2
v3.8.1
patch we'll live with it and take what worked in this PR forst2-packages
. The new pants build system is not going to go live for this patch anyway.In st2
v3.9.0
we can drop python 3.6, switch to a new pip dependency resolver, latest pip and remove python env markers alltogether. That'll be quite an exercism that should potentially fix some bugs and technical debt 😈