Skip to content

Use pathlib in cuda.core build hooks, tests and examples (part 5 of #2410) - #2500

Open
LeSingh1 wants to merge 2 commits into
NVIDIA:mainfrom
LeSingh1:pathlib/cuda-core-hooks
Open

Use pathlib in cuda.core build hooks, tests and examples (part 5 of #2410)#2500
LeSingh1 wants to merge 2 commits into
NVIDIA:mainfrom
LeSingh1:pathlib/cuda-core-hooks

Conversation

@LeSingh1

Copy link
Copy Markdown
Contributor

Part 5 of #2410.

Replaces os.path with pathlib.Path in cuda_core/build_hooks.py, tests/helpers/__init__.py, the example test driver, and the two examples that assemble CUDA include paths.

ProgramOptions.include_path is typed str | list[str] | tuple[str], so the values handed to it stay str; only the path construction moves to pathlib. Same for the Extension arguments.

The one non-mechanical change is extension discovery, which previously sliced glob result strings against an os.path.sep-built prefix. It now relative-paths against Path("cuda", "core") and yields POSIX-style module names on every platform — which is what the old mod.replace(os.path.sep, "/") normalization already did. I checked that by re-implementing the old and new logic side by side and diffing them against the real source tree on Linux: identical module names, Extension names, source tuples and arch-specific sources across all 45 modules.

glob.glob is left as-is throughout; only the pattern construction moved. os.path.isdirPath.is_dir() was checked per call rather than bulk-replaced — each one guards a "does this include/lib dir exist" decision where both spellings return False for a broken symlink or a permission error.

Verified with cuda_core/tests/test_build_hooks.py on Linux CI: 14 passed, unchanged from the baseline. The test modules and examples need a GPU or a built cuda.bindings, so those changes are by inspection plus ruff and py_compile; they are strictly mechanical.

NOTE: developed with the assistance of an AI coding agent. I reviewed and verified the change before submitting.

@copy-pr-bot

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actionsgithub-actionsBot added the cuda.core Everything related to the cuda.core module label Aug 4, 2026

@mdboommdboom left a comment

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.

This generally looks good, but we should make ProgramOptions accept either Path or str (and coerce all incoming str to Path) to simplify this further.

samples_path = os.path.join(os.path.dirname(__file__), "..", "..", "examples")
sample_files = [os.path.basename(x) for x in glob.glob(samples_path + "**/*.py", recursive=True)]
samples_path = Path(__file__).parents[2] / "examples"
sample_files = [Path(x).name for x in glob.glob(f"{samples_path}**/*.py", recursive=True)]

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.

Why no samples_path.glob here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Switched to samples_path.glob("**/*.py"). Verified it yields the same 20 files as the old glob.glob(..., recursive=True) call, and every name still resolves under samples_path (the examples directory is flat, so keeping .name is safe).

include_path = [str(cuda_include)]
cccl_include = cuda_include / "cccl"
if cccl_include.is_dir():
include_path.insert(0, str(cccl_include))

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.

Let's expand what ProgramOptions can take so it also accepts a Path (in addition to str). That means we don't need to convert back to a str here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done. ProgramOptions now accepts str or any os.PathLike for the six path-valued options (include_path, pre_include, create_pch, use_pch, pch_dir, fdevice_time_trace) and normalizes them to Path in __post_init__, so the example passes cccl_include straight through with no str(). _prepare_nvrtc_options_impl accepts either form, and _program_cache/_keys.py was updated to match — otherwise include_path=Path(...) would have slipped past the guard that requires an extra_digest for options reading external files.

Two deliberate exclusions, both noted in the code:

  • name and time are not coerced. NVRTC treats them as filenames, but name is a plain label that gets encoded and inspected for a directory component, and time is forwarded to LinkerOptions.time (a bool) on the PTX path.
  • The empty string is not coerced. Path("") is Path("."), so it would turn --include-path= into --include-path=. and start searching the working directory. That is the only input where normalizing changes what the compiler does; the other normalizations (foo/ to foo, a//b to a/b) name the same directory. Test added.

@LeSingh1
LeSingh1force-pushed the pathlib/cuda-core-hooks branch from 49a2238 to 7ea6afeCompareAugust 7, 2026 23:14
@LeSingh1

Copy link
Copy Markdown
ContributorAuthor

Done — ProgramOptions now takes str or os.PathLike for every path-valued field (include_path, pre_include, create_pch, use_pch, pch_dir, fdevice_time_trace) and normalizes to Path in __post_init__, so the str(...) calls are gone from both examples and from tests/helpers. Non-path values (False, range(...)) pass through untouched so the "silently ignored at compile time" semantics the cache tests pin still hold.

Two widenings were needed to make that work, and one of them fixes a latent bug: _prepare_nvrtc_options_impl dispatched on isinstance(..., str), so a Path in include_path was neither str nor Sequence and got silently dropped. _option_is_set in the program cache had the same shape, where a Path would have stopped tripping the external-content guard.

Left alone: name stays str (NVRTC label, and the cache guard uses it as a directory component) and time stays as-is since _translate_program_options forwards it to LinkerOptions.time, a bool flag.

test_basic_examples.py uses samples_path.glob("**/*.py") now — same 20 files.

One behaviour change worth your attention rather than burying: an empty-string path value now normalizes to Path("."), so --pre-include= becomes --pre-include=.. I updated the stale comment in test_program_cache.py that spelled out the old flags. Say the word if you would rather empty strings pass through untouched.

Not run locally: the cuda_core suite and the Cython build need CUDA. I verified with py_compile, ruff, cython-lint, regenerated _program.pyi via toolshed/run_stubgen_pyx.py, mypy (4 pre-existing critical_section errors in untouched .pyi files, identical on a baseline worktree), and standalone harnesses over the coercion and cache-guard logic. CI is the gate.

@LeSingh1
LeSingh1force-pushed the pathlib/cuda-core-hooks branch from 7ea6afe to fb01304CompareAugust 8, 2026 22:52
Part of NVIDIA#2410. Replaces os.path with pathlib.Path in
cuda_core/build_hooks.py, tests/helpers, the example test driver and
the two examples that assemble CUDA include paths.
ProgramOptions now accepts os.PathLike for every path-valued option
(include_path, pre_include, create_pch, use_pch, pch_dir,
fdevice_time_trace) and normalizes what it is given to pathlib.Path, so
callers no longer have to convert back to str. Non-path values (False,
range(...), ...) are left untouched, preserving the existing
"silently ignored at compile time" behavior. name stays str (NVRTC uses
it as a label and the program cache inspects it for a directory
component); time stays str-or-bool because the same field is forwarded
to LinkerOptions.time, which is a flag.
@LeSingh1
LeSingh1force-pushed the pathlib/cuda-core-hooks branch from fb01304 to 2f96f8cCompareAugust 8, 2026 22:53
@LeSingh1

Copy link
Copy Markdown
ContributorAuthor

Rebased onto current main; the conflicts are resolved and this is mergeable again.

Three of them, all additive:

  • cuda_core/docs/source/release/1.2.0-notes.rst — kept both of main's new entries (cuda.core: accept ProgramOptions(name=None) #2517 and the ctypes host-callback check) alongside this PR's entry.
  • cuda_core/tests/helpers/__init__.pymain added import os, this PR converted the os.path.join/os.path.isdir calls it was there for. Dropped the now-unused import rather than carrying it.
  • cuda_core/build_hooks.pymain had already adopted Path(p) / "cuda" at the sys.path scan, so I took main's line over the equivalent Path(p, "cuda"). At all_include_dirs, main introduced a local cuda_path = _get_cuda_path(); the resolution reuses that local instead of calling _get_cuda_path() again, which would print the CUDA path a second time.

ruff check on the touched files is back to the same findings as main (5 pre-existing UP038s in _program_cache/_keys.py, untouched here) — one new I001 that the rebase introduced in tests/example_tests/test_basic_examples.py is fixed.

The only red check is the assignee/labels/milestone gate, which I cannot set as an external contributor.

…int-clean
Follow-up to mdboom's review.
_coerce_path_option now leaves the empty string alone. Path("") is Path("."),
so coercing it would turn --include-path= into --include-path=. and start
searching the working directory -- the one input where normalizing changes
what the compiler does. Every other normalization Path applies (a trailing
separator, a doubled separator) names the same directory. Covered by
test_program_options_empty_path_option_is_not_coerced.
The list/tuple branch now recurses through the same helper so its entries get
the identical treatment.
_keys.py uses str | os.PathLike rather than the tuple form: ruff's UP038 fires
on the tuple in .py files. The four other tuple-form isinstance calls in that
file are pre-existing and left alone.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.coreEverything related to the cuda.core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LeSingh1@mdboom