fix(pathfinder): remove dead/misleading error handling in platform loaders#2239
fix(pathfinder): remove dead/misleading error handling in platform loaders#2239aryanputta wants to merge 2 commits into
Conversation
…aders
Two dead error-handling paths in the dynamic-lib platform loaders:
- load_dl_linux.load_with_system_search guarded abs_path with
'if abs_path is None: raise RuntimeError("No expected symbol ...")'.
abs_path_for_dynamic_library never returns None (it returns a resolved
path or raises OSError), so the branch is unreachable and its message is
factually wrong (it concerns dlinfo path resolution, not symbols). Drop
the dead branch and let the descriptive OSError surface, matching the
deterministic-loader policy of not masking discovery/load failures.
- load_dl_windows.add_dll_directory had 'if not result: pass', a no-op; the
PATH update below already runs unconditionally. Remove the dead branch and
clarify the comment on why PATH is updated regardless.
No behavior change: both removed branches were unreachable or no-ops.
Signed-off-by: Aryan <aryansputta@gmail.com>
github-actions
Bot
added
the
cuda.pathfinder
|
Note for triage: I can't set labels/milestones as an external contributor. Intended metadata for |
| # abs_path_for_dynamic_library never returns None: it returns a | ||
| # resolved path or raises OSError. Let that error surface rather | ||
| # than masking it, consistent with the deterministic-loader policy. |
There was a problem hiding this comment.
This comment explains behavior elsewhere. It could easily drift. Please remove.
| # resolved path or raises OSError. Let that error surface rather | ||
| # than masking it, consistent with the deterministic-loader policy. | ||
| abs_path = abs_path_for_dynamic_library(desc.name, handle) | ||
| if abs_path is None: |
There was a problem hiding this comment.
assert abs_path
will make the expectation explicit and obvious, without risking drift.
| RuntimeError: If the library is loaded but no expected symbol is found | ||
| OSError: If the library is loaded but its absolute path cannot be | ||
| resolved via dlinfo (surfaced from abs_path_for_dynamic_library). |
There was a problem hiding this comment.
I'd rather delete the entire Raises section: this is documenting behavior elsewhere / risks drift. It's not a public API. agents will figure this out anyway, and might even get distracted by this part of the docstring.
| # 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 |
There was a problem hiding this comment.
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.
|
@rwgk I pushed |
2 participants
Summary
Removes two dead error-handling paths in the dynamic-lib platform loaders. Both are unreachable / no-ops today; the Linux one additionally carries a factually wrong message that would mislead anyone debugging a load failure. This aligns with the
cuda_pathfinderpolicy of not masking why discovery/loading failed.Details
load_dl_linux.load_with_system_searchguarded the resolved path with:abs_path_for_dynamic_librarynever returnsNone- it returns a resolved path (os.path.join(l_origin, basename)) or raisesOSErrorfromdlinfo. So the branch is unreachable, and its message is wrong: it talks about a missing symbol when the only real failure mode is dlinfo path resolution. Dropped the dead branch so the descriptiveOSErrorsurfaces directly, and updated the docstringRaises:accordingly.load_dl_windows.add_dll_directoryhad:The
if not result: passis a no-op - the PATH update below already runs unconditionally. Removed the dead branch and clarified why PATH is updated regardless.Behavior
No behavior change: both removed branches were unreachable or no-ops.
Testing
ruff checkandruff format --check(v0.15.9, repo-pinned) pass on both changed files.