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

Reduce amount of hard-coding of RAPIDS version #15

Closed
KyleFromNVIDIA opened this issue Jan 24, 2024 · 21 comments
Closed

Reduce amount of hard-coding of RAPIDS version #15

KyleFromNVIDIA opened this issue Jan 24, 2024 · 21 comments
Assignees

Comments

@KyleFromNVIDIA
Copy link
Contributor

Many (most?) projects have an update-version.sh script that uses sed expressions to replace the RAPIDS version in the repository's files. Many of these hard-coded usages of the version can be replaced with smarter dynamic reading of the VERSION file, and the remaining usages that must be hard-coded can be updated by a centralized hook in https://github.com/rapidsai/pre-commit-hooks that reads a configuration file from the repo.

In this issue, I propose two RAPIDS-wide changes:

  • Intelligently read from VERSION wherever possible. Read version from VERSION file in CMake cudf#14867 serves as an example for updating CMake code to read from VERSION. Other scripts in other languages can do something similar.
  • Add a hook to https://github.com/rapidsai/pre-commit-hooks that searches for version strings and warns you about hard-coding version numbers. In some cases (README files, etc.), hard-coding is unavoidable, and the hook can be configured to instead update these files when the version changes.
@KyleFromNVIDIA
Copy link
Contributor Author

@jakirkham
Copy link
Member

Yes! Really like this idea

A single file with just this value makes it easy to read and use anywhere (CMake, Python, Bash, etc.)

@vyasr
Copy link
Contributor

vyasr commented Jan 24, 2024

I was going to write up a list of ways I see this working, but it looks like you've already started on some of the core ones so I'll just let you keep pushing on! Thanks for tackling this.

@KyleFromNVIDIA
Copy link
Contributor Author

KyleFromNVIDIA commented Jan 25, 2024

For cases in which hard-coding is infeasible (due to the file not being a script that can read the VERSION file or something), I was thinking we could have a .rapids_metadata_template file next to it that's formatted with str.format(). The linter would use this to ensure that the file always stays up to date. If the linter finds the RAPIDS version string in a file without a corresponding .rapids_metadata_template, then it will issue a warning.

For example, if README.txt says:

The next RAPIDS version is 24.04.

The `frobnicator()` function was deprecated in 24.02 and will be removed in 24.04.

Then README.txt.rapids_metadata_template would say:

The next RAPIDS version is {RAPIDS_VERSION_MAJOR_MINOR}.

The `frobnicator()` function was deprecated in 24.02 and will be removed in 24.04.

@KyleFromNVIDIA
Copy link
Contributor Author

I've opened rapidsai/pre-commit-hooks#13 to implement the above idea.

jakirkham added a commit to rapidsai/pynvjitlink that referenced this issue Feb 20, 2024
* Update to 0.1.13
* Use `VERSION` file to set version dynamically

xref: rapidsai/build-planning#15
@jakirkham
Copy link
Member

This was done for pynvjitlink in PR: rapidsai/pynvjitlink#58

@vyasr
Copy link
Contributor

vyasr commented Feb 22, 2024

rapidsai/pynvjitlink#58 also includes an example of how we can read versions in recipes (see https://github.com/rapidsai/pynvjitlink/blob/main/conda/recipes/pynvjitlink/meta.yaml#L4). This isn't generally applicable to RAPIDS packages, which set versions during package build time via https://github.com/rapidsai/gha-tools/blob/main/tools/rapids-generate-version (which uses git tag/distance information under the hood), but it's worth keeping in mind in case there are other special cases like pynvjitlink that don't use that script for generating versions.

@jakirkham
Copy link
Member

Am actually curious why the pynvjitlink approach wouldn't work in more cases. It is doing the same thing scikit-build-core is doing in pyproject.toml (for example)

@vyasr
Copy link
Contributor

vyasr commented Feb 23, 2024

Actually you are right. I take back what I said, we should be able to do this in the rest of RAPIDS as well since we overwrite the VERSION file in the build scripts with the appropriate setting. That would allow us to make some simplifications. To aplpy the changes, we would do (using the cudf repo as an example):

  1. Copy the change from pynvjitlink
  2. Stop setting the version variable in recipes: https://github.com/rapidsai/cudf/blob/branch-24.04/conda/recipes/cudf/meta.yaml#L3
  3. Stop using the RAPIDS_PACKAGE_VERSION variable in the build scripts

