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

Use rapids-generate-version for env var #622

Open
wants to merge 3 commits into
base: branch-25.04
Choose a base branch
from
Open

Use rapids-generate-version for env var #622

wants to merge 3 commits into from

Conversation

jakirkham
Copy link
Member

Follow up on this thread: #616 (comment)

@jakirkham jakirkham requested a review from a team as a code owner February 5, 2025 22:53
@jakirkham jakirkham requested a review from bdice February 5, 2025 22:53
@jakirkham jakirkham added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Feb 5, 2025
@jakirkham jakirkham requested review from madsbk and vyasr and removed request for bdice February 5, 2025 22:53
@jakirkham jakirkham mentioned this pull request Feb 5, 2025
Merged
vyasr
vyasr reviewed Feb 6, 2025
View reviewed changes
ci/build_python.sh Outdated
@@ -22,7 +22,7 @@ conda config --set path_conflict prevent

sccache --zero-stats

RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
RAPIDS_PACKAGE_VERSION=$(rapids-generate-version) rapids-conda-retry mambabuild \
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the function is being called once above, hence why I previously just read the output.

Copy link
Member Author

@jakirkham jakirkham Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the line Vyas referenced

kvikio/ci/build_python.sh

Line 16 in 173c62c

rapids-generate-version > ./VERSION

And we need to that line as the VERSION file has to be written out there

Copy link
Member

Choose a reason for hiding this comment

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

I think this is slightly preferably. I misunderstood the original code, I though that ./VERSION was written just to be read here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment to that line. Also shared a few examples below

madsbk
madsbk approved these changes Feb 6, 2025
View reviewed changes
ci/build_python.sh Outdated
@@ -22,7 +22,7 @@ conda config --set path_conflict prevent

sccache --zero-stats

RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
RAPIDS_PACKAGE_VERSION=$(rapids-generate-version) rapids-conda-retry mambabuild \
Copy link
Member

Choose a reason for hiding this comment

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

I think this is slightly preferably. I misunderstood the original code, I though that ./VERSION was written just to be read here :)

@bdice bdice mentioned this pull request Feb 7, 2025
Merged
rapids-bot bot pushed a commit that referenced this pull request Feb 8, 2025
@bdice
Use rapids-generate-version for package versions (#628)
5a825df 
This uses the `RAPIDS_PACKAGE_VERSION` values set in #616. This ensures we have consistent nightly versions.

This PR is independent of #622 (it is needed regardless of whether that PR is closed or merged).

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

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #628
jakirkham
jakirkham commented Feb 10, 2025
View reviewed changes
ci/build_python.sh
Comment on lines +16 to 17
# This file is used throughout to define the version
rapids-generate-version > ./VERSION
Copy link
Member Author

Choose a reason for hiding this comment

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

Here are a few examples of where VERSION is used

kvikio/python/kvikio/kvikio/_version.py

Lines 17 to 19 in c38038e

__version__ = (
importlib.resources.files(__package__).joinpath("VERSION").read_text().strip()
)

kvikio/python/kvikio/pyproject.toml

Lines 137 to 140 in c38038e

[tool.scikit-build.metadata.version]
provider = "scikit_build_core.metadata.regex"
input = "kvikio/VERSION"
regex = "(?P<value>.*)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr left review comments

@madsbk madsbk madsbk approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakirkham @madsbk @vyasr