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.

Sign up for GitHub

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

Jump to bottom

WX-1192 Updated semver versions for flagged dependencies #790

Merged
merged 5 commits into from
Aug 2, 2023
Merged

WX-1192 Updated semver versions for flagged dependencies #790

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eff2f64
WX-1192 updated semver versions for flagged dependencies
JVThomas Jul 27, 2023
c31c008
WX-1192 added constraints.txt so that it doesn't install and break on…
JVThomas Jul 27, 2023
64a74b5
WX-1192 updated dockerfiles and readme to include constraints.txt
JVThomas Jul 28, 2023
fd3388c
WX-1192 added oxford comma
JVThomas Jul 31, 2023
30c8987
WX-1192 updated pip install commands to correctly account for constra…
JVThomas Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions servers/cromwell/.swagger-codegen-ignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ README.md
.gitignore
tox.ini
test-requirements.txt
constraints.txt

# These have been hand-modified, no need to regenerate as they would get overwritten.
requirements.txt
Expand Down
1 change: 1 addition & 0 deletions servers/cromwell/constraints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
cython<3.0.0
Copy link

Choose a reason for hiding this comment

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

Will this constraint apply to every python package? I'm worried that it will end up causing more problems down the road as other packages begin to rely on new cython features. Is it possible to constrain just the problematic package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it will, however we pin dependencies to their exact values. If a fixed dependency updates it's Cython sub-dependency from v2 to v3 and still uses the deprecated features of Cython v2 then it'll break as well.

Constraining the version won't change the functionality of the library, and new features on a library would be tied to a version update which JM won't see due to pinned versions.

Copy link

Choose a reason for hiding this comment

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

That makes sense.

1 change: 1 addition & 0 deletions servers/cromwell/tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
envlist = py310

[testenv]
setenv=PIP_CONSTRAINT={toxinidir}/constraints.txt
Copy link

Choose a reason for hiding this comment

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

I believe it's good practice to include the constraints.txt inside a particular requirements.txt. I think it could make it a little easier down the road to figure out what is being constrained and why. This article seems helpful. Not a huge deal if there's some reason we can't do that.

Copy link
Collaborator Author

@JVThomas JVThomas Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I went with the above per tox documentation here, which outlines setting constraints as seen above.

Didn't know you could do the above (most Github posts have people adding the -c <file name> flag to pip install), but seeing how it conflicts with the documented usage in tox I've opted for this instead.

That said I do need to update any pip install references to include the constraints flag. I'll add a new commit that does it.

Copy link

Choose a reason for hiding this comment

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

I'm in favor of doing what tox documentation does.

Good catch on the pip/docker stuff!

deps=-r{toxinidir}/requirements.txt
-r{toxinidir}/test-requirements.txt

Expand Down
4 changes: 4 additions & 0 deletions ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,9 @@
"ts-node": "^10.9.1",
"typescript": "~4.8.2"
},
"resolutions": {
"node-gyp/semver@^7.3.5": "7.5.2",
"@npmcli/fs/semver@^7.3.5": "7.5.2"
},
"packageManager": "yarn@3.2.2"
}
11 changes: 11 additions & 0 deletions ui/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9250,6 +9250,17 @@ __metadata:
languageName: node
linkType: hard

"semver@npm:7.5.2":
version: 7.5.2
resolution: "semver@npm:7.5.2"
dependencies:
lru-cache: ^6.0.0
bin:
semver: bin/semver.js
checksum: 3fdf5d1e6f170fe8bcc41669e31787649af91af7f54f05c71d0865bb7aa27e8b92f68b3e6b582483e2c1c648008bc84249d2cd86301771fe5cbf7621d1fe5375
languageName: node
linkType: hard

"semver@npm:^5.3.0, semver@npm:^5.6.0":
version: 5.7.1
resolution: "semver@npm:5.7.1"
Expand Down
Loading