rapids-generate-pip-constraints: stop special-casing 'latest'#247
rapids-generate-pip-constraints: stop special-casing 'latest'#247rapids-bot[bot] merged 4 commits intomainfrom
Conversation
jameslamb
added
breaking
| 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" \ |
There was a problem hiding this comment.
Notice I'm slipping in use_cuda_wheels=true here.
That key is used in dependencies.yaml files across RAPIDS to avoid things like nvidia-cusparse or cuda-toolkit making it into output lists in RAPIDS devcontainers or DLFW builds.
We always want those lists in all the places rapids-generate-pip-constraints is used, though, so this passes it unconditionally.
| --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}" |
There was a problem hiding this comment.
here's the change switching from overwriting to appending
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
why append instead of overwrite
So you can build up a constraints file over multiple steps if you want, for example for a mix of constraints from dependencies.yaml and {package} @ file:// stuff from downloaded files.
Appending makes updates to whatever file PIP_CONSTRAINT points to idempotent and allows you to do that in any order. With overwriting, you'd right multiple files and then pass each on the command line with --constraint, which is just more opportunity to mistakenly forget one.
Maybe we should make this write to stdout if no output file is provided, and refactor towards removing the outfile entirely.
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 rapids-echo-stderr instead of rapids-logger.
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.
There was a problem hiding this comment.
🤷♂️ Okay! I agree moving it forward is better. We can refactor later if needed.
There was a problem hiding this comment.
Alright yep definitely! Thank you. I'll merge this right now, I'm around to help if it breaks something.
|
It looks to me like merging this would not break CI anywhere (see the draft PRs in "How I tested this"), I think it's ready for review. |
jameslamb
changed the title
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Approving to unblock but would like to think more about appending/overwriting before merge.
|
/merge |
2 participants
Contributes to rapidsai/build-planning#256
rapids-generate-pip-constraintscurrently special-casesRAPIDS_DEPENDENCIES="latest"and skips generating constraints in that case.This will be helpful in rapidsai/build-planning#256, where we want to start constraining
cuda-toolkitin wheels CI based on the CTK version in the CI image being used.Notes for Reviewers
How I tested this
Looked for projects using this (GitHub search) and tested in them.
It's just a few:
On all of those, wheels CI jobs worked exactly as expected and without needing any code changes or
dependencies.yamlupdates... so this PR is safe to merge any time.Is this safe?
It should be (see "How I tested this").
This is only used to add constraints (not requirements), so it shouldn't change our ability to catch problems like "forgot to declare a dependency" in CI.
It WILL increase the risk of
[test]extras being underspecified. For example, ifcuml[test]hasscikit-learn>=1.3and the constraints havescikit-learn>=1.5, we might never end up testingscikit-learn>=1.3,<1.5(unless it's explicitly accounted for in adependencies: "oldest"block).The other risk here is that this creates friction because constraints passed to
--constraintcannot contain extras. So e.g. if you want to depend onxgboost[dask], that cannot be in any of the lists generated byrapids-generate-pipe-constraints. I think we can work around that though when we hit those cases.Overall, I think these are acceptable tradeoffs.