Search conda environment before general system search in cuda-pathfinder#919
Search conda environment before general system search in cuda-pathfinder#919kkraus14 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
…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. |
| 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?
| 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
| - 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.
| * 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
left a comment
rwgk
left a comment
There was a problem hiding this comment.
This is awesome, I was also thinking having an explicit step for conda will be best. I'm learning a lot from looking at your implementation.
Do you want me to take over, to make this more DRY, and then iterate on the details from there?
| 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
3 participants
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