Make Windows static library searches architecture-aware - #2491
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. |
isVoid
added
enhancement
ChatGPT Codex Connector
isVoid
changed the title
|
/ok to test f59381f |
|
lijinf2
left a comment
lijinf2
left a comment
There was a problem hiding this comment.
Looks pretty good! Minor comments about potential risk that I don't think would happen. And the solution is already the best under the limitation of mocking-based testing.
|
|
||
|
|
||
| def _cudadevrt_info() -> _StaticLibInfo: | ||
| if not IS_WINDOWS: |
There was a problem hiding this comment.
Might be risky if non-windows non-linux (RISC-V)? But yes or no I think this is good enough for now.
There was a problem hiding this comment.
Yes, definitely good for now. We have many other existing conditions like this that'd need revisiting.
| } | ||
|
|
||
| arch_dir = windows_python_arch() | ||
| component_wheel_dirs = ("nvidia/cuda_runtime/lib/x64",) if arch_dir == "x64" else () |
There was a problem hiding this comment.
I expect this hardcoded path nvidia/cuda_runtime/lib/x64 and the next path nvidia/cu13/lib/x64 will not be changed in CTK future releases. If the paths are changed, the mock test function would still pass.
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
LGTM, but could you please look into the codex test-gap finding below before you merge?
Medium - the Arm64 behavior is table-tested, but not exercised through the public locator or required by CI
References:
cuda_pathfinder/tests/test_find_static_lib.py:164.github/workflows/test-wheel-windows.yml:357
The new unit test monkeypatches IS_WINDOWS and windows_python_arch(), calls
the private _cudadevrt_info() helper, and compares its tuples. It never calls
locate_static_lib() or find_static_lib(). In particular,
_SUPPORTED_STATIC_LIBS_INFO was already constructed at import time, so the
public locator does not consume the configuration generated under the test's
monkeypatches.
The live CI result does not close this gap:
- All Windows jobs are
win-64; there is no native Arm64 job. - The tested toolkits are CUDA 13.3 or older, not the CUDA 13.4 layout that
introduces the public Windows Arm64 support relevant to this PR. - The workflow step named
all_must_worksets strictness for dynamic
libraries, headers, and bitcode, but omits
CUDA_PATHFINDER_TEST_FIND_NVIDIA_STATIC_LIB_STRICTNESS. A missing static
library therefore remainssee_what_worksand does not fail that step. - One inspected x64 wheel job did successfully resolve
nvidia\cu13\lib\x64\cudadevrt.lib, which is useful x64 evidence but says
nothing about the new Arm64 route.
I would add public-API tests that create synthetic Windows directory trees and
verify the selected file, not just the generated tuple. With the target-aware
API suggested above, the important cases are:
- An x64 Python/default request chooses x64.
- An explicit Arm64 target from the same simulated x64 process chooses Arm64.
- A toolkit containing both
lib/x64andlib/arm64returns the requested
target, including throughfind_static_lib(). - Arm64 never falls back to the CUDA 12 x64 component-wheel path or the legacy
unqualified CondaLibrary/libpath. - x64 continues to use both legacy fallbacks in the existing order.
- Cached x64 and Arm64 lookups remain distinct.
|
|
||
|
|
||
| def _cudadevrt_info() -> _StaticLibInfo: | ||
| if not IS_WINDOWS: |
There was a problem hiding this comment.
Yes, definitely good for now. We have many other existing conditions like this that'd need revisiting.
3 participants
What changed
Make Windows lookup for the currently supported static library,
cudadevrt, follow the Python interpreter architecture:lib/x64, retaining the x64-only CUDA 12 wheel and legacy Conda fallbacks.lib/arm64.Why
CUDA 13.4 ships
cudadevrt.libin separatelib/x64andlib/arm64locations. The existing lookup was hardcoded to x64, so Arm64 Python could not find its native library. The Arm64 and x64 CUDA 13.4 kitpick archives confirm the architecture-specific layout.Validation