Repository navigation
[no-ci] Fix Windows coverage test stack overflows - #3342
Conversation
Raise the GPU runner's base Python executable stack reserve to at least 8 MiB using a standard-library PE-header helper. Write a patched copy and replace the executable after Python exits, avoiding the need for editbin on the GPU runner. Preserve the existing editbin build-job step. run_pytest_with_stack.py enlarges only the worker thread running pytest. Samples, subprocess tests, and multiprocessing workers start fresh Python interpreters with the executable's original stack reserve, so they still overflow. The wrapper also leaves Python's main thread waiting for pytest, which can delay deferred cleanup scheduled through Py_AddPendingCall. Run pytest directly on the main thread with the patched interpreter and remove the worker-thread wrapper. Keep continue-on-error enabled. Validation: 22 tests passed; Ruff, formatting, and workflow syntax checks passed. Verified patched PE32 and PE32+ executables with objdump. Windows GPU CI confirmation remains pending.
|
No actionable comments were generated in the recent review. Summary
Change: Bug fix · Severity of issue fixed: Medium Merge Risk:
 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Ralf: whoa ... that's quite an involved workaround. Did you already ask the runner team about pre-installing MSVC tools?
But LGTM, except the CR finding.
GPT-6.1-Sol ultra:
- P1 — Strip CR before converting the interpreter path (coverage.yml:393). Windows Python emits CRLF; Bash command substitution removes only LF, leaving
python.exe\r. The patcher receives an invalid source path, stopping Windows coverage before tests run. Replaceprint(sys._base_executable)withsys.stdout.write(sys._base_executable). This confirms CodeRabbit’s existing finding. (CPython source)
No additional findings. All 22 helper tests pass on Linux; native Windows GPU validation remains pending.
|
@rwgk it doesn't make sense to have msvc on gpu runners, that would be a lot of work. |
|
@rwgk @jpascucci-nv Following up on Ralf's
I'd also move this into # .github/actions/setup-hardened-python/action.yml
inputs:
stack-reserve:
description: >-
Windows only. Raise the base interpreter's main-thread stack reserve to at
least this many bytes. Empty (default) leaves the interpreter unchanged.
default: ""New last step of the action, after the ssl check and pip update, so - name: Raise the interpreter's stack reserve
if: runner.os == 'Windows' && inputs.stack-reserve != ''
shell: bash --noprofile --norc -xeuo pipefail {0}
env:
PYTHON: ${{ steps.venv.outputs.python-path }}
STACK_RESERVE: ${{ inputs.stack-reserve }}
run: |
# sys.stdout.write, not print: Windows Python ends lines with CRLF.
base="$("${PYTHON}" -c 'import sys; sys.stdout.write(sys._base_executable)')"
# Windows can't overwrite a running executable: write a patched copy,
# then replace the original after the patching interpreter has exited.
uv run --no-project --isolated --with pefile==2024.8.26 --python "${base}" python - "${base}" "${STACK_RESERVE}" <<'EOF'
import sys, pefile
path, reserve = sys.argv[1], int(sys.argv[2])
pe = pefile.PE(path)
pe.OPTIONAL_HEADER.SizeOfStackReserve = max(pe.OPTIONAL_HEADER.SizeOfStackReserve, reserve)
pe.write(path + ".stack-reserve")
EOF
mv -f -- "${base}.stack-reserve" "${base}"
# Check the effect on the real binary, not just the header bytes.
"${base}" - "${STACK_RESERVE}" <<'EOF'
import ctypes, sys
lo, hi = ctypes.c_size_t(), ctypes.c_size_t()
ctypes.windll.kernel32.GetCurrentThreadStackLimits(ctypes.byref(lo), ctypes.byref(hi))
print("main-thread stack reserve:", hi.value - lo.value)
sys.exit(hi.value - lo.value < int(sys.argv[1]))
EOFIn Why this shape:
On tests: with |
juenglin
left a comment
juenglin
left a comment
There was a problem hiding this comment.
Let's try to simplify this.
|
@juenglin Implemented your suggestions. |
|
Dispatched a coverage run on |
|
I had started a CI coverage run against this PR a few minutes back: https://github.com/NVIDIA/cuda-python/actions/runs/41694943698 |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Just to confirm: I used my existing codex session to re-review after the substantial changes: no findings.
|
Coverage CI passed — all 37 checks green on run 41696905251 (temp branch |
Description
closes #3341
Raise the GPU runner's base Python executable stack reserve to at least 8 MiB using a standard-library PE-header helper. Write a patched copy and replace the executable after Python exits, avoiding the need for editbin on the GPU runner. Preserve the existing editbin build-job step. (note, we don't have editbin on gpu runners, so this is the workaround)
run_pytest_with_stack.py enlarges only the worker thread running pytest. Samples, subprocess tests, and multiprocessing workers start fresh Python interpreters with the executable's original stack reserve, so they still overflow. The wrapper also leaves Python's main thread waiting for pytest, which can delay deferred cleanup scheduled through Py_AddPendingCall.
Run pytest directly on the main thread with the patched interpreter and remove the worker-thread wrapper. Keep continue-on-error enabled.
Validation: 22 tests passed; Ruff, formatting, and workflow syntax checks passed. Verified patched PE32 and PE32+ executables with objdump. Windows GPU CI confirmation remains pending.
Checklist