-
Notifications
You must be signed in to change notification settings - Fork 16
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
add rapids-get-pr-wheel-artifact for quickly downloading cpp wheels #101
Conversation
Note that we may not update every repo that's using these tools to the new wheels strategy, but we will at least have to ensure that we update them for the breaking changes here. |
On second thought, the breaking change here may not be the best approach. I thought it would be a helpful guide for which repos need fixing to add wheel support. If it's going to disrupt things with a lot of repos for no change to those repos, then I think it might be better to preserve the wheel_python default behavior. Maybe something like:
|
93c5a5d
to
6ecb362
Compare
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.
Yikes I reviewed this a week ago but apparently I never submitted. Sorry about that.
@@ -6,6 +6,14 @@ set -eo pipefail | |||
|
|||
source rapids-prompt-local-repo-config | |||
|
|||
pkg_name="$(rapids-package-name wheel_python)" | |||
# For legacy reasons, allow this script to be run without the pkg_type being the first arg |
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.
Another option that would probably be more consistent with other scripts in this repo would be to use an environment variable. I'm not the biggest fan of how much we've proliferated those, and maybe what we really need to start doing is introducing some proper argument parsing (e.g. via getopts), but that's out of scope for this PR (but curious what others like @bdice and @ajschmidt8 think about 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.
I'm fine with the code here. We can have a follow-up to clean up all uses of the script so they declare the pkg_type
.
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.
To be honest I'm hoping we can remove a lot of these scripts entirely pending the outcome of https://github.com/rapidsai/ops/issues/2982.
If that issue ends up being rolled out, I think that would be a good time to rethink some of the interfaces for any of the helper scripts that we do keep around.
@@ -6,6 +6,14 @@ set -eo pipefail | |||
|
|||
source rapids-prompt-local-repo-config | |||
|
|||
pkg_name="$(rapids-package-name wheel_python)" | |||
# For legacy reasons, allow this script to be run without the pkg_type being the first arg |
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.
I'm fine with the code here. We can have a follow-up to clean up all uses of the script so they declare the pkg_type
.
It would also be good to update the docs showing how this works once we finalize and merge this tool. |
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.
No questions/objections from me.
Approved pending the resolution of Brad's questions
This idea comes from a slack DM with Viyas:
At the time that I'm submitting this, I haven't yet found examples of the wheels_cpp files. Vyas is working on fleshing those out, and I will be taking over that work from him once he gets it on the right track.