Migrate cuda_pathfinder/_dynamic_libs search modules from os.path to pathlib - #2489
Migrate cuda_pathfinder/_dynamic_libs search modules from os.path to pathlib#2489LeSingh1 wants to merge 2 commits into
Conversation
os.path is soft-deprecated in favor of the type-safe pathlib API (NVIDIA#2410). Convert the two filesystem-search modules under _dynamic_libs so path construction, joining, and predicates go through Path/PurePath instead of os.path string manipulation, which makes it explicit which values are paths and which are the plain strings the search cascade accumulates for error reporting. Public compatibility is treated as a hard constraint: every entry point still accepts str, and every function that documents or returns str still returns str. Path is used strictly as the internal representation and converted back with str() at each return, so SearchPlatform, FindResult.abs_path, and the LoadedDL.abs_path that reaches users are all unchanged in type and value. Two sites are deliberately left on the old API rather than converted silently, each with a comment explaining why: - _find_lib_dir_using_anchor keeps os.path.normpath. pathlib has no equivalent because PurePath intentionally does not collapse "..", so converting would change the path reported for an anchor such as a CUDA_PATH containing "..". - _derive_ctk_root_windows keeps ntpath. ntpath.dirname slices its input and preserves the caller's separators, while PureWindowsPath rewrites them to backslashes. derive_ctk_root also reaches this function on Linux, so converting would return an unusable root for a POSIX path whose parent directory is named "bin". glob.glob is also kept where wildcard expansion is needed; Path.glob differs in result ordering, and the reverse-sorted newest-first policy from NVIDIA#1732 depends on the current string sort. No behavior change is intended. The only string-level difference is that redundant "." components in an input directory are now collapsed, which cannot occur for the values these functions actually receive. Signed-off-by: LeSingh1 <sshaurya914@gmail.com>
github-actions
Bot
added
the
cuda.pathfinder
|
@mdboom — this is the part 1 I offered on #2410, sent as a small sample so the conversion style and the compatibility approach can be checked before the remaining six parts follow the same template. Happy to hold the rest until you've had a look, or to drop this entirely if you'd rather assign it elsewhere. Two notes for whoever picks it up:
On verification: I don't have CUDA hardware, so I ran the suite on Linux CI rather than locally. Rather than just matching the pass count, I diffed the full pytest output against |
|
A few implementation comments:
This only needs to apply to functions in the public API. We are free to change the implementation details of internal function calls as we see fit, so we should prefer
All uses of |
Follow-up to review feedback on NVIDIA#2489. Narrow the str-compatibility constraint to the public API. The internal helpers and the SearchPlatform protocol now pass and return Path; str() is applied only where the value leaves this module: - FindResult.abs_path, which reaches users unchanged as LoadedDL.abs_path - derive_ctk_root(), whose result is consumed by load_nvidia_dynamic_lib and the header search Replace all four glob.glob() call sites with Path.glob(). Sorting is done on the string form via a small sorted_glob() helper: PurePath ordering is case-insensitive on Windows, so plain sorted() over Path objects would reorder mixed-case filenames relative to the byte-wise ordering these call sites have always used, and the newest-first policy from NVIDIA#1732 rides on that ordering. Two sites deliberately stay on os.path: - _find_lib_dir_using_anchor() keeps os.path.normpath(): PurePath does not collapse "..", and Path.resolve() would additionally follow symlinks. - _derive_ctk_root_windows() keeps ntpath. derive_ctk_root() also reaches it on Linux, where PureWindowsPath would rewrite "/opt/foo/bin/libx.so" to "\opt\foo" instead of "/opt/foo". Differential fuzzing against the previous revision (400k randomized path shapes, 600 randomized directory trees) shows one intended change: LinuxSearchPlatform.conda_anchor_point() now returns a normalized Path rather than $CONDA_PREFIX verbatim. That is not observable downstream, because _find_lib_dir_using_anchor() already normpath'd its result; resolved paths, error messages and listdir attachments are byte-identical. Signed-off-by: LeSingh1 <sshaurya914@gmail.com>
|
Done in 5d5f21a. Internals and the One thing worth flagging:
A correction to my earlier comment. I said the CI baseline was "1188 passed and 88 Differential fuzzing against the previous revision (400k randomized path shapes, 600 randomized directory trees) shows one intended difference: |
Follow-up to the review feedback on NVIDIA#2489: the str-compatibility constraint applies only to the public API. The try_* methods and _no_such_file_in_dir now work in Path throughout. str() is applied once, where abs_path is stored on the public LocatedStaticLib and LocatedBitcodeLib. The relative-path constants go from os.path.join(...) to forward-slash literals, matching how site_packages_dirs is already written in the same dicts; Path normalizes the separator on Windows. One behavior change: a CUDA_PATH or CONDA_PREFIX containing redundant separators ("//", "/.") now produces a normalized abs_path, because Path collapses them. Differential fuzzing against the pre-revision code (16k lookups over randomized trees, comparing located paths and full error text) shows no other difference, and none at all when those variables are free of redundant separators. Signed-off-by: LeSingh1 <sshaurya914@gmail.com>
| ``abs_path`` is a ``str``: it is handed to the platform loaders and surfaces | ||
| unchanged as the public ``LoadedDL.abs_path``. Everything upstream of this | ||
| dataclass works with ``Path``. |
|
|
||
| Returns a ``str`` because the result crosses into ``load_nvidia_dynamic_lib`` | ||
| and the header search, which are outside this module. |
| abs_path: str | ||
| found_via: str |
There was a problem hiding this comment.
Let's store these as Path with a custom field converter. That way all users of this class don't have to convert their paths back to strings and back again.
| abs_path: str | |
| found_via: str | |
| abs_path: Path = field(converter=Path) | |
| found_via: str |
| if abs_path is None: | ||
| return None | ||
| return FindResult(abs_path, "system-ctk-root") | ||
| return FindResult(str(abs_path), "system-ctk-root") |
There was a problem hiding this comment.
| return FindResult(str(abs_path), "system-ctk-root") | |
| return FindResult(abs_path, "system-ctk-root") |
| abs_path = ctx.platform.find_in_site_packages(rel_dirs, ctx.lib_searched_for, ctx.error_messages, ctx.attachments) | ||
| if abs_path is not None: | ||
| return FindResult(abs_path, "site-packages") | ||
| return FindResult(str(abs_path), "site-packages") |
There was a problem hiding this comment.
| return FindResult(str(abs_path), "site-packages") | |
| return FindResult(abs_path, "site-packages") |
| abs_path = _find_using_lib_dir(ctx, lib_dir) | ||
| if abs_path is not None: | ||
| return FindResult(abs_path, "conda") | ||
| return FindResult(str(abs_path), "conda") |
There was a problem hiding this comment.
| return FindResult(str(abs_path), "conda") | |
| return FindResult(abs_path, "conda") |
| abs_path = _find_using_lib_dir(ctx, lib_dir) | ||
| if abs_path is not None: | ||
| return FindResult(abs_path, "CUDA_PATH") | ||
| return FindResult(str(abs_path), "CUDA_PATH") |
There was a problem hiding this comment.
| return FindResult(str(abs_path), "CUDA_PATH") | |
| return FindResult(abs_path, "CUDA_PATH") |
| result = _find_lib_dir_using_anchor(desc, LinuxSearchPlatform(), tmp_path) | ||
| assert result is not None | ||
| assert result.endswith(os.path.join("nvvm", "lib64")) | ||
| assert str(result).endswith(os.path.join("nvvm", "lib64")) |
There was a problem hiding this comment.
Let's also update the uses of os.path in test files.
| def sorted_glob(directory: Path, pattern: str, *, reverse: bool = False) -> list[Path]: | ||
| """Return ``directory.glob(pattern)`` matches in a deterministic order. | ||
|
|
||
| The ordering is deliberately taken from the string form rather than from | ||
| ``Path`` comparison: ``PurePath`` ordering is case-insensitive on Windows, | ||
| so plain ``sorted()`` over ``Path`` objects would reorder mixed-case | ||
| filenames relative to the byte-wise ordering used up to now. Issue #1732 | ||
| tracks the newest-first policy that rides on this ordering, so it is kept | ||
| unchanged on both platforms. | ||
| """ | ||
| return sorted(directory.glob(pattern), key=str, reverse=reverse) | ||
|
|
||
|
|
||
| def _sorted_dir_entry_names(directory: Path) -> list[str]: | ||
| return sorted(entry.name for entry in directory.iterdir()) | ||
|
|
||
|
|
There was a problem hiding this comment.
One thing worth flagging: sorted() over Path compares PurePath._str_normcase, which is lowercased on Windows, so it isn't the same order as sorted() over str for mixed-case names. I kept the existing order by sorting on the string form in a small sorted_glob() helper, rather than change Windows behaviour in a PR that otherwise changes nothing — and I have no Windows machine to check it on. Happy to drop the key for simpler code if you'd prefer.
Yes, let's switch to the simpler code. I would consider this a fix of a latest Windows bug because it makes sorting behave the same way as it normally does on that platform.
So let's remove these helpers, take the change in Windows sorting behavior as desired (one of the reasons to move to Path is to get these sorts of bugfixes from upstream Python), and update all uses of these helpers to the simpler form.
Part 4 of the series proposed in #2410. Filesystem predicates and path joining in the pathfinder tests now go through pathlib: os.path.isfile/isdir become Path.is_file()/is_dir(), os.path.basename becomes Path.name, os.path.join becomes Path joining, and the site-packages check uses Path.parts instead of splitting on os.path.sep. site_pkg_rel.replace("/", os.sep) is dropped in test_find_static_lib.py: Path already accepts forward slashes on Windows. Two files are left out on purpose. test_find_nvidia_binaries.py moves with part 3, whose signature changes it depends on. test_search_steps.py is being edited by #2489 (part 1), so converting it here would only create a conflict. Left on the stdlib modules: glob.glob in test_find_nvidia_headers.py, which expands an absolute pattern from the header catalog (Path.glob needs a base dir, and the wildcard is not pinned to the last component); os.pathsep in test_ctk_root_discovery.py, which builds PYTHONPATH, not a path; and os.sep in test_utils_env_vars.py, which builds a trailing separator on purpose. Signed-off-by: LeSingh1 <sshaurya914@gmail.com>
2 participants
Part 1 of the series proposed in #2410:
cuda_pathfinder/cuda/pathfinder/_dynamic_libs/—search_platform.pyandsearch_steps.py. Deliberately small so the conversion style and the compatibility approach can be reviewed before the remaining six parts follow the same template.What changed
Path construction, joining, and filesystem predicates now go through
Path/PurePathinstead ofos.pathstring manipulation.search_platform.pyno longer importsosat all.Compatibility
Treated as a hard constraint, per the discussion on the issue. Every entry point still accepts
str, and every function that documents or returnsstrstill returnsstr—Pathis used strictly as the internal representation and converted back withstr()at each return.SearchPlatform,FindResult.abs_path, and theLoadedDL.abs_paththat reaches users are unchanged in both type and value. No signature changes.Deliberately not converted
Rather than folding these in silently, both are left alone with a comment saying why:
_find_lib_dir_using_anchorkeepsos.path.normpath. pathlib has no equivalent becausePurePathintentionally does not collapse.., so converting would change the path reported for an anchor such as aCUDA_PATHcontaining..._derive_ctk_root_windowskeepsntpath.ntpath.dirnameslices its input and preserves the caller's separators;PureWindowsPathrewrites them to backslashes.derive_ctk_rootalso reaches this function on Linux, so converting would return an unusable root (\opt\foo) for a POSIX path whose parent directory happens to be namedbin.glob.globis also retained where wildcard expansion is needed —Path.globorders results differently, and the reverse-sorted newest-first policy from #1732 depends on the current string sort.Happy to revisit any of these if you'd prefer them handled differently; that decision will set the pattern for the rest of the series.
Verification
No behavior change intended. Beyond the test suite, I fuzzed the old and new implementations against each other: 200k randomized path shapes for the
derive_ctk_roothelpers, and 3000 randomized real directory trees (including trailing-slash,//and/./spellings) for the search helpers. The only string-level divergence is that a redundant.component in an input directory is now collapsed — the new string is the normalized form of the old and names the same file — and that input cannot occur, sincelib_diralways arrives alreadynormpath-ed.pre-commit runpasses every hook at the pinned versions;mypyis clean overcuda_pathfinder/cuda.Since I don't have CUDA hardware, I verified the suite on Linux CI rather than locally. Against
upstream/mainthe full pytest output is byte-identical apart from elapsed time — same 1188 passed, 4 skipped, and the same 88Not founderrors for absent NVIDIA libraries, in the same order.