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 ability to pull a wheel artifact as well as a conda one #131

Merged
merged 8 commits into from
Jan 3, 2025

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jan 3, 2025

This generalizes the preexisting rapids-get-pr-conda-artifact to support wheels via rapids-get-pr-artifact. The former is now a trivial passthrough to the latter for backwards compatibility.

@vyasr vyasr added feature request New feature or request non-breaking Introduces a non-breaking change labels Jan 3, 2025
@vyasr vyasr self-assigned this Jan 3, 2025
@vyasr vyasr changed the title FAdd ability to pull a wheel artifact as well as a conda one Add ability to pull a wheel artifact as well as a conda one Jan 3, 2025
@vyasr vyasr marked this pull request as ready for review January 3, 2025 03:43
@jameslamb
Copy link
Member

jameslamb commented Jan 3, 2025

Have you seen that there's already a rapids-get-pr-wheel-artifact that does this? Would that meet the need for whatever you're trying to do?

https://github.com/rapidsai/gha-tools/blob/main/tools/rapids-get-pr-wheel-artifact

I've found it helpful for testing PRs that rely on wheels from other PRs / repos.

@vyasr
Copy link
Contributor Author

vyasr commented Jan 3, 2025

Wow. Well... no 😢 my local copy of gha-tools must have been very out of date, and I haven't done nearly as much wheel development of late. Ah well. I guess the question now is whether it's worth using this PR as an opportunity for cleanup to make rapids-get-pr-conda-artifact and rapids-get-pr-wheel-artifact both call the new function. The two scripts are basically copies of each other and now that I've already made this PR it seems like we might as well do to the work to unify them. WDYT? If you'd rather skip that idea I can close this PR.

@jameslamb
Copy link
Member

I'm supportive of consolidating more of the shared logic into one place like this, if you want to spend the time on it! Happy to review and help test.

If rapids-get-pr-artifact is intended to be just for code-sharing and not called explicitly, I think for clarity it should be prefixed with an underscore, like _rapids-download-from-s3: https://github.com/rapidsai/gha-tools/blob/main/tools/_rapids-download-from-s3

@vyasr
Copy link
Contributor Author

vyasr commented Jan 3, 2025

I'm supportive of consolidating more of the shared logic into one place like this, if you want to spend the time on it! Happy to review and help test.

If I hadn't already written this I wouldn't bother with the work, but now that I've come this far it seems a shame to waste it. It shouldn't take more than a few minutes to propagate the changes

If rapids-get-pr-artifact is intended to be just for code-sharing and not called explicitly, I think for clarity it should be prefixed with an underscore, like _rapids-download-from-s3: https://github.com/rapidsai/gha-tools/blob/main/tools/_rapids-download-from-s3

Sure I can do that. In the long run maybe it would make more sense to not have separate tools at all, but that's a minor cleanup task that we could do later.

@vyasr
Copy link
Contributor Author

vyasr commented Jan 3, 2025

Done.

@vyasr vyasr requested a review from jameslamb January 3, 2025 19:34
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Just one minor suggestion, do what you want with it, otherwise this looks good to me!

tools/_rapids-get-pr-artifact Outdated Show resolved Hide resolved
@vyasr vyasr merged commit 0558ffc into rapidsai:main Jan 3, 2025
1 check passed
@vyasr vyasr deleted the feat/get_wheel_artifact branch January 3, 2025 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants