feat(operations): add release soak test - #176
Conversation
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
|
WalkthroughChangesSoak Test
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
scripts/soak_test.py (3)
132-149: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
process_sampleblocks the event loop.It's called from
reporter(Line 442) on the running loop, so thepsfork/exec stalls all in-flight workers and skews the latency samples they're recording. Cheap to make non-blocking.As per coding guidelines: "Use async-first Python APIs".
♻️ Proposed fix
- rss_mib, cpu_percent = process_sample(server_pid) + rss_mib, cpu_percent = await asyncio.to_thread(process_sample, server_pid)🤖 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 `@scripts/soak_test.py` around lines 132 - 149, Make process_sample asynchronous and replace the blocking subprocess.run call with an async-first subprocess API, awaiting completion while preserving the existing RSS/CPU parsing and None-return behavior. Update reporter to await process_sample wherever it samples process metrics.Source: Coding guidelines
247-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to the public writer methods.
write_interval,write_error, andcloseare public API entry points; note theMAX_ERROR_RECORDSdrop invariant inwrite_error.As per coding guidelines: "Add concise triple-quoted docstrings for public functions, classes, methods, and API entry points, documenting behavior, important invariants, and relevant errors."
🤖 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 `@scripts/soak_test.py` around lines 247 - 261, Add concise triple-quoted docstrings to the public methods write_interval, write_error, and close in the writer class, documenting their behavior; explicitly note that write_error drops records after MAX_ERROR_RECORDS and tracks the dropped count.Source: Coding guidelines
876-903: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTask failure discards
summary.jsonfor the whole run.Raising here exits before the summary block, so a single failed worker after 47 hours leaves only
intervals.csv/errors.jsonland a stderr line. Consider recording the failure into the summary (completed_durationis already false, so it will be marked FAIL) and returning 1/2 instead of skipping artifact generation.🤖 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 `@scripts/soak_test.py` around lines 876 - 903, Update the task-result handling loop around run_tasks and worker_results/background_results so failed tasks are recorded for the run and do not raise before the build_summary and summary.json artifact generation. Preserve the failure details in the summary’s failure_reasons, ensure the resulting summary is marked failed, and return the appropriate nonzero status after writing the summary.tests/test_soak_test.py (1)
96-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winByte-matching the serialized body is brittle; prefer
respxper guidelines.
request.read().find(b'"stream":true')depends on httpx's compact JSON separators; a serializer change silently flips these tests to the wrong branch without failing. Decode the body instead. Separately, these handlers are the caserespxis meant for.As per coding guidelines: "Use
respxfor HTTP mocking".♻️ Sturdier branch condition
def handler(request: httpx.Request) -> httpx.Response: - if request.read().find(b'"stream":true') >= 0: + if json.loads(request.read())["stream"]:🤖 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 `@tests/test_soak_test.py` around lines 96 - 147, Replace the byte-level request-body checks in the handlers for test_send_request_accepts_json_and_streaming_success and test_send_request_rejects_missing_fields_and_non_sse_stream with decoded JSON inspection of the stream field, and migrate these HTTP mocks to respx as required by the project guidelines. Preserve the existing JSON-success, SSE-success, invalid-response, and invalid-stream outcomes for stream=False and stream=True.Source: Coding guidelines
🤖 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.
Inline comments:
In `@docs/operations/soak_test.md`:
- Line 44: Update the health readiness instruction in the soak test
documentation to require both HTTP 200 and a JSON response containing
{"status":"ok"} from GET /health. Make clear that both conditions must be
satisfied before proceeding.
- Around line 45-46: Update the soak-test instructions to select and pass the
model id returned in each `/v1/models` response, rather than a release route
identifier. Apply this correction to all referenced command examples and
preserve the existing `--model` usage.
- Around line 97-110: Update the pass-criteria checklist in the soak test
documentation to replace the “no inference request fails” requirement with a
requirement that the inference error rate is at or below --max-error-rate, whose
default is 0. Keep the remaining criteria unchanged and align the wording with
the runner’s configured error-budget gate.
In `@scripts/soak_test.py`:
- Around line 403-407: Update the health-check logic around the response JSON
parsing so non-dict JSON bodies are treated as unhealthy rather than calling
.get() on them. Validate that the parsed body is a mapping before reading its
"status" field, while preserving the existing healthy condition and failure
handling in the surrounding try/except.
In `@tests/test_soak_test.py`:
- Around line 179-204: Increase the live soak duration configured in the test
invocation around soak_test.run, and adjust the report and invalid-canary
intervals if needed to preserve multiple metric and canary checks. Keep the
existing success, endpoint, and invalid_request_canaries assertions unchanged
while providing sufficient timing headroom for slower CI runners.
---
Nitpick comments:
In `@scripts/soak_test.py`:
- Around line 132-149: Make process_sample asynchronous and replace the blocking
subprocess.run call with an async-first subprocess API, awaiting completion
while preserving the existing RSS/CPU parsing and None-return behavior. Update
reporter to await process_sample wherever it samples process metrics.
- Around line 247-261: Add concise triple-quoted docstrings to the public
methods write_interval, write_error, and close in the writer class, documenting
their behavior; explicitly note that write_error drops records after
MAX_ERROR_RECORDS and tracks the dropped count.
- Around line 876-903: Update the task-result handling loop around run_tasks and
worker_results/background_results so failed tasks are recorded for the run and
do not raise before the build_summary and summary.json artifact generation.
Preserve the failure details in the summary’s failure_reasons, ensure the
resulting summary is marked failed, and return the appropriate nonzero status
after writing the summary.
In `@tests/test_soak_test.py`:
- Around line 96-147: Replace the byte-level request-body checks in the handlers
for test_send_request_accepts_json_and_streaming_success and
test_send_request_rejects_missing_fields_and_non_sse_stream with decoded JSON
inspection of the stream field, and migrate these HTTP mocks to respx as
required by the project guidelines. Preserve the existing JSON-success,
SSE-success, invalid-response, and invalid-stream outcomes for stream=False and
stream=True.
🪄 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.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c51ae66a-bea9-400b-b5b3-fb971f696e24
📒 Files selected for processing (5)
.gitignoredocs/operations/soak_test.mdmkdocs.ymlscripts/soak_test.pytests/test_soak_test.py
| SOAK_SERVER_PID=$! | ||
| ``` | ||
|
|
||
| Wait for `GET http://127.0.0.1:4000/health` to return HTTP 200. Check |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the full health readiness contract.
The runner requires /health to return HTTP 200 and JSON with {"status": "ok"}. Mentioning only HTTP 200 can falsely indicate readiness before the soak test’s preflight will pass.
🤖 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 `@docs/operations/soak_test.md` at line 44, Update the health readiness
instruction in the soak test documentation to require both HTTP 200 and a JSON
response containing {"status":"ok"} from GET /health. Make clear that both
conditions must be satisfied before proceeding.
| `GET http://127.0.0.1:4000/v1/models` and choose the route id that represents | ||
| the release workload. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the model ID expected by --model, not a route ID.
--model is validated against the id values returned by /v1/models. If a release route identifier differs from that model ID, these documented commands fail during preflight.
Proposed wording
-choose the route id that represents the release workload.
+choose the model id returned by `/v1/models` that represents the release workload.
- --model RELEASE_ROUTE_ID
+ --model RELEASE_MODEL_IDAlso applies to: 62-63, 79-80, 91-92
🤖 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 `@docs/operations/soak_test.md` around lines 45 - 46, Update the soak-test
instructions to select and pass the model id returned in each `/v1/models`
response, rather than a release route identifier. Apply this correction to all
referenced command examples and preserve the existing `--model` usage.
| The command exits with status 0 only when: | ||
|
|
||
| - the requested duration completes; | ||
| - at least one inference request completes; | ||
| - no inference request fails; | ||
| - every periodic liveness check passes; | ||
| - every `/metrics` read returns both Switchyard request counters; | ||
| - every requested process sample returns RSS and CPU data; | ||
| - every invalid-request recovery check passes; | ||
| - the server request counter never resets; and | ||
| - RSS growth stays within `--max-rss-growth-mib` when that limit is set. | ||
|
|
||
| If the release plan permits transient failures from a remote provider, set an | ||
| explicit error budget with `--max-error-rate`. Record the reason for that |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the pass criteria with --max-error-rate.
The checklist says “no inference request fails,” but the runner explicitly supports an error budget. Replace that criterion with “the inference error rate is at or below --max-error-rate (default 0)” so the runbook matches the implemented gate.
🤖 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 `@docs/operations/soak_test.md` around lines 97 - 110, Update the pass-criteria
checklist in the soak test documentation to replace the “no inference request
fails” requirement with a requirement that the inference error rate is at or
below --max-error-rate, whose default is 0. Keep the remaining criteria
unchanged and align the wording with the runner’s configured error-budget gate.
| try: | ||
| response = await client.get("/health") | ||
| healthy = response.status_code == 200 and response.json().get("status") == "ok" | ||
| except (httpx.HTTPError, json.JSONDecodeError, ValueError): | ||
| healthy = False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-dict /health JSON body raises an uncaught AttributeError.
If /health returns a JSON scalar or list, .get(...) raises AttributeError, which isn't in the except tuple. That kills the reporter task, sets stop, and aborts the whole 48-hour run with exit 2 instead of recording a liveness failure — exactly the degraded-server case this monitor exists to survive.
🛡️ Proposed fix
try:
response = await client.get("/health")
- healthy = response.status_code == 200 and response.json().get("status") == "ok"
+ body = response.json() if response.status_code == 200 else None
+ healthy = isinstance(body, dict) and body.get("status") == "ok"
except (httpx.HTTPError, json.JSONDecodeError, ValueError):
healthy = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| response = await client.get("/health") | |
| healthy = response.status_code == 200 and response.json().get("status") == "ok" | |
| except (httpx.HTTPError, json.JSONDecodeError, ValueError): | |
| healthy = False | |
| try: | |
| response = await client.get("/health") | |
| body = response.json() if response.status_code == 200 else None | |
| healthy = isinstance(body, dict) and body.get("status") == "ok" | |
| except (httpx.HTTPError, json.JSONDecodeError, ValueError): | |
| healthy = False |
🤖 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 `@scripts/soak_test.py` around lines 403 - 407, Update the health-check logic
around the response JSON parsing so non-dict JSON bodies are treated as
unhealthy rather than calling .get() on them. Validate that the parsed body is a
mapping before reading its "status" field, while preserving the existing healthy
condition and failure handling in the surrounding try/except.
| "--duration", | ||
| "0.8s", | ||
| "--concurrency", | ||
| "2", | ||
| "--stream-ratio", | ||
| "0", | ||
| "--report-interval", | ||
| "0.2", | ||
| "--invalid-canary-interval", | ||
| "0.2", | ||
| "--max-error-rate", | ||
| "0", | ||
| "--results-dir", | ||
| str(results_dir), | ||
| ] | ||
| ) | ||
| assert await soak_test.run(args) == 0 | ||
| finally: | ||
| server.should_exit = True | ||
| thread.join(timeout=10) | ||
|
|
||
| assert not thread.is_alive() | ||
| summary = json.loads((results_dir / "summary.json").read_text()) | ||
| assert summary["passed"] is True | ||
| assert all(summary["endpoint_successes"][endpoint] > 0 for endpoint in soak_test.ENDPOINTS) | ||
| assert summary["invalid_request_canaries"] > 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm /metrics exposes both counters the soak gate requires.
rg -nP 'switchyard_total_(requests|errors)' -g '!scripts/soak_test.py' -g '!tests/test_soak_test.py'
# Confirm empty `messages` yields HTTP 400 (the canary contract).
rg -nP -C3 'messages.*(empty|min_length|at least one)' --type=py --type=rustRepository: NVIDIA-NeMo/Switchyard
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant test and implementation files first.
git ls-files | rg '^(tests/test_soak_test\.py|.*soak.*|.*metrics.*|.*switchyard.*|.*canary.*|.*health.*)$' || true
printf '\n== tests/test_soak_test.py ==\n'
wc -l tests/test_soak_test.py
sed -n '1,260p' tests/test_soak_test.py
printf '\n== search for metrics counters and empty-message handling ==\n'
rg -n -C 3 'switchyard_total_(requests|errors)|messages|canary|metrics' .
printf '\n== outline likely source files if present ==\n'
fd -a 'soak_test.py|metrics|canary|switchyard' . 2>/dev/null || trueRepository: NVIDIA-NeMo/Switchyard
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow the inspection to the soak test and the service code it exercises.
ast-grep outline tests/test_soak_test.py --view expanded || true
printf '\n== relevant ranges from tests/test_soak_test.py ==\n'
sed -n '1,240p' tests/test_soak_test.py
printf '\n== files mentioning /metrics or 400 responses for empty messages ==\n'
rg -n -C 3 'metrics|400|Bad Request|messages' tests . --glob '!tests/test_soak_test.py'Repository: NVIDIA-NeMo/Switchyard
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation behind the soak test's live server and its metrics endpoint.
fd -a 'app.py|main.py|server.py|routes.py|metrics*.py|*switchyard*' . 2>/dev/null | head -n 200
printf '\n== refs to metrics endpoint definitions ==\n'
rg -n -C 3 '(^|/)metrics\b|@.*metrics|def .*metrics|prometheus|Counter\(' . --glob '!tests/test_soak_test.py'
printf '\n== refs to empty messages / validation errors ==\n'
rg -n -C 3 'messages.*(empty|len|min_length|at least one)|Bad Request|HTTPException\(.+400|status_code\s*=\s*400' . --glob '!tests/test_soak_test.py'Repository: NVIDIA-NeMo/Switchyard
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== tests/test_soak_test.py (relevant range) ==\n'
sed -n '150,220p' tests/test_soak_test.py
printf '\n== switchyard/lib/endpoints/outcome_metrics.py ==\n'
sed -n '1,180p' switchyard/lib/endpoints/outcome_metrics.py
printf '\n== switchyard/lib/endpoints/anthropic_messages_endpoint.py ==\n'
sed -n '90,110p' switchyard/lib/endpoints/anthropic_messages_endpoint.py
printf '\n== empty messages validation in chat request path ==\n'
rg -n -C 4 'empty messages|messages.*empty|validate\(\)|SwitchyardInvalidRequestError|HTTPException\(status_code=400' switchyard tests --glob '!tests/test_soak_test.py'Repository: NVIDIA-NeMo/Switchyard
Length of output: 19234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('tests/test_soak_test.py')
text = path.read_text()
for needle in ['--duration', '--max-error-rate', 'endpoint_successes', 'invalid_request_canaries']:
idx = text.find(needle)
print(f'needle={needle!r} idx={idx}')
if idx != -1:
start = text.rfind('\n', 0, idx - 120)
end = text.find('\n', idx + 200)
print(text[start:end])
print('\n--- outcome_metrics names ---')
om = Path('switchyard/lib/endpoints/outcome_metrics.py').read_text()
for needle in ['switchyard_client_responses_total', 'switchyard_client_successes_total', 'switchyard_total_requests', 'switchyard_total_errors']:
print(needle, needle in om)
PYRepository: NVIDIA-NeMo/Switchyard
Length of output: 1851
Timing-sensitive soak test
An 0.8s live run with --max-error-rate 0 leaves little headroom on slower CI runners. The metrics/canary premise is supported by the code, but this still looks like a narrow integration window that could flap under load.
🤖 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 `@tests/test_soak_test.py` around lines 179 - 204, Increase the live soak
duration configured in the test invocation around soak_test.run, and adjust the
report and invalid-canary intervals if needed to preserve multiple metric and
canary checks. Keep the existing success, endpoint, and invalid_request_canaries
assertions unchanged while providing sufficient timing headroom for slower CI
runners.
1 participant
What
This PR adds
scripts/soak_test.pyfor 48-hour release-candidate runs againsteither the Python server or
switchyard-server. The command:/health,/metrics, and optional local process RSS and CPU;The existing
switchyard serveandswitchyard-servercommands and configfiles remain unchanged. The release manager starts one of those servers and
points the new command at it. The runner process owns its request workers,
statistics, and result files.
The command returns a nonzero status for an early stop, an inference error,
a malformed JSON response, a non-SSE streaming response, a failed liveness or
metrics sample, a failed invalid-request check, a server counter reset, or an
RSS limit violation.
Why
Short tests do not expose late request failures, process restarts, falling
throughput, rising latency, or steady memory growth. This command gives release
managers one repeatable run and one result directory to attach to the release
record.
How tested
A local test started a real Switchyard HTTP server with a fixed loopback
backend, then ran:
It completed 506 requests with 0 errors, a 4.134 ms p95, successful health and
metrics samples, successful invalid-request checks, and a passing
summary.json.With live-provider E2E tests excluded and user credentials isolated, the
repository suite completed 1,730 tests and skipped 9. The full
tests/commandcannot complete on this workstation because Docker Desktop requires an NVIDIA
organization sign-in, and another existing E2E fixture still invokes the
removed
switchyard passthroughcommand.Checklist
Notes for reviewers
The default release decision allows no inference errors. Teams may set an
explicit error fraction for a remote provider, but the operations guide asks
them to record that exception with the release.
Summary by CodeRabbit
New Features
Documentation
Tests