Prevent CUDA error cascade on multi-GPU test runs - #2234
adenzler-nvidia merged 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClarified CLI help for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
shi-eric
added
testing
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/tests/thirdparty/unittest_parallel.py`:
- Line 236: The change made max_tasks_per_child=1 unconditionally (breaking the
--disable-process-pooling CLI flag); update the ProcessPoolExecutor construction
so that max_tasks_per_child=1 is only set when process pooling is enabled by
checking args.disable_process_pooling (e.g., pass max_tasks_per_child=1
conditionally or omit the kw arg when args.disable_process_pooling is True), use
the args.disable_process_pooling symbol and the max_tasks_per_child kwarg near
the ProcessPoolExecutor/bootstrap code in unittest_parallel.py to locate the
spot, and ensure the CLI behavior is preserved (or if you intentionally remove
the option instead, remove the flag and add a [Unreleased]/Fixed entry in
CHANGELOG.md).
- Around line 234-237: The ProcessPoolExecutor call using max_tasks_per_child is
only valid on Python 3.11+, so change the block that constructs the worker pool
(the with concurrent.futures.ProcessPoolExecutor(...
mp_context=multiprocessing.get_context(...), max_workers=process_count,
max_tasks_per_child=1) call) to first check sys.version_info >= (3,11) (or
attempt creation and fall back on TypeError), and otherwise use the existing
multiprocessing.Pool(..., processes=process_count, maxtasksperchild=1,
context=...) fallback; preserve honoring the existing --disable-process-pooling
flag so that when pooling is disabled you still use the non-recycling path, and
ensure mp_context and process_count variables are passed appropriately in both
branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a9cfd092-1902-42db-9e8f-031e46f86bcf
📒 Files selected for processing (1)
newton/tests/thirdparty/unittest_parallel.py
9533b79 to
ac52e41
Compare
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looks good — correct version guard for max_tasks_per_child (Python 3.11+), consistent with the existing maxtasksperchild behavior in the multiprocessing.Pool path, and well-documented help text update.
|
Setting this back to draft mode. The time it takes to run the entire suite slowed down more than expected, especially on Windows. |
ac52e41 to
b66e87a
Compare
shi-eric
changed the title
b66e87a to
a6df162
Compare
|
@adenzler-nvidia this one should be ready again. Due to the additional overheads, I only enable this setting when the system has multiple GPUs. |
|
Pushing to 1.2 not critical to get into 1.1. |
|
oh looks like I missed that ping. This PR is still relevant, right? |
yes |
a6df162 to
9c6f3f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/tests/thirdparty/unittest_parallel.py`:
- Around line 242-243: The current check only applies
args.disable_process_pooling when sys.version_info >= (3, 11), making the flag
silently no-op on Python 3.10; add a one-line stderr warning when
args.disable_process_pooling is True but sys.version_info < (3, 11) so users
know the flag is ineffective. Detect this right where sys.version_info and
args.disable_process_pooling are evaluated (the same block that sets
executor_kwargs["max_tasks_per_child"]) and write a short warning to stderr (or
use the existing logger) explaining that --disable-process-pooling requires
Python 3.11+. Ensure the message runs only when the flag is set and Python
version < (3, 11).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d7fbdffb-5a92-4bf2-9655-45fb82f2e2bc
📒 Files selected for processing (2)
CHANGELOG.mdnewton/tests/thirdparty/unittest_parallel.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
LGTM — could you just move the CHANGELOG entry to the [Unreleased] section? After merging main it lands in the released [1.1.0] block.
Replace worker processes after each test suite to prevent a fatal CUDA error in one test from corrupting the context for subsequent tests in the same worker process.
The max_tasks_per_child parameter for ProcessPoolExecutor was added in Python 3.11. Build the executor kwargs conditionally so the test runner remains compatible with Python 3.10 while still recycling workers on 3.11+ to prevent CUDA error cascade.
Explain that the flag only affects the multiprocessing.Pool backend and that the concurrent.futures backend always recycles workers via max_tasks_per_child=1 on Python 3.11+.
Only set max_tasks_per_child=1 when multiple CUDA devices are detected or --disable-process-pooling is explicitly passed. This restores process reuse on single-GPU systems for better test throughput while still preventing CUDA error cascade on multi-GPU setups.
9c6f3f2 to
44a40c3
Compare
4 participants
Description
Prevent CUDA errors from cascading across unrelated test suites in the parallel test runner on multi-GPU systems.
Without process isolation, a fatal CUDA error in one test suite (e.g. an illegal memory access) corrupts the CUDA context for the entire worker process. Every subsequent test suite dispatched to that worker then fails — even completely unrelated tests — because the context is unrecoverable.
This PR sets
max_tasks_per_child=1on theProcessPoolExecutor(Python 3.11+) so each worker is replaced after completing a single test suite, but only when multiple CUDA devices are detected or when--disable-process-poolingis explicitly passed. Single-GPU systems retain process reuse for faster throughput.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
All multi-GPU tests pass on
maintoday. To verify this change prevents error cascade, we temporarily reverted #2226 (which fixed the originaltest_sdf_textureCUDA errors) to reproduce the cascading failure scenario on a multi-GPU AWS EC2 instance (3247 tests).Without process isolation: 7 errors
An illegal memory access in
test_sdf_texturecascaded to 4 unrelated tests across different modules:test_uint16_vs_nanovdb_distance_cuda_1test_uint16_vs_nanovdb_gradient_cuda_0test_uint16_vs_nanovdb_gradient_cuda_1test_capsule_geometry_is_batched_across_scalestest_log_state_routes_capsules_to_log_capsulestest_picking_setup_device_cuda_0test_picking_setup_device_cuda_1The viewer geometry batching and viewer picking tests have nothing to do with SDF textures — they failed only because they were dispatched to a worker whose CUDA context was poisoned.
With process isolation: 3 errors
Only the actual
test_sdf_texturefailures remain — the 4 cascaded failures in unrelated modules are eliminated:test_uint16_vs_nanovdb_distance_cuda_1test_uint16_vs_nanovdb_gradient_cuda_0test_uint16_vs_nanovdb_gradient_cuda_1The remaining 3 errors are all within the same
test_sdf_texturesuite (the deliberately reintroduced bug from reverting #2226).Summary by CodeRabbit