Repository navigation
ci(recipes): use one load point for aggregated nightly tests - #15571
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe patch performance benchmark now uses concurrency levels 1, 8, 64, and 128. It no longer uses levels 256, 512, or 1024. ChangesBenchmark concurrency settings
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to The benchmark retains the intended concurrency levels and is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description does not match the changeset: it describes a single DeepSeek load point and Kimi changes, while the stated objective retains four DeepSeek concurrency levels and does not mention a Kimi change. It also omits the required Related Issues section. Resolution Revise the description to state that DeepSeek retains concurrency levels 1, 8, 64, and 128 and removes 256, 512, and 1024. Remove unsupported claims about a single DeepSeek load point and Kimi changes. Add the required Related Issues section and select either the linked-issue option or the no-related-issue confirmation.
Comment |
sara4dev
changed the title
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
sara4dev
changed the title
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at db33213f835df2c5bd4577b2214840aa679dd850. Two P2 findings are open in inline threads. There is no P3.
- [P2]
.github/ci/deepseek-v4-pro-agg/patch-perf.yaml:27: the removed load points are where an untracked vLLM worker crash happens. The thread asks for an issue. - [P2]
.github/ci/kimi-k25-agg/patch-perf.yaml:24: the Kimi trace allows at most 20 requests at once, so the job never reaches128. - Open item for #15537: it changes the same "Verify AIPerf results" step, and the two heads conflict there. To resolve it, keep the
concurrencies=line and the regex from #15571, and add|| "$RECIPE" == kimi-k25-disaggfrom #15537 to the condition. With only the #15537 side, no line setsconcurrencies, and every DeepSeek result passes. With only the #15571 side, every Kimi disaggregated result fails.
What I measured.
- The merge commit adds no change of its own. A new merge of
c4c7342054withmainatd9ea8c475bgives the committed tree343283eed5. - At the head, kustomize renders Kimi with
CONCURRENCIES=128andBENCHMARK_DURATION=120. It renders DeepSeek aggregated withCONCURRENCIES=128, and DeepSeek disaggregated keeps512. - I extracted the "Verify AIPerf results" step at the base and at the head. I ran each copy against 34 fixtures with
bash --noprofile --norc -e -o pipefail, the shell that the job log shows. The fixtures use the real Oct 2 exports, the realperf.yamlfiles, and the real yq.
| Fixtures | Base | Head |
|---|---|---|
| Kimi, valid c128 profile (3 fixtures, one with 1 of 200 requests) | fail | pass |
Kimi, CONCURRENCIES=128 with only a c1 profile |
pass | fail |
Kimi, CONCURRENCIES is 1,128 or 1 128 |
pass | fail |
Kimi, CONCURRENCIES missing or empty, with a c1 profile |
pass | fail |
| Kimi, valid c1 profile | pass | pass |
| Kimi, 14 bad profiles: missing or duplicate profile, wrong concurrency, errors, cancelled, zero or missing request count, zero output, empty file | fail | fail |
Kimi, CONCURRENCIES missing or *, with a c128 profile |
fail | fail |
| DeepSeek, 4 valid cases, among them c128 with 384 requests | pass | pass |
DeepSeek, the real Oct 2 sweep, wrong count, errors, missing profile, missing CONCURRENCIES |
fail | fail |
Each failure at the head stops at the expected command. Two mutants of the head step fail the fixtures. With == 1 changed to >= 1, the two-profile fixtures pass. With the old _trace_c1_ glob, the valid c128 fixtures fail.
- actionlint 1.7.12 with shellcheck 0.11.0 gives the same single error at the base and at the head: the runner label
prod-deploy-tester-v1is not inactionlint.yaml. - CI on the head: every source-branch job that ran passed, and the
rust-tests,rust-clippy,Recipe Check, and docs jobs were skipped.pull-request/15571does not exist, so the PR and deploy lanes did not run. Only the nightly or a manual dispatch runs the changed step.
sara4dev
changed the title
1c31c39 to
db33213
Compare
sara4dev
changed the title
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The DeepSeek aggregated nightly still drops the c256 and higher load points by setting
CONCURRENCIESto only128, and the current file has no linked issue or adjacent comment documenting the vLLM worker crash that the removed c256 point exposed.
2e01cdd to
5970c35
Compare
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The DeepSeek aggregated nightly still sets CONCURRENCIES to only 128, removing the c256 and higher load points where the previously reported vLLM worker crash was observed. The current patch-perf.yaml has no adjacent comment or linked issue documenting that unresolved crash, so the earlier tracking concern remains present.
- Original discussion: The DeepSeek aggregated nightly still sets
CONCURRENCIESto only128, and the file still has no adjacent comment or linked issue documenting the vLLM worker crash exposed by the removed c256-and-higher load points. - Original discussion: The DeepSeek aggregated nightly still sets
CONCURRENCIESto only128, dropping the c256 and higher load points where the vLLM worker crash was observed, and the current file still has no adjacent comment or linked issue documenting that unresolved crash. - Original discussion: The DeepSeek aggregated nightly still sets
CONCURRENCIESto only128, removing the c256 and higher load points where the previously reported vLLM worker crash was observed. I do not see an adjacent comment or linked issue documenting that unresolved crash, so the previously raised defect remains present. - Original discussion: The DeepSeek aggregated nightly still sets
CONCURRENCIESto only128, so the c256 and higher load points that exposed the vLLM worker crash remain removed, and the current file still has no adjacent comment or linked issue documenting that unresolved crash.
❌ Dynamo PR CI failed — run 37370105482 (attempt 2) on
|
| Other | Jobs |
|---|---|
| dynamo-runtime | ❌ 2 ✅ 6 |
Failure details
2 jobs failed: on both amd64 and arm64, the same 2 tests in tests/fault_tolerance/cancellation/test_utils.py fail because the streaming HTTP request gets no response within 30s (requests.exceptions.Timeout). The other 789 tests in each job passed.
❌ dynamo-runtime / test / parallel cuda13.0, amd64: 2 cancellation drained-stream tests time out (30s HTTP response)
Job: dynamo-runtime / test / parallel cuda13.0, amd64 · Failed step: Run CPU-only tests (parallelized) · Logs: gh run view --job 112000906111 -R ai-dynamo/dynamo --log-failed
tests/fault_tolerance/cancellation/test_utils.py:50: in test_drained_stream_can_require_generated_content
read_streaming_responses(
tests/fault_tolerance/cancellation/utils.py:309: in read_streaming_responses
response_raw = cancellable_req.get_response()
tests/fault_tolerance/cancellation/utils.py:188: in get_response
raise requests.exceptions.Timeout(
E requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response
FAILED tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_can_require_generated_content - requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response
FAILED tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_accepts_generated_content - requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response
=== 2 failed, 789 passed, 23 skipped, 5398 deselected in 1192.66s (0:19:52) ====
test_drained_stream_can_require_generated_content and test_drained_stream_accepts_generated_content both fail the same way, also after 3 auto-retries each: CancellableRequest.get_response() gets no HTTP response within 30s while read_streaming_responses drains the stream (drain=True, require_content=True). The later upload-artifact error (asterisk in the test_unsafe_or_nonexact_protobuf_pins[protobuf==6.33.*] log path) and the stage-verification error are follow-on noise, not the cause.
❌ dynamo-runtime / test / parallel cuda13.0, arm64: 2 cancellation drained-stream tests time out (30s HTTP response)
Job: dynamo-runtime / test / parallel cuda13.0, arm64 · Failed step: Run CPU-only tests (parallelized) · Logs: gh run view --job 112000906131 -R ai-dynamo/dynamo --log-failed
tests/fault_tolerance/cancellation/test_utils.py:50: in test_drained_stream_can_require_generated_content
read_streaming_responses(
tests/fault_tolerance/cancellation/utils.py:309: in read_streaming_responses
response_raw = cancellable_req.get_response()
tests/fault_tolerance/cancellation/utils.py:188: in get_response
raise requests.exceptions.Timeout(
E requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response
FAILED tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_can_require_generated_content - requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response
FAILED tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_accepts_generated_content - requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response
=== 2 failed, 789 passed, 23 skipped, 5398 deselected in 1434.44s (0:23:54) ====
test_drained_stream_can_require_generated_content and test_drained_stream_accepts_generated_content both fail the same way, also after 3 auto-retries each: CancellableRequest.get_response() gets no HTTP response within 30s while read_streaming_responses drains the stream (drain=True, require_content=True). The later upload-artifact error (asterisk in the test_unsafe_or_nonexact_protobuf_pins[protobuf==6.33.*] log path) and the stage-verification error are follow-on noise, not the cause.
For agents
{"pr": 15571, "run_id": 37370105482, "run_attempt": 2, "head_sha": "3ba5ced536ab74853734d28b66d46e93a9bcea12", "failures": [{"job": "dynamo-runtime / test / parallel cuda13.0, amd64", "job_id": 112000906111, "failed_step": "Run CPU-only tests (parallelized)", "signature": "requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response", "tests": ["tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_can_require_generated_content", "tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_accepts_generated_content"], "log_cmd": "gh run view --job 112000906111 -R ai-dynamo/dynamo --log-failed"}, {"job": "dynamo-runtime / test / parallel cuda13.0, arm64", "job_id": 112000906131, "failed_step": "Run CPU-only tests (parallelized)", "signature": "requests.exceptions.Timeout: Timed out after 30.0s waiting for the HTTP response", "tests": ["tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_can_require_generated_content", "tests/fault_tolerance/cancellation/test_utils.py::test_drained_stream_accepts_generated_content"], "log_cmd": "gh run view --job 112000906131 -R ai-dynamo/dynamo --log-failed"}]}Posted automatically by Devin for run 37370105482. Updated on every full-CI run of this PR.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 3ba5ced536ab74853734d28b66d46e93a9bcea12. One P2 of mine stays open. There is no P3.
- [P2]
.github/ci/deepseek-v4-pro-agg/patch-perf.yaml:27: no issue tracks the vLLM worker crash, and on Oct 4 it hit c128, the load point that this PR keeps. - Fixed:
.github/ci/kimi-k25-agg/patch-perf.yaml:24. Kimi now uses concurrency 20, and theVerify AIPerf resultsstep needs all 200 requests. - Open item for #15537: the two PRs still conflict in the
Verify AIPerf resultsstep. Keep the block of this PR and add|| "$RECIPE" == kimi-k25-disagg. With only the #15537 side, no line setsconcurrencies, and every DeepSeek result passes. - Not run yet: the changed step and settings run only in the nightly recipe jobs or in a manual dispatch.
What I measured.
- Since
db33213f83, the diff of this PR changed in three lines. KimiCONCURRENCIES"128" became "20", and.request_count.avg > 0became== 200, with a comment. The recommitted commits have the same patches as before, and the three merges ofmainadd no change of their own. A merge withmainatdef3b79b15keeps the three files of this PR unchanged. - The conflict with #15537 occurs in both merge orders, at the #15537 heads
c4f65da29cand1477b7b4d1. - I ran the step with
bash --noprofile --norc -e -o pipefailagainst 55 fixtures: the 36 of my first round, 14 new Kimi fixtures, and the real DeepSeek c128 exports of 5 nights.
| Script | Result |
|---|---|
| This head | Same as at db33213f83, except that Kimi profiles with 1, 199, or 201 requests now fail |
| Union with #15537 | Passes each valid fixture and fails each bad one, 55 of 55. Kimi disaggregated also needs 200 requests, and it reads the same trace file |
| Only the #15537 side | Passes 6 DeepSeek fixtures that must fail: the real Oct 2 sweep, the real Oct 4 c128 export, a wrong count, errors, a missing profile, and a missing CONCURRENCIES |
This head with > 0 in place of == 200 |
Passes the Kimi fixtures with 1, 199, or 201 requests |
- CI on the head: PR run 37370105482 (attempt 2) fails
dynamo-runtime / test / parallel cuda13.0on amd64 and arm64, in 2 cancellation tests. The same 2 tests fail onmainatd15ec1dda0, the merge base, in post-merge job 111954651344. This diff does not touch them. On the source branch, every check that ran passed.
Signed-off-by: Saravana Periyasamy <saperiyasamy@nvidia.com>
Signed-off-by: Saravana Periyasamy <saperiyasamy@nvidia.com>
Signed-off-by: Saravana Periyasamy <saperiyasamy@nvidia.com>
Signed-off-by: Saravana Periyasamy <saperiyasamy@nvidia.com>
3ba5ced to
da2797f
Compare
|
/ok to test da2797f |
@sara4dev, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test da2797f |
4 participants
Summary
Use one configured load point for each aggregated nightly benchmark: DeepSeek V4 Pro at concurrency 128 and Kimi K2.5 at concurrency 20. DeepSeek removes its multi-point sweep after the October 2 run crashed at concurrency 256; its validator requires 384 measured requests, zero errors and positive output.
Kimi preserves its existing trace of 20 conversations with 10 sequential turns each. Cap concurrency at 20 and require all 200 requests, zero errors, positive output and no cancellation. Select exactly one matching export using the rendered concurrency. Its 120-second duration is a maximum; trace timing and the existing ramp can yield lower achieved concurrency or earlier completion.
Validation
git diff --checkpassed. No GPU benchmark was run.