Since we already overwrite the VERSION file, changing the recipes to read that file should "just work" as far as pulling the right version.

@jakirkham
Copy link
Member

Ok cool. Wasn't sure if I'd missed something particular in the wheel case

Thanks Vyas! 🙏

That enumeration should provide a good guide forward

@jakirkham
Copy link
Member

jakirkham commented Feb 28, 2024

Is there a good way to do this for GHA shared-workflows?

Edit: Looks like this may be possible as this example demonstrates

@vyasr
Copy link
Contributor

vyasr commented Feb 28, 2024

Yes, we ought to be able to do it using some combination of the environment variables available in GHA. In particular, using GITHUB_REF and GITHUB_BASE_REF combined with whether the workflow is a pull-request or not we should be able to figure out what shared workflows branch to pull in subsequent jobs. We would add an initial job to every workflow that uses those variables to determine the branch and set an output, then use that in the subsequent jobs to determine the workflow to use.

rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Feb 29, 2024
Following up on issue ( rapidsai/build-planning#15 ), drop RAPIDS version hard-coding in doc builds.

Authors:
  - https://github.com/jakirkham

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Ray Douglass (https://github.com/raydouglass)

URL: #15101
rapids-bot bot pushed a commit to rapidsai/cucim that referenced this issue Feb 29, 2024
* Drop version update steps for doc pages that no longer exist: #666
* Use the `main` branch in links as that is updated every release

xref: rapidsai/build-planning#15

Authors:
  - https://github.com/jakirkham

Approvers:
  - Gregory Lee (https://github.com/grlee77)
  - Ray Douglass (https://github.com/raydouglass)

URL: #705
@KyleFromNVIDIA
Copy link
Contributor Author

Unfortunately, the uses field has to be static. You can't do something like:

uses: rapidsai/shared-workflows/.github/workflows/pr-builder.yaml@${{inputs.rapids_branch}}

Some people have come up with a workaround for this, where they create a proxy workflow that then calculates the real workflow to use. This would require use to create a workflow repository or directory that doesn't change between RAPIDS versions.

@vyasr
Copy link
Contributor

vyasr commented Mar 4, 2024

Ah that's too bad, I wasn't aware of that limitation. That definitely complicates things.

@KyleFromNVIDIA
Copy link
Contributor Author

The shared-workflows repo doesn't currently mention the 24.04 branch anywhere in the code. What would be the feasibility of removing the per-version branch and using only a single branch instead? There is the issue that only 24.04 and newer supports Python 3.11, but we can probably figure out a way to deal with that.

@jakirkham
Copy link
Member

Agree that sounds like a good option. Let's raise that as an issue on the shared-workflows repo and discuss there

@bdice
Copy link
Contributor

bdice commented Mar 4, 2024

We chose to have versioned branches for shared-workflows specifically because of the need for reproducibility with RAPIDS releases and hotfixes, etc. In my eyes this is a minor subpoint of the larger question of whether RAPIDS can adopt a single development branch. I don't think this is feasible in the short-term but I'm firmly in support of the longer-term goal of a single development branch.

@jakirkham
Copy link
Member

My guess is GH doesn't want us to use variables in uses sections to avoid vulnerabilities that would introduce. So if we don't want to make changes in shared-workflows, then we may need to consider other options

Would add there are probably several ways we could consider changes in shared-workflows. So wouldn't anchor to the first idea we come up with

@vyasr
Copy link
Contributor

vyasr commented Mar 4, 2024

Given that this task is general fairly low-priority IMO, I would suggest that we focus on making all the other changes we want that don't involve shared workflows and then coming back to the shared workflows last and seeing where we stand on moving RAPIDS to a simpler branching strategy. I don't expect that change to happen soon, but I suspect it'll take us a while to root out all the other versioning changes first too.

rapids-bot bot pushed a commit to rapidsai/rmm that referenced this issue Mar 8, 2024
* Read `VERSION` file from CMake
* Read `rmm.__version__` from docs build
* Read `VERSION` file from shell scripts
* Remove updates from `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #1496
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this issue Mar 8, 2024
* Read `VERSION` file from CMake
* Read `cuml.__version__` from docs build
* Read `VERSION` file from `ci/build_docs.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #5793
rapids-bot bot pushed a commit to rapidsai/cucim that referenced this issue Mar 8, 2024
* Read `cucim.__version__` in docs build
* Remove update from `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #711
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Mar 8, 2024
Doxyfiles support environment variable substitution, so read the version from `VERSION` and put it in an environment variable.

Also remove a hard-coded version from `ci/check_style.sh`.

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #15231
KyleFromNVIDIA added a commit to KyleFromNVIDIA/cuvs that referenced this issue Mar 8, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/cuxfilter that referenced this issue Mar 8, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/kvikio that referenced this issue Mar 11, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/kvikio that referenced this issue Mar 11, 2024
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this issue Mar 12, 2024
* Read `VERSION` file in CMake
* Read `cuspatial.__version__` and `cuproj.__version__` in docs build
* Read `VERSION` file in shell scripts
* Use environment variable substitution in Doxygen
* Remove updates from `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)
  - Mark Harris (https://github.com/harrism)

URL: #1357
rapids-bot bot pushed a commit to rapidsai/raft that referenced this issue Mar 12, 2024
* Read `VERSION` file from CMake
* Read `pylibraft.__version__` from docs build
* Read `VERSION` file from shell scripts
* Use environment variable substitution in Doxygen
* Remove updates from `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Jake Awe (https://github.com/AyodeAwe)
  - Bradley Dice (https://github.com/bdice)
  - Divye Gala (https://github.com/divyegala)

URL: #2219
rapids-bot bot pushed a commit to rapidsai/cuvs that referenced this issue Mar 12, 2024
* Read `VERSION` file from CMake
* Read `cuvs.__version__` from docs build
* Read `VERSION` file from shell scripts
* Use variable substitution in Doxyfile
* Update `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)
  - Ben Frederickson (https://github.com/benfred)

URL: #45
rapids-bot bot pushed a commit to rapidsai/cuxfilter that referenced this issue Mar 12, 2024
* Read `cuxfilter.__version__` from docs build
* Update `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #579
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this issue Mar 12, 2024
* Add `VERSION` file
* Read `VERSION` file from CMake
* Read `VERSION` file from Python
* Read `kvikio.__version__` from docs build
* Read `VERSION` file from shell scripts
* Use variable substitution in Doxyfile
* Remove unused `ci/checks/style.sh`
* Update `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)
  - Vukasin Milovanovic (https://github.com/vuule)
  - https://github.com/jakirkham

URL: #351
rapids-bot bot pushed a commit to rapidsai/ucxx that referenced this issue Mar 13, 2024
* Read `VERSION` file from CMake
* Read `ucxx.__version__` from docs build
* Read `VERSION` file from shell scripts
* Use variable substitution in Doxyfile
* Update `pyproject.toml` to use dynamic version
* Update `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)
  - https://github.com/jakirkham

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)
  - Peter Andreas Entschev (https://github.com/pentschev)

URL: #201
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this issue Mar 13, 2024
* Read `VERSION` file in CMake
* Read `cugraph.__version__` in docs build
* Read `VERSION` file in shell scripts
* Use environment variables in Doxyfile
* Remove updates from `ci/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Don Acosta (https://github.com/acostadon)
  - Chuck Hastings (https://github.com/ChuckHastings)
  - Brad Rees (https://github.com/BradReesWork)
  - Jake Awe (https://github.com/AyodeAwe)
  - https://github.com/jakirkham

URL: #4217
rapids-bot bot pushed a commit to rapidsai/raft that referenced this issue May 2, 2024
@KyleFromNVIDIA
Copy link
Contributor Author

Closed as completed.

@jakirkham
Copy link
Member

Thanks Kyle! 🙏

difyrrwrzd added a commit to difyrrwrzd/cuvs that referenced this issue Aug 10, 2024
* Read `VERSION` file from CMake
* Read `cuvs.__version__` from docs build
* Read `VERSION` file from shell scripts
* Use variable substitution in Doxyfile
* Update `ci/release/update-version.sh`

Issue: rapidsai/build-planning#15

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Bradley Dice (https://github.com/bdice)
  - Jake Awe (https://github.com/AyodeAwe)
  - Ben Frederickson (https://github.com/benfred)

URL: rapidsai/cuvs#45
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

No branches or pull requests

4 participants