Build cuda.core RDC test fixtures without Bash#2335
Conversation
rwgk
added
test
|
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 |
|
| return rdc_path.read_bytes() | ||
|
|
||
|
|
||
| def _subprocess_output(result: subprocess.CompletedProcess[str]) -> str: |
There was a problem hiding this comment.
Could we delete this function _subprocess_output and just default to system stdout/stderr behaviors?
There was a problem hiding this comment.
I would prefer to keep this helper. We intentionally capture stdout/stderr here because we need to inspect the nvcc output to distinguish known host-compiler setup failures from unexpected nvcc failures.
For unexpected failures, pytest.fail(...) includes the captured stdout/stderr in one self-contained failure message. That makes the CI/test log easier to interpret, especially when pytest output capture or parallel test output would otherwise split the context.
On success we still print the captured output back to stdout/stderr, so the normal diagnostic trail is preserved.
| # Keep these patterns narrow. Unknown nvcc failures should fail the test and | ||
| # expose their diagnostics, not be silently reclassified as environment skips. | ||
| windows_compiler_missing = ( | ||
| "cannot find compiler" in normalized and "cl.exe" in normalized and "in path" in normalized |
There was a problem hiding this comment.
Is "cannot find compiler" keyword stable enough and will not be modified in future release of nvcc logging?
I got the following from agent and it works on linux but not sure on windows.
for line in p.stdout.splitlines():
if line.startswith("#$"):
cmd = line[3:].strip().split()
if cmd:
return cmd[0] # gcc / g++ / clang++ / cl.exeThere was a problem hiding this comment.
Good question. I do not want to rely on this message being stable forever. The intent is almost the opposite: keep the recognized skip cases narrow.
If nvcc changes this diagnostic in a future release, this helper will stop recognizing it as a known environment setup issue. The test will then fail and include the full nvcc output, which gives us the evidence needed to decide whether to add another narrowly scoped skip pattern.
The #$ ... parsing approach can identify commands emitted by nvcc -v/dry-run style output, but it does not directly answer the question we need here: did the actual fixture build fail because the host compiler environment is missing, or did it fail for some other reason? For this test, it is safer to classify only the exact known failure modes after the real build attempt, and let everything else fail visibly.
| # expose their diagnostics, not be silently reclassified as environment skips. | ||
| windows_compiler_missing = ( | ||
| "cannot find compiler" in normalized and "cl.exe" in normalized and "in path" in normalized | ||
| ) |
There was a problem hiding this comment.
On windows is "cl.exe" the only host compiler available? What if nvcc is using another host compiler?
There was a problem hiding this comment.
For the path this PR exercises, yes, the Windows host compiler we expect is MSVC cl.exe. The fixture builder invokes bare nvcc and does not pass --compiler-bindir / -ccbin, so it is intentionally using the normal Windows nvcc setup.
If someone has a custom Windows nvcc host-compiler setup and it fails with a different diagnostic, this code will not silently skip it. It will fail the test and expose the actual output. That is intentional: the skip classification is only for the known default Windows setup failure where nvcc reports that it cannot find cl.exe in PATH.
In other words, this PR does intentionally not try to generalize all possible Windows host compiler configurations. It makes the supported test contract (see ctk-next PR 436)explicit: nvcc on PATH, with the normal host compiler environment already configured.
|
|
||
| _run( | ||
| [ | ||
| "nvcc", |
There was a problem hiding this comment.
Your previous PR uses NVCC environmental variable and pathfinder instead of bare nvcc command. Do we consider that here?
There was a problem hiding this comment.
Yes, this was considered. Now that the builder is Python instead of Bash, a full nvcc.exe path would be much less problematic than it was in the earlier Bash-based version.
I still prefer bare nvcc here because it keeps the test contract simple and explicit: the test environment should put the intended CUDA toolkit's bin directory on PATH, and the host compiler environment should already be configured. That is easy to satisfy in CI, runbooks, and local development.
Using pathfinder or an NVCC override inside this unit test would add a second toolkit-selection mechanism. That can create mismatches where the test discovers one nvcc, while CUDA_PATH, PATH, runtime libraries, or the surrounding runbook point at another toolkit. For this fixture build, relying on the environment's normal executable lookup is simpler and easier to reason about.
2 participants
This changes the
cuda.coreRDC test fixture build from a Bash script to a small Python driver aroundnvcc.Why this change:
nvcc, but they no longer require Bash. That avoids Windows-specific ambiguity between Git Bash, WSL Bash, path conversion behavior, and environment propagation.nvccdirectly from Python withsubprocess, using the platform's normalPATHlookup.nvccto be onPATHis straightforward, matches normal CUDA command-line expectations, and avoids passing a fullnvccpath through another shell layer.nvccbeing unavailable, the host compiler environment being unavailable, and othernvccfailures. Only the first two become skips; unexpectednvccfailures still fail the test.Suggested review path:
cuda_core/tests/test_binaries/build_test_binaries.pyfirst. It is the Python replacement for the deleted Bash script._build_saxpy_rdc()incuda_core/tests/test_module.pynext. That is where the local fallback behavior and skip/fail policy lives.nvcc --versionto aid troubleshooting and then the Python fixture builder.