Fix Windows binary utility discovery on Arm64 - #2586
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test daf96fb |
|
PR 2586 second review: findings and recommendations (gpt-5.6.-sol)Review target:
Line references in this report refer to the reviewed head above. OutcomeThe refreshed branch addresses the earlier launcher-alias, search-composition,
There is also one low-priority documentation follow-up and one explicit Finding 1: High - native architecture detection is version-dependentLocation
Current behavior
raw_machine = platform.machine()The result controls these standalone paths:
The public docstring explicitly promises that x64 Python on an Arm64 machine Why Michael's experiment is real but does not close the issueMichael reported that
CPython 3.12 added a WMI query before the old environment-variable path. It This is not merely theoretical:
The current unit test at User impactWith x64 CPython 3.10/3.11 on an Arm64 Windows host, Recommended fixUse
The API's Implementation details for Codex:
Existing implementation that can be portedMichael's earlier dangling commit already contains a focused ctypes helper and git show c5033c6d7bb -- cuda_pathfinder/cuda/pathfinder/_utils/windows_arch.py
git show c5033c6d7bb -- cuda_pathfinder/tests/test_search_steps.pyPort the Required testsReplace the mocked
Every newly generated unit test must carry the repository's per-test Acceptance criteria
Primary and corroborating references
Finding 2: Low - corrupt registry data is silently treated as not installedLocation
Current behavior
An absent outer product key is the normal "product is not installed" case. The existing test covers only a valid registration. Recommended fixNarrow the normal-miss handling to the initial product-key open: try:
product_context = winreg.OpenKey(...)
except FileNotFoundError:
return None
with product_context as product_key:
try:
# Read CurrentVersion, open its subkey, and read the install root.
...
except FileNotFoundError as exc:
raise RuntimeError(
f"Incomplete Nsight {product!r} registry registration at {product_key_path!r}"
) from excKeep the existing type/empty-value validation. Let unrelated errors such as Add tests for:
Also update the public Documentation follow-up: Low - supported-platform text omits Windows Arm64Location
The installation guide says the supported Windows platform is only x86-64, If native Windows Arm64 is an officially supported runtime after this PR, Compatibility decision - terminal registry misses for
|
|
/ok to test 86dcf9c |
|
/ok to test f985d6d |
For this, there's no conda / pip packaging for nsys and ncu. So it makes no sense to search these two locations for nsight. |
Non-blocking follow-on refactoring ideaI do not think this needs to change in PR 2586. After this PR is merged, I would like to follow up with a semantics-neutral internal cleanup that extracts standalone Windows Nsight discovery from The proposed module would be: This name makes the ownership explicit without adding _REGISTRY_ROOT = ...
_NSYS_TARGET_DIR_BY_ARCH = ...
_NCU_TARGET_DIR_BY_ARCH = ...
def _installed_product_root(product: str) -> str | None: ...
def nsys_candidate_paths() -> Iterator[str]: ...
def ncu_candidate_paths() -> Iterator[str]: ...The generic finder would continue to own search orchestration, caching, executable validation, absolute-path normalization, and candidate resolution. Its Nsight call sites would become approximately: from cuda.pathfinder._binaries import windows_nsight
return _resolve_candidate_paths(windows_nsight.nsys_candidate_paths())
return _resolve_candidate_paths(windows_nsight.ncu_candidate_paths())Using lazy candidate iterators avoids a dependency from Other behavior to preserve:
Compute Sanitizer should remain in the generic finder for now. Its special path is a Windows CUDA Toolkit layout, not standalone Nsight product discovery, and the current helper is too small to justify another module. If additional Windows-specific CTK utility layouts appear later, they could motivate a separate For tests, the direct registry and Nsight-layout tests could move to This would leave |
|
/ok to test cf9cd66 |
rwgk
left a comment
There was a problem hiding this comment.
LGTM!
I added two small commits during the review:
- b0c2a77 corrects the documented CUDA environment-variable precedence to
CUDA_PATHbeforeCUDA_HOMEand renames the internal local fromcuda_hometocuda_path, avoiding the implication of the old precedence. - f876364 adds the user-visible Windows binary-utility discovery fix to the cuda-pathfinder 1.6.1 release notes.
One final documentation request: the PR description clearly states the WoA architecture-selection contract, and the individual API docstrings capture much of the reasoning, but the PR description does not yet bring the rationale together in one place. Could you please expand that part of the description to cover all three artifact types?
- Dynamic libraries select the Python interpreter architecture because they are loaded into that process and must match its ABI.
- Static-library discovery selects the Python interpreter architecture as its current target-selection policy/default. Since a static library is not itself loaded into Python, it would be helpful to state the intended consumer assumption explicitly rather than imply that process compatibility requires this choice.
- Architecture-specific executable layouts select the native Windows machine architecture because the utilities run out of process and therefore do not need to match Python. This also makes selection independent of which Python architecture is running and avoids unnecessarily choosing an emulated x64 executable on Windows Arm64.
That would make the rationale across the pathfinder components fully documented in one place.
Separately, I documented a possible semantics-neutral follow-on refactoring of the Windows Nsight discovery code here: #2586 (comment). I think it is worthwhile cleanup after this PR merges.
What changed
bin/compute-sanitizer.batlauncher, withcompute-sanitizer/compute-sanitizer.exeas the compatibility fallback.nsysto the native machine target only:target-windows-x64/nsys.exeon x64 ortarget-windows-armv8/nsys.exeon Arm64. There is no cross-architecture fallback.ncu.batlauncher, then fall back to the native machine binary:target/windows-desktop-win7-x64/ncu.exeon x64 ortarget/windows-desktop-win10-t23x-a64/ncu.exeon Arm64.nsight-sysandnsight-computeGUI launcher names; they are not aliases for thensysandncuCLIs.Binary layout survey
nvcc,nvdisasm,cuobjdump,nvprune,fatbinary,bin2c, andnvlinkremain atbin/<utility>.exein both CUDA 13.3 x64 and CUDA 13.4 x64/Arm64. The installed executable architecture changes with the CTK installer, but the path does not.nsight-sys.exeandnsight-compute.exeremain underbin. They launch the Nsight Systems and Nsight Compute GUIs and are distinct from thensysandncuCLIs.bin/compute-sanitizer.bat, which launchescompute-sanitizer/compute-sanitizer.exe.target-windows-x64andtarget-windows-armv8.nsysrequest.ncu.bat, with direct executables under architecture-specifictargetdirectories.ncurequest.The existing
IS_WINDOWSmacro is intentionally unchanged.Backward compatibility
nsight-sysandnsight-computecontinue to resolve their literal GUI launchers instead of being rewritten to the CLI names.Testing
pixi run --manifest-path cuda_pathfinder -e cu13 test: 1305 passed, 4 skipped.mypy-pathfinder: passed.pixi run --manifest-path cuda_pathfinder -e docs build-docs: passed with Sphinx warnings treated as errors.