Skip to content

fix(pathfinder): remove dead/misleading error handling in platform loaders #2239

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.

Sign up for GitHub

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

Open
aryanputta wants to merge 2 commits into
base: main
Choose a base branch
from
+5 −9
Open

fix(pathfinder): remove dead/misleading error handling in platform loaders #2239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32596ce
fix(pathfinder): remove dead/misleading error handling in platform lo…
aryanputta Jun 21, 2026
c1ce967
fix(pathfinder): simplify loader review comments
aryanputta Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions cuda_pathfinder/cuda/pathfinder/_dynamic_libs/load_dl_linux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ def load_with_system_search(desc: LibDescriptor) -> LoadedDL | None:
Returns:
A LoadedDL object if successful, None if the library cannot be loaded

Raises:
RuntimeError: If the library is loaded but no expected symbol is found
"""
for soname in _candidate_sonames(desc):
try:
Expand All @@ -174,8 +172,7 @@ def load_with_system_search(desc: LibDescriptor) -> LoadedDL | None:
pass
else:
abs_path = abs_path_for_dynamic_library(desc.name, handle)
if abs_path is None:

@rwgk rwgk Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

            assert abs_path

will make the expectation explicit and obvious, without risking drift.

raise RuntimeError(f"No expected symbol for libname={desc.name!r}")
assert abs_path
return LoadedDL(abs_path, False, handle._handle, "system-search")
return None

Expand Down
9 changes: 4 additions & 5 deletions cuda_pathfinder/cuda/pathfinder/_dynamic_libs/load_dl_windows.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,10 @@ def add_dll_directory(dll_abs_path: str) -> None:
dirpath = os.path.dirname(dll_abs_path)
assert os.path.isdir(dirpath), dll_abs_path

# Add the DLL directory to the search path
result = kernel32.AddDllDirectory(dirpath)
if not result:
# Fallback: just update PATH if AddDllDirectory fails
pass
# Add the DLL directory to the native search path. AddDllDirectory only
# affects the LOAD_LIBRARY_SEARCH_USER_DIRS search; PATH is updated

@rwgk rwgk Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a remark: I'm not sure if there is actually any situation where PATH is still searched on Windows. Modern Python versions disabled that AFAIK, but I don't know the exact version details. I.e. the change here looks good as-is for the purpose of this PR.

# unconditionally below to also cover legacy dependent-DLL resolution.
kernel32.AddDllDirectory(dirpath)

# Update PATH as a fallback for dependent DLL resolution
curr_path = os.environ.get("PATH")
Expand Down
Loading