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

Update cugraph python build #2378

Merged
merged 97 commits into from
Jul 29, 2022

Conversation

jnke2016
Copy link
Contributor

@jnke2016 jnke2016 commented Jun 29, 2022

This PR updates cugraph python build to use scikit-build instead of setuptools.
Scikit-build leverages cmake to build the python extension

closes #2333

@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python labels Jun 29, 2022
@rlratzel rlratzel added this to the 22.08 milestone Jun 29, 2022
python/cugraph/cugraph/community/CMakeLists.txt Outdated Show resolved Hide resolved
python/cugraph/cugraph/internals/CMakeLists.txt Outdated Show resolved Hide resolved
python/cugraph/cugraph/utilities/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Please double-check the RPATH updates to make sure that you have enough .. everywhere. I think I got them all, but I may have missed one or two.

fetch_rapids.cmake Show resolved Hide resolved
python/cugraph/cugraph/dask/centrality/CMakeLists.txt Outdated Show resolved Hide resolved
python/cugraph/cugraph/dask/comms/CMakeLists.txt Outdated Show resolved Hide resolved
python/cugraph/cugraph/dask/community/CMakeLists.txt Outdated Show resolved Hide resolved
python/pylibcugraph/pyproject.toml Outdated Show resolved Hide resolved
@@ -94,46 +62,16 @@ def run(self):
os.system('rm -rf *.egg-info')
os.system('find . -name "*.cpp" -type f -delete')
os.system('find . -name "*.cpython*.so" -type f -delete')
os.system('rm -rf _skbuild')


cmdclass = dict()
cmdclass.update(versioneer.get_cmdclass())
cmdclass["build_ext"] = build_ext
cmdclass["clean"] = CleanCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions as for cugraph's setup.py. You don't need to set build_ext, I'm not 100% sure you need the custom CleanCommand, and you should be able to use versioneer.cmdclass directly without the update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure you need the custom CleanCommand

I think it is useful because whenever I run setup.py clean, it doesn't for instance remove _skbuild so I added it to the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, sure that's fine then.

@jnke2016
Copy link
Contributor Author

rerun tests

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @jnke2016!

@@ -94,46 +62,16 @@ def run(self):
os.system('rm -rf *.egg-info')
os.system('find . -name "*.cpp" -type f -delete')
os.system('find . -name "*.cpython*.so" -type f -delete')
os.system('rm -rf _skbuild')


cmdclass = dict()
cmdclass.update(versioneer.get_cmdclass())
cmdclass["build_ext"] = build_ext
cmdclass["clean"] = CleanCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, sure that's fine then.

@@ -245,3 +245,23 @@ def get_repo_cmake_info(names, file_path):
def _get_repo_path():
Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine then. I see the same logic in cuML, and cudf has some of the same functions directly in setup.py. I have previously wondered if in addition to the CMake code in rapids-cmake there might not also be a use case for a small standalone Python module (not even a package) of setup.py helpers that could be shared across RAPIDS libs. This is definitely making me reconsider that possibility, but it's out of scope for now. I'm good to leave this as is for the moment.

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a1ef1de into rapidsai:branch-22.08 Jul 29, 2022
@jnke2016 jnke2016 deleted the branch-22.08_scikit-build branch September 24, 2022 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Update cugraph python build(s) to use scikit-build
8 participants