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

add rapids-get-pr-wheel-artifact for quickly downloading cpp wheels #101

Merged
merged 8 commits into from
Apr 16, 2024

Conversation

msarahan
Copy link
Contributor

@msarahan msarahan commented Apr 2, 2024

This idea comes from a slack DM with Viyas:

OK, so the first piece I'm thinking of is artifact download. If you look at the raft PR, we have snippets like this:
artifact_name=$(RAPIDS_PY_WHEEL_NAME="librmm_${RAPIDS_PY_CUDA_SUFFIX}" RAPIDS_REPOSITORY=rmm RAPIDS_PY_VERSION="3.11" rapids-package-name wheel_python)
commit=$(git ls-remote https://github.com/rapidsai/rmm.git refs/heads/pull-request/1512 | cut -c1-7)
librmm_wheelhouse=$(rapids-get-artifact "ci/rmm/pull-request/1512/${commit}/${artifact_name}")

The snippet above demonstrates two issues:
There isn't an easy way to download arbitrary PR wheel artifacts. However, there is such a tool for conda artifacts. That was added in #85. What we want is to generalize this tool to work for wheels.
The librmm wheel should not be specific to a particular Python version because it's a pure C++ wheel. However, our current naming scheme on downloads.rapids.ai doesn't know that. I'm currently hacking it by specifying the RAPIDS_PY_VERSION="3.11". This is working because I'm restricting the C++ wheel build CI job to only run with Python 3.11. There was another recent gha-tools PR that added support for pure Python wheels. What we want is to generalize this tool to also work for C++ wheels. The main difference is that C++ wheels have to care about arch, whereas pure Python wheels do not

I think a good starting point would be to make PRs to gha-tools to address the above issues. Does that make sense?

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.

@msarahan msarahan requested a review from vyasr April 2, 2024 19:56
@vyasr
Copy link
Contributor

vyasr commented Apr 2, 2024

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.

@msarahan
Copy link
Contributor Author

msarahan commented Apr 3, 2024

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:

# default assumes wheel_python. If manually specified, do that instead.
pkg_name="$(rapids-package-name wheel_python)"

if [ "$1" = "cpp" ] || [ "$1" = "python" ]; then
	pkg_type=$1
	pkg_name="$(rapids-package-name "wheel_${pkg_type}")"
	# remove pkg_type from args because we handle it in this script
	shift;
fi

@msarahan msarahan marked this pull request as ready for review April 3, 2024 19:03
Copy link
Contributor

@vyasr vyasr left a 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.

tools/rapids-get-pr-wheel-artifact Show resolved Hide resolved
@@ -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
Copy link
Contributor

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).

Copy link
Contributor

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.

Copy link
Member

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
Copy link
Contributor

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.

tools/rapids-package-name Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Apr 11, 2024

It would also be good to update the docs showing how this works once we finalize and merge this tool.

tools/rapids-package-name Outdated Show resolved Hide resolved
Copy link
Member

@ajschmidt8 ajschmidt8 left a 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

@msarahan msarahan merged commit 4c878b8 into rapidsai:main Apr 16, 2024
1 check passed
@msarahan msarahan deleted the get-pr-wheel-artifact branch April 17, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants