Search conda environment before general system search in cuda-pathfinder #919
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…neral system search
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
kkraus14
commented
Aug 28, 2025
Comment on lines
+216
to
+226
| try: | ||
| handle = _load_lib(libname, soname) | ||
| except OSError: | ||
| pass | ||
| else: | ||
| # TODO KEITH: Do we need this abs_path_for_dynamic_library call? | ||
| # We're already resolving the absolute path based on the conda environment variables | ||
| abs_path = abs_path_for_dynamic_library(libname, handle) | ||
| if abs_path is None: | ||
| raise RuntimeError(f"No expected symbol for {libname=!r}") | ||
| return LoadedDL(abs_path, False, handle._handle) |
There was a problem hiding this comment.
This should probably be refactored into a helper function. I copy pasted it from load_with_system_search
| except OSError: | ||
| pass | ||
| else: | ||
| # TODO KEITH: Do we need this abs_path_for_dynamic_library call? |
There was a problem hiding this comment.
@rwgk was this only needed in the case we were relying on dlopen / windows equivalent to find the library for us?
Comment on lines
+163
to
+168
| handle = kernel32.LoadLibraryExW(dll_name, None, 0) | ||
| if handle: | ||
| # TODO KEITH: Do we need this abs_path_for_dynamic_library call? | ||
| # We're already resolving the absolute path based on the conda environment variables | ||
| abs_path = abs_path_for_dynamic_library(libname, handle) | ||
| return LoadedDL(abs_path, False, ctypes_handle_to_unsigned_int(handle)) |
There was a problem hiding this comment.
+1 we should probably refactor this into a helper function
| @@ -23,3 +23,8 @@ Highlights | |||
| - Improves stability in general and supports nvmath specifically | |||
| - Proactive change to improve library loading consistency | |||
| - Drops boilerplate docstrings for private functions | |||
|
|
|||
| * Add conda search path prioritization for ``load_nvidia_dynamic_lib()`` (`PR #856 <https://github.com/NVIDIA/cuda-python/pull/856>`_) | |||
There was a problem hiding this comment.
Suggested change
| * Add conda search path prioritization for ``load_nvidia_dynamic_lib()`` (`PR #856 <https://github.com/NVIDIA/cuda-python/pull/856>`_) | |
| * Add conda search path prioritization for ``load_nvidia_dynamic_lib()`` (`PR #919 <https://github.com/NVIDIA/cuda-python/pull/919>`_) |
|
/ok to test |
|
rwgk
reviewed
Aug 28, 2025
| in_conda_env = False | ||
| if os.getenv("CONDA_BUILD") == "1": | ||
| in_conda_build = True | ||
| elif os.getenv("CONDA_PREFIX"): |
There was a problem hiding this comment.
I spent a few minutes quizzing ChatGPT about the most robust way to figure out if we're in a conda environment. It produced this:
Typical code snippet looks like:
import os, sys
def in_conda_env() -> bool:
return os.path.exists(os.path.join(sys.prefix, "conda-meta"))
That is generally considered the most robust test.
There was a problem hiding this comment.
I had thought about this as well, but ultimately decided on using the $CONDA_PREFIX approach for a few reasons:
- Conda environment activation is guaranteed to set the environment variable and conda usage in general is dependent on it
- Aligned implementation between conda-build and general conda environments
- You can have a conda environment without
python, where technically you could have a pure C++ environment and someone could want to usecuda-pathfinderas a utility in a C++ build pipeline where Python would potentially be coming from outside of the conda environment, in which casesys.prefixwouldn't properly point inside of the activated conda environment.
leofang
added
enhancement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #839
Adds functions for searching in conda environments for libraries and uses the functions before doing the general system search. This should resolve the problem for when you have mismatched versions between a conda environment and the underlying system CTK and pathfinder picks up the system library.
There's still a lot of TODOs with some open questions in this PR and I didn't add any testing yet, but figured I should open this to drive some conversation.
Checklist