Skip to content

Test using pytest-run-parallel and related fixups in the tests - #2194

Open
seberg wants to merge 7 commits into
NVIDIA:mainfrom
seberg:ft-testing
Open

Test using pytest-run-parallel and related fixups in the tests#2194
seberg wants to merge 7 commits into
NVIDIA:mainfrom
seberg:ft-testing

Conversation

@seberg

@seberg seberg commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Description

Enable testing with pytest-run-parallel on CI for 3.14t and 3.15t as well some residual related fixes.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

copy-pr-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added cuda.bindings Everything related to the cuda.bindings module cuda.core Everything related to the cuda.core module cuda.pathfinder Everything related to the cuda.pathfinder module labels Jun 10, 2026
@seberg seberg changed the title Ft testing Test using pytest-run-parallel and related fixups in the tests Jun 10, 2026
@leofang
leofang self-requested a review June 11, 2026 02:55
@seberg

seberg commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test ed40f60

@github-actions

Copy link
Copy Markdown

@github-actions github-actions Bot added the CI/CD CI/CD infrastructure label Jun 11, 2026
@seberg

seberg commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Fun, the refactor made the cufile xfail-strict tests pass on CI, but I didn't set up the parallel run correctly... one more try:

/ok to test eb6a2ff

@seberg

seberg commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test eb6a2ff

@seberg

seberg commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 7b59bff

@copy-pr-bot

copy-pr-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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.

@seberg
seberg force-pushed the ft-testing branch 2 times, most recently from 7d33149 to 4f2cc7a Compare July 8, 2026 11:52
@seberg
seberg marked this pull request as ready for review July 8, 2026 11:53
@seberg seberg added the P1 Medium priority - Should do label Jul 8, 2026
@seberg

seberg commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Still quite a few changes in this now (and let's see if CI passes) including gh-2321. But maybe it's still fine to review.

The most annoying one is probably the test_object_protocols.py changes. IIRC the problem was that the design with the .getfixturevalue() deferred things into threads, which might even have been fine with initialized.
But none of the tests actually test things deep enough to require cuda set up in the test itself.

@seberg seberg added this to the cuda.core next milestone Jul 14, 2026

@Andy-Jost Andy-Jost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs another review round after #2454 merges and it is rebased/merged with main

Comment thread ci/tools/run-tests Outdated
if python -c 'import sys; assert not sys._is_gil_enabled()' 2> /dev/null; then
FREE_THREADING="-ft"
PYTEST_PARALLEL_ARGS=(--parallel-threads=4)
pip install pytest-run-parallel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add this to the test groups in the normal places under cuda_{bindings,core,pathfinder}/pyproject.toml?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I added it as a test-ft but it currently only contains pytest-run-parallel. Happy to do it differently.

(Semi-related, removed the special logic for cupy not support free-threaded as it does now.)

seberg added 4 commits August 28, 2026 15:56
Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
This also seems to fix the issues around memory resource finishing/cleanup.
I.e. the main part of the issue was apparently issueing device syncs
from all threads.
(Clean-up in a follow-up.)

@seberg seberg left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(Few comments for review 🤞 that it will run through successfully in CI with all the drift going on.)

# never enqueued because their owning object had not been collected yet.
gc.collect()
driver.cuCtxSynchronize()
_ = _device_unset_current()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was causing issues for some graph tests (because a device sync disrupts graph capture).

But, actually, it also seems to fix the mr cleanup issue... I.e. the dead-lock I was seeing happens only if there is a device synchronization on each thread.

That should allow a bunch of cleanups, but I would prefer to follow-up for it.

@pytest.mark.agent_authored(model="gpt-5.6")
@pytest.mark.parametrize("fixture_name", CLOSEABLE_TYPES)
def test_closeable_object_state_and_safe_inspection(fixture_name, request):
@pytest.mark.thread_unsafe(reason="closes a fixture object shared between threads")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just marked as unsafe here, but also changed to follow the same indirect fixture pattern as the rest of the file.

info = cuda_stream_attr()
else:
info = cuda_stream_attr
warnings.simplefilter("once", DeprecationWarning)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This triggered a failure. Could also just mark the test as unsafe, but mutating global state here seems wrong (e.g. what if the user wants to raise DeprecationWarnings?).

Comment thread ci/tools/run-tests Outdated
if python -c 'import sys; assert not sys._is_gil_enabled()' 2> /dev/null; then
FREE_THREADING="-ft"
PYTEST_PARALLEL_ARGS=(--parallel-threads=4)
pip install pytest-run-parallel

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I added it as a test-ft but it currently only contains pytest-run-parallel. Happy to do it differently.

(Semi-related, removed the special logic for cupy not support free-threaded as it does now.)

@seberg
seberg requested a review from Andy-Jost August 28, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD CI/CD infrastructure cuda.bindings Everything related to the cuda.bindings module cuda.core Everything related to the cuda.core module cuda.pathfinder Everything related to the cuda.pathfinder module P1 Medium priority - Should do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants