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 hook to verify RAPIDS version isn't hard-coded #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KyleFromNVIDIA
Copy link
Contributor

No description provided.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

The rapids_metadata_template piece is new to me. Is that documented somewhere?

@KyleFromNVIDIA
Copy link
Contributor Author

.rapids_metadata_template is a brand new concept introduced by this PR. The idea is to use a pre-commit hook to ensure that any file must contain the RAPIDS version (due to it not being a script that can read the VERSION file) is kept up to date with the VERSION file.

Where would be a good place to write documentation on this?

@bdice
Copy link
Contributor

bdice commented Jan 30, 2024

Where would be a good place to write documentation on this?

A new page here about pre-commit hooks, perhaps? https://docs.rapids.ai/resources That source code is here: https://github.com/rapidsai/docs/tree/main/resources

@ajschmidt8 may have thoughts about this, since there is some work in progress on the website.

@bdice
Copy link
Contributor

bdice commented Jan 30, 2024

.rapids_metadata_template is a brand new concept introduced by this PR. The idea is to use a pre-commit hook to ensure that any file must contain the RAPIDS version (due to it not being a script that can read the VERSION file) is kept up to date with the VERSION file.

I'm not sure if I understand the implementation. What are the contents of this file? Where does it go? It looks like we would be maintaining two files, like file.txt and file.txt.rapids_metadata_template, and changes would have to go in the template file? I'm not excited about adding template files to RAPIDS repos...

@KyleFromNVIDIA
Copy link
Contributor Author

It looks like we would be maintaining two files, like file.txt and file.txt.rapids_metadata_template, and changes would have to go in the template file?

Yes, that's correct. This is one idea for how to enforce not hard-coding the RAPIDS version. I agree it's not ideal, and I'm open to suggestions for alternatives.

@bdice
Copy link
Contributor

bdice commented Feb 4, 2024

I thought a bit about ways to centralize this information, so we don't have to maintain a bunch of template files (which I think introduces a confusing abstraction layer and breaks the normal development flow). I've used bumpversion before, which has a centralized configuration file that lists the files containing a version string and the current version (so it knows what string to replace). (Note: the original project is no longer maintained, but the c4urself/bump2version fork is still alive.) It isn't quite flexible enough to fit RAPIDS' needs, but I think something like this would be the best case scenario. That said, this concept is not terribly different than the ci/release/update-version.sh script. It would just be a solution that doesn't rely on sed.

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.

2 participants