ci(test): fix grpc server GPU-memory leak and run cuopt tests in parallel#1512
Merged
Conversation
…olution test_incumbent_get_set_callback[swath1.mps] fails intermittently with assert 0 > 0 because swath1.mps is sometimes solved at the root node (integral LP relaxation or presolve), which produces no branch-and-bound iterations and therefore never triggers incumbent callbacks. - Skip gracefully when n_callbacks == 0 instead of failing, with a message indicating root-node solve - Fix the always-true assertion on termination status (== FeasibleFound or MILPTerminationStatus.Optimal was always True because the or operand is a truthy enum value) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
ramakrishnap-nv
added
non-breaking
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR updates incumbent callback handling, increases CI pytest parallelism, and refactors gRPC test server startup and shutdown around shared process-group helpers and xdist grouping markers. ChangesIncumbent callback, CI, and gRPC test orchestration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py (1)
95-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the skip guard.
n_callbacks == 0also covers a broken callback path, so usesolution.get_milp_stats()["num_nodes"] == 0before skipping; otherwise this test can hide a regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py` around lines 95 - 105, The skip condition in the incumbent callback test is too broad because get_callback.n_callbacks == 0 can also indicate a callback regression; tighten the guard in test_incumbent_callbacks by checking solution.get_milp_stats()["num_nodes"] == 0 before calling pytest.skip. Keep the existing callback assertions tied to get_callback and set_callback, but only skip when the solve finished at the root node, not just when no callbacks fired.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py`:
- Around line 95-105: The skip condition in the incumbent callback test is too
broad because get_callback.n_callbacks == 0 can also indicate a callback
regression; tighten the guard in test_incumbent_callbacks by checking
solution.get_milp_stats()["num_nodes"] == 0 before calling pytest.skip. Keep the
existing callback assertions tied to get_callback and set_callback, but only
skip when the solve finished at the root node, not just when no callbacks fired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 978aaa9e-f825-44ea-adf1-74ff68693f89
📒 Files selected for processing (1)
python/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py
tmckayus
approved these changes
Jul 2, 2026
Collaborator
Author
|
/merge |
CI Test Summary✅ All 31 test job(s) passed. |
All four tests run on a single worker so they don't compete for GPU resources with each other under -n 4. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
…karound Add a lightweight background GPU-memory sampler (ci/utils/gpu_monitor.sh) wrapped around the parallel (pytest-xdist -n 2) path in run_cuopt_pytests.sh. It records device memory.used/free/total and per-process (per xdist worker) usage on an interval and prints the peak on exit, so the ramp to the rmm::out_of_memory abort is visible in the job log. Only the -n 2 branch is instrumented; the nightly serial path is unchanged. Remove the @pytest.mark.xdist_group(name="incumbent_callbacks") markers from test_incumbent_callbacks.py — grouping onto one worker was a workaround for the same contention and is superseded by this diagnostic.
ramakrishnap-nv
requested review from
KyleFromNVIDIA
and removed request for
KyleFromNVIDIA
Fix a GPU-memory leak and stabilize the parallel wheel-test run: - Reap cuopt_grpc_server reliably: launch it in its own process group (start_new_session) with PR_SET_PDEATHSIG=SIGKILL, and tear it down with os.killpg (SIGTERM then SIGKILL). Previously only the parent was signalled, so the --workers child was orphaned and kept its ~7 GiB CUDA context; a crashed worker leaked the whole server. These accumulated across tests/runs until the device OOMed. Shared spawn_server/kill_server helpers now back every server start/stop (client, CPU-only, CLI, TLS, mTLS). - Group all cuopt_grpc_server-backed classes with xdist_group and run pytest with --dist loadgroup, so at most one server runs at a time instead of one duplicated per xdist worker. - Run cuopt Python tests at -n 4 (was -n 2) and add --max-worker-restart=0 so a crashed worker fails once instead of respawning into a crash spiral. - Make test_cpu_only_execution import grpc_server_fixtures when re-executed as a standalone subprocess (add the fixtures dir to sys.path). - Remove the temporary GPU-memory sampler used to diagnose the above.
…k-flaky-test # Conflicts: # ci/run_cuopt_pytests.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py (1)
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints (and param/return docs) to the new public helpers.
spawn_serverandkill_server(Line 156) are new public functions but lack type annotations and their docstrings don't document parameters/returns. Suggestspawn_server(cmd: list[str], env: dict[str, str] | None = None) -> subprocess.Popenandkill_server(proc: subprocess.Popen | None) -> None.As per path instructions: "Type hints on NEW public functions/classes" and "Docstring CONTENT on new public APIs — params, returns, raises".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py` at line 135, The new public helpers spawn_server and kill_server need proper type annotations and API docs. Update spawn_server to use explicit parameter and return types, and do the same for kill_server with a nullable process argument and None return; ensure both functions’ docstrings document their parameters and return values in the existing fixture helpers module.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py`:
- Line 135: The new public helpers spawn_server and kill_server need proper type
annotations and API docs. Update spawn_server to use explicit parameter and
return types, and do the same for kill_server with a nullable process argument
and None return; ensure both functions’ docstrings document their parameters and
return values in the existing fixture helpers module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0a6081d-c40e-4f92-bb8d-7de31be93cd0
📒 Files selected for processing (4)
ci/run_cuopt_pytests.shpython/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.pypython/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.pypython/cuopt/cuopt/tests/linear_programming/test_grpc_client.py
✅ Files skipped from review due to trivial changes (1)
- python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py
ramakrishnap-nv
changed the title
tmckayus
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cuopt_grpc_serverreliably — start it in its own process group and kill the whole group on teardown, so the server and its--workerschild are cleaned up instead of leaking their GPU context across tests/runs.--dist loadgroup) so at most one server runs at a time.-n 4, with--max-worker-restart=0so a crashed worker fails once instead of respawning into a crash loop.