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

Update Orquesta to v1.6.0 #6050

Merged
merged 12 commits into from
Nov 20, 2023
Merged

Update Orquesta to v1.6.0 #6050

merged 12 commits into from
Nov 20, 2023

Conversation

arm4b
Copy link
Member

@arm4b arm4b commented Oct 25, 2023

Update Orquesta to v1.6.0 to be included in the next st2 release.
Orquesta v1.6.0 comes with some updated dependencies.

@arm4b arm4b added the WIP label Oct 25, 2023
@arm4b arm4b added this to the 3.8.1 milestone Oct 25, 2023
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Oct 25, 2023
fixed-requirements.txt Outdated Show resolved Hide resolved
Make sure lines with markers are considered as unique requirements and can be duplicated
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Oct 26, 2023
fixed-requirements.txt Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Oct 26, 2023
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like unpinning much better.

@arm4b
Copy link
Member Author

arm4b commented Oct 26, 2023

Yeah, I tried to go with updating fixate-requirements.txt script to support duplicate requirements with markers (for different python versions added in orquestra requirements.txt).

But went alternative way by unpinning the networkx in the st2 requirements, because the specific version is requested and installed by the orquesta dependencies here: https://github.com/Stealthii/orquesta/blob/7dadd4cfa0e01a2fd65a545fbcdaad363bee4c68/requirements.txt#L5-L7

networkx>=2.5.1,<2.6; python_version < '3.7'
networkx>=2.6,<3; python_version >= '3.7'

I'm getting what I want during the build stage:

Now the only blocker is 🔴 EL7 (py3.6) which is getting the python_version markers wrong (or ignoring them?) for some reason taking this requirement line for py3.8 instead:
https://app.circleci.com/pipelines/github/StackStorm/st2/4158/workflows/11838dac-5adc-4554-9a87-f7bf506bedb6/jobs/15940?invite=true#step-109-929748_119

[package: st2] [18:59:09]      ERROR: pip's legacy dependency resolver does not consider dependency conflicts when selecting packages. This behaviour is the source of the following dependency conflicts.
[package: st2] [18:59:09]      networkx 2.5.1 requires decorator<5,>=4.3, but you'll have decorator 5.1.1 which is incompatible.
WRONG ----> [package: st2] [18:59:09]      orquesta 1.6.0 requires networkx<3,>=2.6, but you'll have networkx 2.5.1 which is incompatible.

I'm guessing something is outdated related to the build env for EL7 that doesn't quite work with dependency markers.
@cognifloyd @nzlosh @amanda11 Any ideas to try?

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Oct 27, 2023
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Oct 27, 2023
@arm4b
Copy link
Member Author

arm4b commented Oct 27, 2023

Figured it out in StackStorm/st2-packages#728, - which is RFR now (this PR relies on that st2-packages branch).
Need to fix e2e tests first before merging this, but this PR is RFR too.

@arm4b arm4b marked this pull request as ready for review October 27, 2023 13:36
@arm4b arm4b requested review from a team and cognifloyd October 27, 2023 13:36
@@ -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
Copy link
Member Author

@arm4b arm4b Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to play a bit with pants.

@cognifloyd For ./pants generate-lockfiles --resolve=st2 I've got the following error:

ERROR: Cannot install networkx and orquesta==1.6.0 because these package versions have conflicting dependencies.
ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies
 
 The conflict is caused by:
     The user requested networkx
     orquesta 1.6.0 depends on networkx<3 and >=2.6
 
 To fix this you could try to:
 1. loosen the range of package versions you've specified
 2. remove package versions to allow pip attempt to solve the dependency conflict

the dependency conflict made no sense to me

Copy link
Member Author

@arm4b arm4b Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove networkx from requirements-pants.txt I'm getting the following during the pants lock generation:

ERROR: Could not find a version that satisfies the requirement networkx<3,>=2.6 (from orquesta)
ERROR: No matching distribution found for networkx<3,>=2.6

weird 🤔

# networkx v2.6 does not support Python3.6. Update networkx to match orquesta
networkx>=2.5.1,<2.6
# required by orquesta (networkx<2.6 for py3.6, networkx<3 for py3.8)
networkx<3
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Member Author

@arm4b arm4b Oct 30, 2023

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 of networkx, I've found later that pip under EL7 didn't work as expected with the environment markers during the s2-packages build: #6050 (comment)

After all the tries in st2-packages and st2 the current approach is the way of less resistance and at least I got the packages build working (with additional #6050 (comment) fix)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the pants generate lockfiles copes with that?

On the pants lockfile side, I saw

"requires_python": ">=3.6",

so technically it may support things, but I experienced issues with pip too, so 🤷‍♂️

Copy link
Member

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.

Copy link
Member Author

@arm4b arm4b Oct 30, 2023

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 conditional python_version env markers failing the EL7 build:

[package: st2] [18:59:15]      ERROR: Cannot install orquesta==1.6.0 and st2==3.9.dev0 because these package versions have conflicting dependencies.
[package: st2] [18:59:15]      
[package: st2] [18:59:15]      The conflict is caused by:
[package: st2] [18:59:15]          st2 3.9.dev0 depends on networkx
[package: st2] [18:59:15]          orquesta 1.6.0 depends on networkx<3 and >=2.6

In this ^^ case orquesta 1.6.0 depends on networkx<3 and >=2.6 is a wrong condition chosen, as EL7 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 pip 22.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 for st2-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 😈

@arm4b arm4b added security and removed WIP labels Nov 3, 2023
@arm4b arm4b merged commit 83b0a8f into master Nov 20, 2023
38 checks passed
@arm4b arm4b deleted the update/orquesta branch November 20, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants