-
Notifications
You must be signed in to change notification settings - Fork 200
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
#1151
Conversation
cc @vyasr |
We may need to update some things on CI (like Edit: Looks like we need |
31d96b6
to
b75352e
Compare
cba7837
to
d1e4c3b
Compare
d1e4c3b
to
379e932
Compare
This looks great! I'm in favor of almost all of these changes. However, I would like to test this out with the wheel building before we merge anything just to be safe. Burn down is coming up fast and build infrastructure is fragile enough by nature that I'm wary of making changes that could hold up that effort. We're pretty close to stabilizing our logic for wheels so I think we should be able to test this with wheel builds within the next week or so. |
Happy to iterate on this PR until then though of course. |
Yeah the changes here should be useful for wheel building. Trying out the changes here would be welcome. Though likely still needs a couple fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome @jakirkham. It fixes a lot of minor problems in the rmm packaging that have been troublesome on occasion. I have a couple small requests. I also fixed the merge conflicts in the dependencies.yaml
file.
fb69a71
to
da35a74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jakirkham! Just a few suggestions with packaging, then this will be good to go.
Thanks Vyas! 🙏 Would it work with a symlink? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included a couple notes about other things that can move to pyproject.toml
potentially
My guess is that a symlink would work for most RAPIDS packages where the C++ is contained in a separate directory in the tree. In this instance I think you would end up with a circular symlink because the C++ is contained at the top-level directory (perhaps something that we should consider changing in rmm anyway). In particular, you need the CMakeLists.txt file contained at the root of the repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Vyas! 🙏
Had a couple small follow up points around packaging of text files
Ok think we should punt on this. Was mostly curious if there was an easy answer, but it seems this needs more thought. So yeah we can raise an issue and skip |
Co-authored-by: jakirkham <jakirkham@gmail.com>
Perhaps it is weird to review my PR. In any event, Vyas these changes look good to me. Thank you for working on them! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two non-blocking suggestions. Otherwise LGTM!
/merge |
Thanks all! 🙏 |
There were a couple of issues that accidentally made it through in #1151. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #1226
We need to update the version regex in update-version.sh for the pyproject.toml changes in #1151. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Ray Douglass (https://github.com/raydouglass) URL: #1227
Description
tomli
dependency for Python pre-3.11MANIFEST.in
setuptools
backendpyproject.toml
setup.cfg
content topyproject.toml
setup.py
to the minimal amount of logic necessaryChecklist