Skip to content

[no-ci] coverage: fix Linux git ownership and add Windows stack wrapper #1829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mdboom merged 11 commits into from
Apr 16, 2026
+71 −34
Merged

[no-ci] coverage: fix Linux git ownership and add Windows stack wrapper #1829

Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b73a09a
coverage: add git safe.directory for linux builds
rluo8 Mar 30, 2026
326a960
coverage: run coverage for linux without root
rluo8 Apr 1, 2026
8bdbff0
Merge branch 'main' into main
rluo8 Apr 3, 2026
f440fa9
Merge branch 'NVIDIA:main' into main
rluo8 Apr 7, 2026
ecf269f
coverage: change github workspace owner for Linux coverage
rluo8 Apr 7, 2026
ed96375
coverage: run cuda core tests in coverage using 8M thread
rluo8 Apr 8, 2026
c0d8861
Merge branch 'NVIDIA:main' into main
rluo8 Apr 13, 2026
7291fcb
Merge branch 'NVIDIA:main' into main
rluo8 Apr 14, 2026
2137fed
Merge branch 'main' into main
rwgk Apr 15, 2026
d9ac8c6
Add coverage test helper to run pytest using larger stack
rluo8 Apr 15, 2026
66c2fd2
Merge branch 'main' into main
rwgk Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions .github/workflows/coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ jobs:
with:
fetch-depth: 0

- name: Fix workspace ownership
run: |
chown -R $(id -u):$(id -g) "$GITHUB_WORKSPACE"

- name: Install dependencies
uses: ./.github/actions/install_unix_deps
continue-on-error: false
Expand Down Expand Up @@ -377,11 +381,40 @@ jobs:
sys.exit(result['code'])
PYTEST_EOF

- name: Run cuda.core tests
# Same 8MB stack thread as bindings: Cython linetrace under coverage on Windows
# can need a larger stack than the default thread size.
- name: Run cuda.core tests (with 8MB stack)
continue-on-error: true
run: |
cd "${{ steps.install-root.outputs.INSTALL_ROOT }}"
"$GITHUB_WORKSPACE/.venv/Scripts/pytest" -v --cov=./cuda --cov-append --cov-context=test --cov-config="$GITHUB_WORKSPACE/.coveragerc" "$GITHUB_WORKSPACE/cuda_core/tests"
"$GITHUB_WORKSPACE/.venv/Scripts/python" << PYTEST_EOF
Copy link
Copy Markdown
Contributor

@rwgk rwgk Apr 15, 2026

Choose a reason for hiding this comment

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

Generated by Cursor GPT-5.4 Extra High Fast with a couple prompts (it's a suggestion to make this DRY):

One cleanup idea: the new cuda.core block looks like it is only changing the execution model from

"$GITHUB_WORKSPACE/.venv/Scripts/pytest" ...

to "run the same pytest args on a freshly-created Python thread after threading.stack_size(8 * 1024 * 1024)".

I do not think there is a native one-line pytest / Actions knob for "run this with an 8 MB stack", so the wrapper approach makes sense. That said, would you consider factoring the duplicated bindings / core heredoc into a small helper, or at least using a shorter inline wrapper? The current ~30-line block seems to be mostly plumbing rather than behavior change.

For example, the inline version could be reduced to something like:

import concurrent.futures
import os
import sys
import threading
import pytest

os.chdir(r'${{ steps.install-root.outputs.INSTALL_ROOT }}')
threading.stack_size(8 * 1024 * 1024)

args = [
    '-v',
    '--cov=./cuda',
    '--cov-append',
    '--cov-context=test',
    f'--cov-config={os.environ["GITHUB_WORKSPACE"]}/.coveragerc',
    f'{os.environ["GITHUB_WORKSPACE"]}/cuda_core/tests',
]

with concurrent.futures.ThreadPoolExecutor(max_workers=1) as ex:
    sys.exit(ex.submit(pytest.main, args).result())

Even better might be a tiny checked-in helper that both steps call, so the workflow stays readable and the "8 MB stack thread" workaround only lives in one place.

Agent notes / clues

  • Scope: the only changed file in PR 1829 is .github/workflows/coverage.yml.
  • The key semantic change in the cuda.core step is the same workaround already used for cuda.bindings: run pytest.main(...) on a newly-created thread after threading.stack_size(8 * 1024 * 1024).
  • I did not see another meaningful behavior change in the expanded block:
    • same pytest flags
    • same coverage config
    • same target test directory
    • same effective working directory
    • exit code is still propagated
  • os.chdir(...) in the Python snippet is probably redundant because the shell step already does cd "${{ steps.install-root.outputs.INSTALL_ROOT }}", but keeping it is harmless.
  • There is no obvious native one-liner in GitHub Actions / pytest / CPython to say "run this test invocation with an 8 MB stack"; some Python wrapper is required because threading.stack_size(...) only affects threads created after the call.
  • If another agent wants to refactor instead of just comment, the likely clean option is a helper script, something like:
    • ci/tools/run_pytest_with_stack.py
    • inputs: cwd, test path, stack size, and passthrough pytest args
    • behavior: call threading.stack_size(...), spawn one thread, run pytest.main(args), exit with its code
  • Alternative smaller inline version: ThreadPoolExecutor(max_workers=1) is a compact replacement for the manual result dict plus Thread.start()/join().
  • Practical review angle: this is a readability / maintainability suggestion, not a correctness concern. The workflow was manually triggered on an upstream branch containing the PR commit, and it passed.

Copy link
Copy Markdown
Contributor

@rwgk rwgk Apr 15, 2026

Choose a reason for hiding this comment

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

I forgot to mention before: I think one of these is redundant:

         cd "${{ steps.install-root.outputs.INSTALL_ROOT }}"
...
          os.chdir(r'${{ steps.install-root.outputs.INSTALL_ROOT }}')

If you add the suggested helper, I'd drop the cd and pass steps.install-root.outputs.INSTALL_ROOT to the helper; I believe that'll be most readable.

Copy link
Copy Markdown
Contributor Author

@rluo8 rluo8 Apr 15, 2026

Choose a reason for hiding this comment

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

Thanks Ralf. It's reasonable to add a helper to make the workflow cleaner. I'll update that.

import os
import sys
import threading
import pytest

os.chdir(r'${{ steps.install-root.outputs.INSTALL_ROOT }}')
threading.stack_size(8 * 1024 * 1024)
result = {'code': 1}

def _run():
workspace = os.environ['GITHUB_WORKSPACE']
result['code'] = pytest.main([
'-v',
'--cov=./cuda',
'--cov-append',
'--cov-context=test',
f'--cov-config={workspace}/.coveragerc',
f'{workspace}/cuda_core/tests'
])

t = threading.Thread(target=_run)
t.start()
t.join()

print(f'cuda.core tests exit code: {result["code"]}')
sys.exit(result['code'])
PYTEST_EOF

- name: Copy Windows coverage file to workspace
run: |
Expand Down
Loading