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

Remove cuda/__init__.py in cuda-parallel package #3750

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Feb 7, 2025

Description

This PR removes cuda/__init__.py within the cuda_parallel project. Without this, attempting to import other packages within the cuda namespace package will fail, e.g., cuda.core or cuda.cccl.

Secondly, it updates the install instructions to clarify that we cannot install cuda_cccl in editable mode. This is because scikit-build-core does not correctly handle editable installs. In particular, it places the CCCL headers in site-packages, rather than to the project directory, even in editable mode. Thus, the headers will no longer be found in cuda_cccl.get_include_paths().

For similar reasons, -e cannot be used for cuda_parallel either.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@shwina shwina requested a review from a team as a code owner February 7, 2025 18:31
@shwina shwina requested a review from rwgk February 7, 2025 18:31
python/cuda_parallel/README.md Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Feb 7, 2025

🟩 CI finished in 29m 55s: Pass: 100%/1 | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
  • 🟩 python: Pass: 100%/1 | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s

    🟩 cpu
      🟩 amd64              Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 ctk
      🟩 12.8               Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 cudacxx
      🟩 nvcc12.8           Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 cudacxx_family
      🟩 nvcc               Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 cxx
      🟩 GCC13              Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 cxx_family
      🟩 GCC                Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 gpu
      🟩 rtx2080            Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    🟩 jobs
      🟩 Test               Pass: 100%/1   | Total: 29m 55s | Avg: 29m 55s | Max: 29m 55s
    

👃 Inspect Changes

Modifications in project?

Project
CCCL Infrastructure
libcu++
CUB
Thrust
CUDA Experimental
+/- python
CCCL C Parallel Library
Catch2Helper

Modifications in project or dependencies?

Project
CCCL Infrastructure
libcu++
CUB
Thrust
CUDA Experimental
+/- python
CCCL C Parallel Library
Catch2Helper

🏃‍ Runner counts (total jobs: 1)

# Runner
1 linux-amd64-gpu-rtx2080-latest-1

@rwgk
Copy link
Contributor

rwgk commented Feb 7, 2025

Wow, losing -e is a huge developer productivity loss. I worked hard (#3201) to make sure -e works reliably.

Weighing "losing -e" with (#3597 PR description)

The advantage of using scikit-build-core is that installation of headers is controlled by project's top-level "CMakeLists.txt"

my feeling is: It's better to revert #3597, to get back -e, and then look for an alternative solution.

@leofang for his opinion.

@leofang
Copy link
Member

leofang commented Feb 7, 2025

@vyasr can correct me but IIRC skbuild-core spent some effort enabling editable install:
https://scikit-build-core.readthedocs.io/en/stable/configuration.html#editable-installs
IIUC for setuptools-like in-tree behavior, we likely need --config-settings=editable.mode=inplace (I did not test). Is this not working?

@vyasr
Copy link
Contributor

vyasr commented Feb 7, 2025

It sounds like you are maybe running into scikit-build/scikit-build-core#807? We use editable installs regularly in RAPIDS with scikit-build-core, but there are definitely still some issues. In particular, my guess (just from reading the comments above, I haven't looked at any code) is that the problem is a combination of two things: 1) cuda_cccl is implemented without using the right importlib tooling, which is in general not a portable way of discovering files, but also 2) even if you did use the right importlib tooling SKBC has difficulty in editable mode due to the issue I linked above. I started some work to try and resolve that a while ago but I haven't been able to dedicate any time to it in months.

@shwina
Copy link
Contributor Author

shwina commented Feb 7, 2025

IIUC for setuptools-like in-tree behavior, we likely need --config-settings=editable.mode=inplace (I did not test). Is this not working?

@leofang Unfortunately, that command from the SKBC docs didn't quite work for me. I think we are running into scikit-build/scikit-build-core#807 as @vyasr said.

cuda_cccl is implemented without using the right importlib tooling, which is in general not a portable way of discovering files

@vyasr Just to clarify, we are using importlib.resources to export location information from cuda_cccl, so it is indeed the latter issue we are running into.

def get_include_paths() -> IncludePaths:

my feeling is: It's better to revert #3597, to get back -e, and then look for an alternative solution.

@rwgk I would not be opposed to this. I totally agree that editable installs are a huge productivity boost.

@oleksandr-pavlyk would you have any objections to reverting #3597?

@oleksandr-pavlyk
Copy link
Contributor

Ok, I'm ok with reverting it. I will revisit these changes to make editable install work as intended

@leofang
Copy link
Member

leofang commented Feb 7, 2025

Before we revert: What potential alternatives do people have in mind? It seems like we are saying skbuild-core and editable install are two mutually exclusive choices, until 807 is fixed for which we don't have an ETA. Am I missing something?

@rwgk
Copy link
Contributor

rwgk commented Feb 7, 2025

My understanding was:

  • scikit-build-core should work in principle, but there are hiccups around editable support.

  • Let's revert now so that we can continue developing with the -e productivity boost.

  • In a new PR, we can work on finding solutions to the hiccups, and then submit again with fixes, without losing -e.

@oleksandr-pavlyk
Copy link
Contributor

As far as I'm aware, scikit-build does not suffer from this problem, so we can use that until scikit-build-core can be properly adopted

@vyasr
Copy link
Contributor

vyasr commented Feb 7, 2025

The original scikit-build package indeed avoids this problem, but it does have different problems with editable installs. If you find those issues easier to deal with then switching back to that could make sense.

@shwina shwina changed the title Remove cuda/__init__.py and update README Remove cuda/__init__.py in cuda-parallel package Feb 8, 2025
@shwina
Copy link
Contributor Author

shwina commented Feb 8, 2025

I'm going to go ahead and merge this PR. We should follow up with a PR that either reverts #3597 or replaces scikit-build-core with scikit-build.

@shwina shwina merged commit 4a0cbfb into NVIDIA:main Feb 8, 2025
20 of 22 checks passed
shwina added a commit to shwina/cccl that referenced this pull request Feb 10, 2025
* Remove cuda/__init__.py and update README

* cuda_parallel editable installs are also broken

---------

Co-authored-by: Ashwin Srinath <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants