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

Fix package versioning #616

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Fix package versioning #616

merged 2 commits into from
Feb 5, 2025

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 5, 2025

The nightly package versions are currently not being correctly assigned.

@vyasr vyasr added bug Something isn't working non-breaking Introduces a non-breaking change labels Feb 5, 2025
@vyasr vyasr self-assigned this Feb 5, 2025
@vyasr vyasr requested a review from a team as a code owner February 5, 2025 00:52
@vyasr vyasr requested a review from gforsyth February 5, 2025 00:52
@jakirkham
Copy link
Member

Thanks Vyas! 🙏

Does this affect released versions as well? IOW is there value in having this for 25.02? Or not really

madsbk
madsbk approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @vyasr

ci/build_python.sh
rapids-logger "Begin py build"

CPP_CHANNEL=$(rapids-download-conda-from-s3 cpp)
conda config --set path_conflict prevent

sccache --zero-stats

rapids-conda-retry mambabuild \
RAPIDS_PACKAGE_VERSION=$(head -1 ./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.

Why not just:

Suggested change
RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
RAPIDS_PACKAGE_VERSION=$(rapids-generate-version) rapids-conda-retry mambabuild \

Copy link
Contributor Author

@vyasr vyasr Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure we could do that if you prefer, I just used this approach since we're already calling that function above to overwrite the VERSION file. If you want to change it feel free to apply this suggestion, I'll leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madsbk once you resolve this thread either way feel free to merge.

Copy link
Member

Choose a reason for hiding this comment

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

Missed this thread was blocking. Looks like Vyas and I posted at the same time in fact

Have added this change in a new PR: #622

We can decide whether or not to include that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 5, 2025

Thanks Vyas! 🙏

Does this affect released versions as well? IOW is there value in having this for 25.02? Or not really

No, this only affects nightlies which is why I targeted 25.04.

@jameslamb jameslamb removed the request for review from gforsyth February 5, 2025 21:48
@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit b001413 into rapidsai:branch-25.04 Feb 5, 2025
59 checks passed
@jakirkham
Copy link
Member

Thanks everyone! 🙏

@vyasr
Copy link
Contributor Author

vyasr commented Feb 5, 2025 •
edited
Loading

I had intended to let @madsbk decide what he wanted to do with #616 (comment) before merging, but it's a minor point. We can always make a follow-up PR if needed.

@vyasr vyasr deleted the fix/package_versioning branch February 5, 2025 22:41
@jakirkham
Copy link
Member

Oops sorry. Saw his approval. So assumed this was ready to go

Yep we can always follow up

@jakirkham jakirkham mentioned this pull request Feb 5, 2025
Open
@madsbk
Copy link
Member

madsbk commented Feb 6, 2025

No worries

@vyasr vyasr mentioned this pull request Feb 7, 2025
Merged
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham left review comments

@madsbk madsbk madsbk approved these changes

@jameslamb jameslamb jameslamb approved these changes

Assignees

@vyasr vyasr

Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

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