-
Notifications
You must be signed in to change notification settings - Fork 314
Add LoadedDL.found_via
#1049
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
Add LoadedDL.found_via
#1049
Changes from 4 commits
599ef39
b5d53c0
8746a22
bf44cdf
95d913c
9c5a059
7557d50
711179f
96574e5
936cd20
4f1abfd
a969731
8df1369
f7994fc
d0b9d65
e02bf25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
| import os | ||
| from typing import Optional, cast | ||
|
|
||
| from cuda.pathfinder._dynamic_libs.load_dl_common import LoadedDL | ||
| from cuda.pathfinder._dynamic_libs.load_dl_common import LoadedDL, Distribution | ||
| from cuda.pathfinder._dynamic_libs.supported_nvidia_libs import ( | ||
| LIBNAMES_REQUIRING_RTLD_DEEPBIND, | ||
| SUPPORTED_LINUX_SONAMES, | ||
|
|
@@ -170,9 +170,21 @@ def load_with_system_search(libname: str) -> Optional[LoadedDL]: | |
| 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) | ||
| return LoadedDL(abs_path, False, handle._handle, Distribution("system", None)) | ||
| return None | ||
|
|
||
| def find_with_system_search_linux(libname: str) -> Optional[str]: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things stand out here:
To maintain that the code reflects actual (inconvenient) realities, it'd be great to avoid adding the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we switch to a version of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately not, beyond the subprocess idea that's tracked already under #757. To double-check, I just asked ChatGPT 5 Pro (which didn't exist last time I was working on it): This was my prompt: I believe I asked before, but could you please give this another very thorough look? Platform: Linux only (please ignore any other platforms) Python versions: 3.13 (please ignore any older Python versions) Consider this example code: On my workstation it prints My question: Is there any way to implement
It reasoned for 15 minutes! For the full answer, please see https://chatgpt.com/share/68dc09d0-c928-8008-9a55-a5aede0dbbf8 Bottom line
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To track some pointers here: When asking ChatGPT about a brute-force idea, surprisingly suggested a new approach: The interesting part from there: A practical, non‑subprocess alternative (doesn’t load the target) If you’re willing to slightly relax “no reverse‑engineering,” there is a clean way that doesn’t load the library and doesn’t guess the search order: Ask the dynamic linker for the exact directory search list it would use for a On glibc, this is what I then asked it to generate the code, which it did in a couple minutes: But running that code, it turns out:
The only non-reverse-engineering way to get at the binary cache is to run I then asked it about the equivalent on Windows, for which it also quickly generated code, but there are also caveats. I decided to stop here:
|
||
| for soname in get_candidate_sonames(libname): | ||
| try: | ||
| handle = _load_lib(libname, soname) | ||
| except OSError: | ||
| pass | ||
| else: | ||
| abs_path = abs_path_for_dynamic_library(libname, handle) | ||
| if abs_path is None: | ||
| raise RuntimeError(f"No expected symbol for {libname=!r}") | ||
| return abs_path | ||
| return None | ||
|
|
||
| def _work_around_known_bugs(libname: str, found_path: str) -> None: | ||
| if libname == "nvrtc": | ||
|
|
@@ -194,21 +206,11 @@ def _work_around_known_bugs(libname: str, found_path: str) -> None: | |
|
|
||
|
|
||
| def load_with_abs_path(libname: str, found_path: str) -> LoadedDL: | ||
| """Load a dynamic library from the given path. | ||
|
|
||
| Args: | ||
| libname: The name of the library to load | ||
| found_path: The absolute path to the library file | ||
|
|
||
| Returns: | ||
| A LoadedDL object representing the loaded library | ||
|
|
||
| Raises: | ||
| RuntimeError: If the library cannot be loaded | ||
| """ | ||
| _work_around_known_bugs(libname, found_path) | ||
| try: | ||
| handle = _load_lib(libname, found_path) | ||
| except OSError as e: | ||
| raise RuntimeError(f"Failed to dlopen {found_path}: {e}") from e | ||
| return LoadedDL(found_path, False, handle._handle) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,51 +6,27 @@ | |
| import sys | ||
|
|
||
| from cuda.pathfinder._dynamic_libs.find_nvidia_dynamic_lib import _FindNvidiaDynamicLib | ||
| from cuda.pathfinder._dynamic_libs.load_dl_common import LoadedDL, load_dependencies | ||
| from cuda.pathfinder._dynamic_libs.load_dl_common import Distribution, LoadedDL, load_dependencies | ||
| from cuda.pathfinder._utils.platform_aware import IS_WINDOWS | ||
|
|
||
| if IS_WINDOWS: | ||
| from cuda.pathfinder._dynamic_libs.load_dl_windows import ( | ||
| check_if_already_loaded_from_elsewhere, | ||
| load_with_abs_path, | ||
| load_with_system_search, | ||
| ) | ||
| else: | ||
| from cuda.pathfinder._dynamic_libs.load_dl_linux import ( | ||
| check_if_already_loaded_from_elsewhere, | ||
| load_with_abs_path, | ||
| load_with_system_search, | ||
| ) | ||
|
|
||
| from typing import Callable, Optional | ||
|
|
||
| def _load_lib_no_cache(libname: str) -> LoadedDL: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect it'll be simpler (less code) to wrap this function in a helper that adds Alternatively, we could pass |
||
| finder = _FindNvidiaDynamicLib(libname) | ||
| abs_path = finder.try_site_packages() | ||
| if abs_path is None: | ||
| abs_path = finder.try_with_conda_prefix() | ||
|
|
||
| # If the library was already loaded by someone else, reproduce any OS-specific | ||
| # side-effects we would have applied on a direct absolute-path load (e.g., | ||
| # AddDllDirectory on Windows for libs that require it). | ||
| loaded = check_if_already_loaded_from_elsewhere(libname, abs_path is not None) | ||
|
|
||
| # Load dependencies regardless of who loaded the primary lib first. | ||
| # Doing this *after* the side-effect ensures dependencies resolve consistently | ||
| # relative to the actually loaded location. | ||
| load_dependencies(libname, load_nvidia_dynamic_lib) | ||
|
|
||
| if loaded is not None: | ||
| return loaded | ||
|
|
||
| if abs_path is None: | ||
| loaded = load_with_system_search(libname) | ||
| if loaded is not None: | ||
| return loaded | ||
| abs_path = finder.try_with_cuda_home() | ||
| if abs_path is None: | ||
| finder.raise_not_found_error() | ||
|
|
||
| return load_with_abs_path(libname, abs_path) | ||
| def _load_lib_no_cache(libname: str) -> LoadedDL: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to move this below the Also, this could be a one-liner: |
||
| loader = _LoadNvidiaDynamicLib(libname) | ||
| dl = loader.load_lib() | ||
| return dl | ||
|
|
||
|
|
||
| @functools.cache | ||
|
|
@@ -130,3 +106,75 @@ def load_nvidia_dynamic_lib(libname: str) -> LoadedDL: | |
| f" {sys.version_info.major}.{sys.version_info.minor}" | ||
| ) | ||
| return _load_lib_no_cache(libname) | ||
|
|
||
|
|
||
| class _LoadNvidiaDynamicLib: | ||
| def __init__(self, libname: str): | ||
| self.finder = _FindNvidiaDynamicLib(libname) | ||
| self.libname = self.finder.libname | ||
|
|
||
| def _load_with_dependencies( | ||
| self, get_path_func: Callable[[], Optional[LoadedDL]], dist_name: str | ||
| ) -> Optional[LoadedDL]: | ||
| # If the library was already loaded by someone else, reproduce any OS-specific | ||
| # side-effects we would have applied on a direct absolute-path load (e.g., | ||
| # AddDllDirectory on Windows for libs that require it). | ||
| loaded = check_if_already_loaded_from_elsewhere(self.libname, True) | ||
|
|
||
| # Load dependencies regardless of who loaded the primary lib first. | ||
| # Doing this *after* the side-effect ensures dependencies resolve consistently | ||
| # relative to the actually loaded location. | ||
| load_dependencies(self.libname, load_nvidia_dynamic_lib) | ||
|
|
||
| abs_path = get_path_func() | ||
| if abs_path is None: | ||
| return None | ||
|
|
||
| dist = Distribution(name=dist_name, version="unknown") | ||
| if loaded is not None: | ||
| loaded.distribution = dist | ||
| return loaded | ||
|
|
||
| dl = load_with_abs_path(self.libname, abs_path) | ||
| dl.distribution = dist | ||
| return dl | ||
|
|
||
| def _load_simple(self, get_path_func: Callable[[], Optional[LoadedDL]], dist_name: str) -> Optional[LoadedDL]: | ||
| abs_path = get_path_func() | ||
| if abs_path is None: | ||
| return None | ||
|
|
||
| dl = load_with_abs_path(self.libname, abs_path) | ||
| dl.distribution = Distribution(name=dist_name, version="unknown") | ||
| return dl | ||
|
|
||
| def load_with_site_packages(self) -> Optional[LoadedDL]: | ||
| return self._load_simple(self.finder.try_site_packages, "site-packages") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this changes the behavior in a subtle but deep way: loading of dependencies should happen in all cases. The interactions are tricky, but that's just a consequence of the realities. The existing order
is very intentional. See the existing comments in existing lines 39-39. I guess we could change the order to
but I didn't do that because there could be exceptions in the "find specific lib" step, and we'd have the side-effects from loading the dependencies already. It's not clear-cut if that's better or worse; I decided side-effects later is probably better. I'm thinking it'd be better to be more conservative with the code organization when adding in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. lets unpack the four ways of loading things back out into their own methods then. Updates to follow. |
||
|
|
||
| def load_with_conda_prefix(self) -> Optional[LoadedDL]: | ||
| return self._load_simple(self.finder.try_with_conda_prefix, "conda") | ||
|
|
||
| def load_with_system_search(self) -> Optional[LoadedDL]: | ||
| return self._load_with_dependencies(self.finder.try_with_system_search, "system") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, if we say Two ideas to explain what I have in mind, for discussion:
The aha part is way more tricky. Is there any value to it, for the purposes of numba-cuda?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's enough for |
||
|
|
||
| def load_with_cuda_home(self) -> Optional[LoadedDL]: | ||
| return self._load_with_dependencies(self.finder.try_with_cuda_home, "CUDA_HOME") | ||
|
|
||
| def load_lib(self) -> LoadedDL: | ||
| dl = self.load_with_site_packages() | ||
| if dl is not None: | ||
| return dl | ||
|
|
||
| dl = self.load_with_conda_prefix() | ||
| if dl is not None: | ||
| return dl | ||
|
|
||
| dl = self.load_with_system_search() | ||
| if dl is not None: | ||
| return dl | ||
|
|
||
| dl = self.load_with_cuda_home() | ||
| if dl is not None: | ||
| return dl | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the unifying group of four methods, you could use a loop like this: if loaded := self._maybe_return_loaded(self.finder.abs_path is not None):
return loaded
for load_meth, found_via in (
(self.load_with_site_packages, "site-packages"),
(self.load_with_conda_prefix, "conda"),
(self.load_with_system_search, "system-search"),
(self.load_with_cuda_home, "CUDA_HOME"),
):
if loaded := load_meth():
loaded.foundvia = FoundVia(found_via)
return loaded |
||
|
|
||
| self.finder.raise_not_found_error() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not used anymore?