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.

Sign up for GitHub

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

Jump to bottom

[FEA] Relax requirements for versions.json git_url / git_tag for proprietary binaries #702

Open
bdice opened this issue Oct 15, 2024 · 4 comments · May be fixed by #703 or #704
Open

[FEA] Relax requirements for versions.json git_url / git_tag for proprietary binaries #702

bdice opened this issue Oct 15, 2024 · 4 comments · May be fixed by #703 or #704
Labels
? - Needs Triage Need team to review and classify feature request New feature or request

Comments

@bdice
Copy link
Contributor

bdice commented Oct 15, 2024 •
edited
Loading

Is your feature request related to a problem? Please describe.
cuDF has not supported nvcomp 2.2.0 for a long time, but rapids-cmake's versions.json still has an entry for nvcomp 2.2.0 and the outdated repo page (the nvcomp repo states, "This page will be retired soon" so the git_url will soon be irrelevant). However, we can't remove this outdated information because the versions.json parser expects git_url and git_tag to exist.

Describe the solution you'd like
Update the parser so that proprietary packages can be used with only proprietary_binary information, without specifying git_url and git_tag.

Describe alternatives you've considered
One alternative would be to allow an empty string like "git_url": "" / "git_tag": "", but that also fails with rapids_cmake can't parse 'nvcomp' json entry, it is missing a `git_url` entry.

Additional context
Related to rapidsai/cudf#16757.
@bdice bdice added ? - Needs Triage Need team to review and classify feature request New feature or request labels Oct 15, 2024
@bdice bdice changed the title [FEA] Relax requirements for versions.json [FEA] Relax requirements for versions.json git_url / git_tag for proprietary binaries Oct 15, 2024
@bdice bdice mentioned this issue Oct 15, 2024
Closed
@bdice
Copy link
Contributor Author

bdice commented Oct 15, 2024 •
edited
Loading

@robertmaynard @KyleFromNVIDIA Could you weigh in on this proposal and/or offer guidance on how to implement it?

@KyleFromNVIDIA
Copy link
Contributor

This sounds fine to me. Updating the logic in rapids-cmake with this exception should be pretty straightforward.

KyleFromNVIDIA added a commit to KyleFromNVIDIA/rapids-cmake that referenced this issue Oct 15, 2024
@KyleFromNVIDIA
Make git tag and repository optional
dc4f69e 
To allow the removal of nvcomp 2.2.0, we make the git tag and repo
arguments optional, since all nvcomp versions now use the proprietary
binary instead.

Fixes: rapidsai#702
@KyleFromNVIDIA KyleFromNVIDIA linked a pull request Oct 15, 2024 that will close this issue
Open
7 tasks
@robertmaynard
Copy link
Contributor

The general design across rapids-cmake is to rely on required fields to well exist. Making a required field conditonal based on the existence of another field will require us to change the pinning logic.

There is very minimal validation performed on the git_url or git_tag fields. So something like no_git_checkout_allowed would be fine for the git url for nvcomp.

Given that nvcomp is our only proprietary_binary package, I am not super in favor of increasing our parsing complexity to support that single use case.

@bdice
Copy link
Contributor Author

bdice commented Oct 16, 2024 •
edited
Loading

@robertmaynard Thanks, that's exactly the kind of feedback I was hoping for. I will open an alternative PR with that change.

edit: See #704.

@bdice bdice linked a pull request Oct 16, 2024 that will close this issue
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
? - Needs Triage Need team to review and classify feature request New feature or request
Projects
None yet
Milestone
No milestone
3 participants
@robertmaynard @bdice @KyleFromNVIDIA