-
Notifications
You must be signed in to change notification settings - Fork 58
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, stop using versioneer to manage versions, update dependencies.yaml. #232
Migrate as much as possible to pyproject.toml, stop using versioneer to manage versions, update dependencies.yaml. #232
Conversation
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.
Changes generally look right to me. Made a couple of small suggestions but approving either way.
"Programming Language :: Python :: 3.9", | ||
], | ||
# Include the separately-compiled shared library | ||
extras_require={"test": ["pytest", "pytest-xdist"]}, | ||
packages=find_packages(exclude=["tests*"]), |
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.
I would reuse the find_packages
result for package_data
to guarantee they stay in sync.
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.
Yes, good call. Will do.
"Programming Language :: Python", | ||
"Programming Language :: Python :: 3.9", | ||
"Programming Language :: Python :: 3.10", | ||
], | ||
packages=find_packages(exclude=["tests*"]), | ||
include_package_data=True, |
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.
Not critical since kvikio doesn't have wheels, but scikit-build has bugs with include_package_data
so specifying package_data
explicitly (like in the non-legate setup.py) is safer. That said I don't see wheels happening for legate-kvikio anytime soon so it's mostly just to be safe.
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.
#233.
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.
I'll unresolve this thread -- I'd be happy to try and fix #233 in this PR.
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.
Sure, if you're here and it's straightforward.
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.
Awesome, thanks @bdice. I only have a minor question.
"Programming Language :: Python :: 3.9", | ||
"Programming Language :: Python :: 3.10", |
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.
I am wondering if it is best just to remove the Python :: <version>
lines? Are they used by anyone?
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.
They are used on pypi for classification (and presented to the user on the webpage), e.g. https://pypi.org/project/numpy/
@bdice, is this PR ready to be merged? |
/merge |
Thanks all! 🙏 |
Modernizes kvikio's Python build system to help with CUDA 12 migration. Some of these changes were made for other RAPIDS repos in separate PRs over the course of time but I am doing them all together in this PR to help kvikio catch up to the state of other repos.
Major changes:
pyproject.toml
for as much metadata as possible.Closes #194. Addresses some things mentioned in #186.