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

Migrate as much as possible to pyproject.toml #5251

Merged
merged 8 commits into from
Mar 7, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 25, 2023

Move most Python package information from setup.py into pyproject.toml and MANIFEST.in.

@vyasr vyasr added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 25, 2023
@vyasr vyasr requested review from a team as code owners February 25, 2023 00:18
@vyasr vyasr self-assigned this Feb 25, 2023
@github-actions github-actions bot added ci Cython / Python Cython or Python issue labels Feb 25, 2023
Copy link
Contributor

@csadorf csadorf 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 overall (one potential issue commented in-line), but I tested the ci/release/update-version.sh script with bash ./ci/release/update-version.sh 23.06 and it yielded non-sensical results for the dependencies.yaml file:

diff --git a/dependencies.yaml b/dependencies.yaml
index 2e4704b5b..d5bac132e 100644
--- a/dependencies.yaml
+++ b/dependencies.yaml
@@ -73,11 +73,11 @@ dependencies:
         packages:
           - c-compiler
           - cxx-compiler
-          - libcumlprims=23.04.*
-          - libraft-headers=23.04.*
-          - libraft-distance=23.04.*
-          - libraft-nn=23.04.*
-          - rmm=23.04.*
+          - libcumlprims=23.063.063.04.*
+          - libraft-headers=23.063.063.04.*
+          - libraft-distance=23.063.063.04.*
+          - libraft-nn=23.063.063.04.*
+          - rmm=23.063.063.04.*

python/MANIFEST.in Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Mar 6, 2023

@csadorf can you make sure that you have all tag available locally (git fetch origin --tags) and see if you get the same result? The only way I was able to reproduce your error was with a repository where no tags were present. That produced the same nonsense results you showed since then sed just uses a single character "." as the "CURRENT_SHORT_TAG" to replace.

This may be partly due to needing to refresh your cuml repo due to that recent rebase of the main branch invalidating a bunch of git refs, including old tags.

@vyasr vyasr requested a review from csadorf March 6, 2023 20:36
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@csadorf
Copy link
Contributor

csadorf commented Mar 7, 2023

@csadorf can you make sure that you have all tag available locally (git fetch origin --tags) and see if you get the same result? The only way I was able to reproduce your error was with a repository where no tags were present. That produced the same nonsense results you showed since then sed just uses a single character "." as the "CURRENT_SHORT_TAG" to replace.

This may be partly due to needing to refresh your cuml repo due to that recent rebase of the main branch invalidating a bunch of git refs, including old tags.

Indeed, that was the issue. After re-fetching all tags with git fetch upstream --tags --force, the output is now sensible.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

LGTM!

@vyasr
Copy link
Contributor Author

vyasr commented Mar 7, 2023

/merge

@rapids-bot rapids-bot bot merged commit 3d33289 into rapidsai:branch-23.04 Mar 7, 2023
@vyasr vyasr deleted the feat/all_pyproject branch March 9, 2023 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team ci Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants