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 or drop the activation script that sets CUDA_PATH? #206

Closed
leofang opened this issue Jun 2, 2023 · 15 comments · Fixed by #252
Closed

Update or drop the activation script that sets CUDA_PATH? #206

leofang opened this issue Jun 2, 2023 · 15 comments · Fixed by #252

Comments

@leofang
Copy link
Member

leofang commented Jun 2, 2023

Currently we point CUDA_PATH to the conda env prefix

export CUDA_PATH=$CONDA_PREFIX

because we wanted to mimic a real CTK existing somewhere that's visible at run time to CuPy. The activation (deactivation) script is called when the conda env containing a CuPy package is activated (deactivated), during which CUDA_PATH is set (unset). However, this trick is only applicable to the "old" layout (pre-CUDA 12). For CUDA 12, this would likely not work very well. I think we'd need something like export CUDA_PATH=$CONDA_PREFIX/targets/${targetDir} at least, for the new layout.

Also, in the past I've occasionally heard complaints about us overwriting CUDA_PATH for users. It may prompt the consideration of dropping it all together. But unfortunately I do not have time to investigate the consequence of doing so.

cc: @jakirkham @kmaehashi

@function2-llx

This comment was marked as off-topic.

@leofang

This comment was marked as off-topic.

@leofang
Copy link
Member Author

leofang commented Nov 20, 2023

Possible options:

  1. Update the (de-)activate scripts for different layouts:
    a. CUDA 11: no change
    b. CUDA 12:
  2. Remove the (de-)activate scripts:
    a. This could be a breaking change, but TBH I prefer this; I no longer feel comfortable about meddling with users' env vars, which are globally visible and not just effective when using CuPy.

@jakirkham
Copy link
Member

If we drop the activation scripts, would it make sense to have some way to silence this CuPy warning about CUDA_PATH?

Can understand that warning in the general case. However for Conda packaging with a known layout, wonder if that warning is more confusing to users (and whether users setting it would cause hard to diagnose issues with Conda packages)

@leofang
Copy link
Member Author

leofang commented Nov 20, 2023

We can certainly do that. (We can patch it here for v12, and upstream the patch for v13.) @kmaehashi Do I miss anything?

@jakirkham
Copy link
Member

FWIW Numba uses this trick to check for a Conda environment. Maybe CuPy could leverage similar logic?

@leofang
Copy link
Member Author

leofang commented Nov 20, 2023

Oh we already have a detection mechanism via reading the json file shipped with the conda cupy package, so detecting whether inside a conda env is not a problem.

@leofang
Copy link
Member Author

leofang commented Nov 20, 2023

(btw why wouldn't Numba just check if the env var CONDA_PREFIX is set?)

@jakirkham
Copy link
Member

The logic was added in PR ( numba/numba#4096 ). Though don't see a mention of this consideration

If I had to guess, it is so that it will work even if the environment is not activated

@kmaehashi
Copy link
Member

kmaehashi commented Dec 25, 2023

If we drop the activation scripts, would it make sense to have some way to silence this CuPy warning about CUDA_PATH?

The warning in question is to notify users about (possibly) missing CUDA shared library installation on Windows. In conda environment this should not happen as dependencies are satisfied by the package manager. I think it's fine to disable this warning in conda environment. -> I've made a PR for this: #8051

Despite that, I guess it's better to avoid modifying CUDA_PATH in the activation scripts because CuPy uses $CUDA_PATH/bin/nvcc as the default nvcc path (e.g. used in RawModule(backend='nvcc'))

@mtjrider
Copy link

mtjrider commented Feb 1, 2024

@leofang encouraged me to comment here.
In multi-stage build pipelines where conda is used to manage dependencies in conjunction with other (build) tools, I have to reset CUDA_PATH because it is corrupted by package installation.

What is the primary reason CUDA_PATH is set by cupy?

@jakirkham
Copy link
Member

FWIW the warning referenced above was removed in CuPy 13 ( cupy/cupy#8076 )

Are there any other action items before making this change?

@mtjrider
Copy link

mtjrider commented Feb 2, 2024

@leofang encouraged me to comment here. In multi-stage build pipelines where conda is used to manage dependencies in conjunction with other (build) tools, I have to reset CUDA_PATH because it is corrupted by package installation.

What is the primary reason CUDA_PATH is set by cupy?

Not trying to be noisy but wanted to capture more detail.
If in the middle of a build, cupy is installed, and the associated environment is active, then CUDA_PATH needs to be reset for follow-up compilation where CUDA_PATH is used.

Likewise, if another library built with CFFI bindings (or similar) relies on the environment variable CUDA_PATH being set, then this library will be broken alongside cupy by default.

@leofang
Copy link
Member Author

leofang commented Feb 5, 2024

Thanks all, the new build has the activation scripts purged. @mtjrider would it be possible if you retry your workflow with the latest cupy from conda-forge?

@jakirkham
Copy link
Member

jakirkham commented Feb 5, 2024

Wonder if we should mark the previous CuPy 13.0.0 builds broken so that users only get packages without the activation script when installing CuPy 13.0.0

Edit: After offline discussion with @leofang and @mtjrider , we thought this would be a reasonable change. Filed issue ( #255 ) to track and document next steps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants