Uh oh!
There was an error while loading. Please reload this page.
Ft testing test fixes - #2454
Conversation
| env = os.environ.copy() | ||
| env["CUDA_BINDINGS_SKIP_EXAMPLE"] = "100" | ||
| env["MPLBACKEND"] = "Agg" # avoid plt.show() from blocking |
There was a problem hiding this comment.
I dunno if others get a pop-up for these tests, I did and with parallel testing, it might be a 100 windows :).
| def test_graphdef_handle_valid(sample_graphdef): | ||
| def test_graphdef_handle_valid(init_cuda): | ||
| """GraphDefinition has a valid non-null handle.""" | ||
| sample_graphdef = GraphDefinition() |
There was a problem hiding this comment.
Admittedly, we could also do the fixture hack, but somehow this felt just as well for now...
| # Indirect-parametrize helpers: request.getfixturevalue() runs here, in the | ||
| # fixture (main thread), so the resolved object is already available when the | ||
| # test function runs in a worker thread. |
There was a problem hiding this comment.
Admittedly, the workers may not be valid on other threads, but we don't actually use CUDA API anywhere...
There was a problem hiding this comment.
This seems like a better approach. Suggested comment:
# Resolve fixture names during pytest setup, before pytest-run-parallel starts
# worker threads. The workers then share the resolved, read-only test object.
| // Check for timeout | ||
| if (clock64() - start >= timeout_cycles) { | ||
| signal.store(-1, cuda::memory_order_relaxed); |
There was a problem hiding this comment.
I think I was just debugging/trying to understand where things are hanging.
Either way, the main point here is that we need to compile the kernel only once, because otherwise compiling the kernel itself will block other things and break other threads.
There was a problem hiding this comment.
Nvm, I pulled out the latch.py fixes... They are correct, but the fact that there are allocations, makes them a bit unreliable and the only way to fix it is to introduce that barrier_wait fixture (I had forgotten about it).
And I don't like that fixtures quite enough yet, maybe after pytest-run-parallel has better infrastructure for it.
| assert list(out) == [0xCD] * 4 | ||
| @pytest.mark.thread_unsafe(reason="deferred cleanup on main thread which would wait") |
There was a problem hiding this comment.
There are three tests here that fail with _wait_until@Andy-Jost. I just skipped them, the reason seems to be that cleanup can never happen as long as the main thread is waiting in thread.join()?
I suspect that is a potential but very minor issue (you would think eventually this cleanup happens). But wanted to make a comment. I didn't try to understand what is going on here exactly!
There was a problem hiding this comment.
The issue is that deferred cleanup requires Python's main thread.
- pytest main thread: start workers -> join workers (blocking)
- pytest worker thread: del graph -> _wait_until
- CUDA calls back to Python from a separate thread and cuda.core schedules the destruction with
Py_AddPendingCall - Python's pending callbacks only run on the main thread.
pytest-run-parallel needs to give pending calls a chance to run. Instead of a blocking worker.join:
while any(worker.is_alive() for worker in workers):
for worker in workers:
worker.join(timeout=0.01)
The test also needs to be updated to use threading.main_thread().ident rather than threading.get_ident()
|
- thread_unsafe: nvml init ref-count, graphMem attr, mock-based tests, OpenGL, peer-access pool state, multiprocessing warning, program-cache race reproduction, and functools.cache mutation tests - parallel_threads_limit: IPC / worker-pool tests that spawn subprocesses or open file descriptors (limit 4), example tests (limit 8), and the event-registration test whose timeouts are slow Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
…unsafe always Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
…empool Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
After my first AI try was a crazy mess, the second run actually found a neat solution... These objects can be created in the main thread, but we can't create them on the fly in many threads as it was... Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
For some reason the latch kernel helper test started failing now (it did not before my update from CUDA 13.2 to 13.3?). The reason isn't that it is not thread-safe, but that something (presumably module loading/unloading) causes synchronizations which in turn cause threads having to wait on their LatchKernel to finish. And of course the test itself really needs that not to happen. Making sure there is only one LatchKernel compiled and loaded exactly once seems to avoid this problem. Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
Concurrent LatchKernel runs can overlap pinned flag alloc/free across workers; mark them thread-unsafe until a barrier_wait is restored. Also drop the compile-once LatchKernel helper changes for now.
40a0175 to
0bde899Compare
Andy-Jost
left a comment
There was a problem hiding this comment.
Getting the graph-cleanup tests running seems to require a change to pytest-run-parallel. Can we merge this and follow up with a PR there?
| assert list(out) == [0xCD] * 4 | ||
| @pytest.mark.thread_unsafe(reason="deferred cleanup on main thread which would wait") |
There was a problem hiding this comment.
The issue is that deferred cleanup requires Python's main thread.
- pytest main thread: start workers -> join workers (blocking)
- pytest worker thread: del graph -> _wait_until
- CUDA calls back to Python from a separate thread and cuda.core schedules the destruction with
Py_AddPendingCall - Python's pending callbacks only run on the main thread.
pytest-run-parallel needs to give pending calls a chance to run. Instead of a blocking worker.join:
while any(worker.is_alive() for worker in workers):
for worker in workers:
worker.join(timeout=0.01)
The test also needs to be updated to use threading.main_thread().ident rather than threading.get_ident()
| # Indirect-parametrize helpers: request.getfixturevalue() runs here, in the | ||
| # fixture (main thread), so the resolved object is already available when the | ||
| # test function runs in a worker thread. |
There was a problem hiding this comment.
This seems like a better approach. Suggested comment:
# Resolve fixture names during pytest setup, before pytest-run-parallel starts
# worker threads. The workers then share the resolved, read-only test object.
Split from #2194 with all test changes. Split out for two reasons:
(adding a few inline comments)
CC @Andy-Jost