-
Notifications
You must be signed in to change notification settings - Fork 21
rapids-generate-pip-constraints: stop special-casing 'latest' #247
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| # Positional arguments: | ||
| # | ||
| # 1) file key to be passed to `rapids-dependency-file-generator --file-key` | ||
| # 2) filepath to write the constraint data to | ||
| # 2) filepath to append the constraint data to (creates it if it doesn't exist) | ||
| # | ||
| # [usage] | ||
| # | ||
|
|
@@ -23,14 +23,8 @@ export RAPIDS_SCRIPT_NAME="rapids-generate-pip-constraints" | |
| file_key="${1}" | ||
| out_file="${2}" | ||
|
|
||
| # 'latest' should be a no-op and not constrain dependencies at all | ||
| # (pip will prefer the latest versions of dependencies by default) | ||
| if [[ "${RAPIDS_DEPENDENCIES}" == "latest" ]]; then | ||
| echo "" > "${out_file}" | ||
| else | ||
| rapids-dependency-file-generator \ | ||
| --output requirements \ | ||
| --file-key "${file_key}" \ | ||
| --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" \ | ||
| | tee "${out_file}" | ||
| fi | ||
| rapids-dependency-file-generator \ | ||
| --output requirements \ | ||
| --file-key "${file_key}" \ | ||
| --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES};use_cuda_wheels=true" \ | ||
| | tee -a "${out_file}" | ||
|
Member
Author
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. here's the change switching from overwriting to appending
Contributor
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. Why append instead of overwrite? I think the caller should handle that explicitly, like rapids-generate-pip-constraints >> append_to_meMaybe we should make this write to stdout if no output file is provided, and refactor towards removing the outfile entirely.
Member
Author
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.
So you can build up a constraints file over multiple steps if you want, for example for a mix of constraints from Appending makes updates to whatever file
I don't personally see that as any better than the behavior currently in this PR, but it is harder to get right because you have to think about output redirection (for example, you have to be sure to use It'd also create PR churn because it'd require coordinating code changes in all the repos using this (which this PR does not). If you read that and are unconvinced, say it and I'll switch this to writing to stdout and make changes in the repos. I care more about moving this forward than I do the exact form.
Contributor
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. 🤷♂️ Okay! I agree moving it forward is better. We can refactor later if needed.
Member
Author
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. Alright yep definitely! Thank you. I'll merge this right now, I'm around to help if it breaks something. |
||
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.
Notice I'm slipping in
use_cuda_wheels=truehere.That key is used in
dependencies.yamlfiles across RAPIDS to avoid things likenvidia-cusparseorcuda-toolkitmaking it into output lists in RAPIDS devcontainers or DLFW builds.We always want those lists in all the places
rapids-generate-pip-constraintsis used, though, so this passes it unconditionally.