[https://nvbugs/6490049][test] Unwaive GB300 Kimi disagg e2e - #16920
[https://nvbugs/6490049][test] Unwaive GB300 Kimi disagg e2e#16920chienchunhung wants to merge 5 commits into
Conversation
|
/bot run --disable-fail-fast --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2,GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-4" |
|
PR_Github #62031 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2,GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-4" |
|
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:
WalkthroughAdds duration-balanced pytest-split performance test selection. The submit flow reuses the selected test for configuration and output naming. Generation-log synchronization now has a dedicated timeout and applies metric collection only to ChangesPytest-split performance selection
Generation-log synchronization
Performance waiver management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SubmitScript
participant PytestCommand
participant DurationData
participant ShardSelector
SubmitScript->>PytestCommand: Parse split options
SubmitScript->>DurationData: Load test durations
DurationData-->>ShardSelector: Provide test durations
PytestCommand-->>ShardSelector: Provide split and group values
ShardSelector-->>SubmitScript: Return one selected test
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot kill |
|
PR_Github #62033 [ kill ] triggered by Bot. Commit: |
|
PR_Github #62031 [ run ] completed with state |
|
PR_Github #62033 [ kill ] completed with state |
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
Test-only: perf-sanity YAML gains kv_transfer_timeout_ms: 600000 (matching the equivalent GB200 Kimi config) and the two matching waivers come out. No product source, no default change — kv_transfer_timeout_ms is an existing per-config knob and the global 60s default is untouched. The single YAML serves both the e2e and gen_only variants, so both unwaived cases are covered.
Two notes, neither blocking:
- The targeted GB300 stages were aborted rather than green, so the e2e case (waived under the broader nvbugs/6490049) isn't yet proven to pass for this reason alone.
- #16921 removes the same
disagg_upload-e2e-gb300_kimi-...con4096...waiver line, so whichever lands second will need a rebase.
|
/bot run --disable-fail-fast --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2,GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-4" |
chienchunhung
changed the title
|
PR_Github #62861 [ run ] triggered by Bot. Commit: |
|
PR_Github #62861 [ run ] completed with state
|
86a1701 to
506174a
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-1" |
|
PR_Github #63102 [ run ] triggered by Bot. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/scripts/test_perf_submit.py (1)
46-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fixture loader logic.
ci_submit_module(46-55) duplicates the module-loading logic already insubmit_module(34-43), only hardcodingSUBMIT_PATHS[0]instead ofrequest.param. Consider derivingci_submit_modulefromsubmit_moduleor extracting the shared spec-loading logic into a helper function.🤖 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/unittest/scripts/test_perf_submit.py` around lines 46 - 55, Refactor ci_submit_module to reuse the existing submit_module fixture or a shared module-loading helper instead of duplicating importlib loading logic. Preserve CI selection through SUBMIT_PATHS[0], while keeping submit_module’s request.param-based behavior unchanged.jenkins/scripts/perf/submit.py (1)
649-671: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate bracket-extraction logic.
test_case_nameextraction (lines 669-671) reimplements the same bracket-splitting logic already used inparse_test_case_name(lines 214-217). If the bracket-parsing rule changes, both sites need updating, so they can silently drift apart.Extract a small shared helper, e.g.
_bracket_content(line), and use it in both places.🤖 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 `@jenkins/scripts/perf/submit.py` around lines 649 - 671, The bracket-content parsing is duplicated between parse_test_case_name and the test_case_name assignment. Extract the bracket-splitting behavior into a shared _bracket_content(line) helper, then reuse it in both locations while preserving the existing empty-string result when no brackets are present.
🤖 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 `@jenkins/scripts/perf/submit.py`:
- Around line 649-671: The bracket-content parsing is duplicated between
parse_test_case_name and the test_case_name assignment. Extract the
bracket-splitting behavior into a shared _bracket_content(line) helper, then
reuse it in both locations while preserving the existing empty-string result
when no brackets are present.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 46-55: Refactor ci_submit_module to reuse the existing
submit_module fixture or a shared module-loading helper instead of duplicating
importlib loading logic. Preserve CI selection through SUBMIT_PATHS[0], while
keeping submit_module’s request.param-based behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7544ded5-a11a-4633-a476-e8054a0ff2c0
📒 Files selected for processing (3)
jenkins/scripts/perf/submit.pytests/integration/test_lists/waives.txttests/unittest/scripts/test_perf_submit.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
506174a to
75ca24f
Compare
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…meout Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
75ca24f to
e0bfe69
Compare
|
/bot run --disable-fail-fast --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-1" |
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-1" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/integration/defs/perf/test_perf_sanity.py (1)
1355-1395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd deterministic coverage for sentinel handling and mode gating.
Add tests for all sentinels present, sentinel timeout,
gen_only, and non-gen_onlydisaggregated mode. On timeout, assert no metric only when the available logs contain no usable rows; the caller intentionally parses usable rows after a timeout. List each new test in the appropriatetest-dborqafile, such astests/integration/test_lists/qa/llm_perf_multinode.txt.🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 1355 - 1395, Add deterministic tests covering all sentinels present, sentinel timeout, gen_only mode, and non-gen_only disaggregated mode, using the existing performance test helpers and mocking filesystem/time as needed. Verify timeout behavior only reports no metric when available logs contain no usable rows, while preserving metric parsing when usable rows exist. Register each test in the appropriate test-db or QA list, including the relevant llm_perf_multinode.txt entry.Source: Path instructions
jenkins/scripts/perf/submit.py (1)
77-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
pytestCommandline lookup.
_pytest_command_tokensre-implements the samenext((line for line in script_prefix_lines if "export pytestCommand=" in line), ...)pattern used later inget_pytest_commands(Line 536-538) andget_test_output_dir(Line 584-586). Extract a shared helper, for example_find_pytest_command_line(script_prefix_lines), and reuse it in all three call sites. This keeps the marker string in one place.♻️ Proposed refactor
+def _find_pytest_command_line(script_prefix_lines): + return next( + (line for line in script_prefix_lines if "export pytestCommand=" in line), "" + ) + + def _pytest_command_tokens(script_prefix_lines): - pytest_command_line = next( - (line for line in script_prefix_lines if "export pytestCommand=" in line), "" - ) + pytest_command_line = _find_pytest_command_line(script_prefix_lines) if not pytest_command_line: return []🤖 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 `@jenkins/scripts/perf/submit.py` around lines 77 - 95, Extract the repeated pytestCommand line lookup into a shared helper such as _find_pytest_command_line(script_prefix_lines), keeping the export pytestCommand marker in that helper. Update _pytest_command_tokens, get_pytest_commands, and get_test_output_dir to use the helper while preserving their existing fallback and parsing behavior.tests/unittest/scripts/test_perf_submit.py (1)
113-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest coverage summary.
- Added test functions:
ci_submit_modulefixture,test_ci_submit_selects_same_least_duration_shard_as_pytest_split,test_ci_submit_rejects_split_group_disagreement.- These are unit tests under
tests/unittest/scripts/, not integration tests undertests/integration/, so listing intests/integration/test_lists/test-db/orqa/does not apply.- Coverage verdict: sufficient for the happy-path least-duration selection and the
--group/split_groupmismatch guard. The happy-path test validates internal self-consistency of_select_least_duration_group(its output matches its own greedy-heap logic) but does not independently validate against a realpytest --collect-onlyrun withpytest-splitinstalled. Consider one additional test exercising the fallback path in_load_pytest_split_durationswhere--durations-pathpoints to a missing file and the code falls back tollm_src/tests/integration/defs/<basename>, since that fallback is exercised implicitly here but not asserted directly (only the JSON contents, not the path fallback itself, are checked). Optional test-coverage improvement, not blocking.As per path instructions, produce a coverage summary for tests/** changes: "Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Always produce a test coverage summary, even if no issues are found."
🤖 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/unittest/scripts/test_perf_submit.py` around lines 113 - 146, Add a focused unit test for the fallback branch in _load_pytest_split_durations, verifying that a missing --durations-path loads the matching basename from the local llm_src/tests/integration/defs directory. Keep the existing least-duration and split-group tests unchanged, and assert the fallback path is actually used rather than only validating equivalent JSON contents.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.
Inline comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 1368-1373: Update the callers of wait_for_gen_log_sentinels(),
including the run_cmd() path and the corresponding logic around the later
referenced block, to check its boolean result before parsing gen_server logs.
Only compute the device-step-time metric when every sentinel exists; otherwise
skip it or mark it unavailable through the existing result-handling path, and do
not consume or parse partial log contents.
---
Nitpick comments:
In `@jenkins/scripts/perf/submit.py`:
- Around line 77-95: Extract the repeated pytestCommand line lookup into a
shared helper such as _find_pytest_command_line(script_prefix_lines), keeping
the export pytestCommand marker in that helper. Update _pytest_command_tokens,
get_pytest_commands, and get_test_output_dir to use the helper while preserving
their existing fallback and parsing behavior.
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 1355-1395: Add deterministic tests covering all sentinels present,
sentinel timeout, gen_only mode, and non-gen_only disaggregated mode, using the
existing performance test helpers and mocking filesystem/time as needed. Verify
timeout behavior only reports no metric when available logs contain no usable
rows, while preserving metric parsing when usable rows exist. Register each test
in the appropriate test-db or QA list, including the relevant
llm_perf_multinode.txt entry.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 113-146: Add a focused unit test for the fallback branch in
_load_pytest_split_durations, verifying that a missing --durations-path loads
the matching basename from the local llm_src/tests/integration/defs directory.
Keep the existing least-duration and split-group tests unchanged, and assert the
fallback path is actually used rather than only validating equivalent JSON
contents.
🪄 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: 4443d3ba-85d4-4fb5-81fa-c7cb4330fa1f
📒 Files selected for processing (4)
jenkins/scripts/perf/submit.pytests/integration/defs/perf/test_perf_sanity.pytests/integration/test_lists/waives.txttests/unittest/scripts/test_perf_submit.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
| Returns True once all sentinels exist, or False if the dedicated | ||
| sentinel timeout is reached first. On False the caller still parses | ||
| whatever is on disk: the sentinel is a correctness optimization | ||
| against reading a mid-flush log, not a reason to consume the whole-test | ||
| timeout and trigger Slurm's kill-on-bad-exit cascade (nvbugs 6487036 / | ||
| 6487040 / 6487038). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not parse a log after the sentinel timeout.
wait_for_gen_log_sentinels() returns False when a gen worker can still flush gen_server_{i}.log. run_cmd() discards that result and parses the current file contents. This can report a partial device-step-time metric and produce an incorrect perf-sanity result.
Skip the metric, or mark it unavailable through the existing result path, unless every sentinel exists.
Proposed fix
if pending_device_step_time:
- self.wait_for_gen_log_sentinels()
- for record in pending_device_step_time:
+ if self.wait_for_gen_log_sentinels():
+ for record in pending_device_step_time:
device_step_time_mean = parse_gen_worker_device_step_time(
self.test_output_dir,
self.num_gen_servers,
start_offsets=record["start_offsets"],
)
+ else:
+ print_info("Skipping device-step-time parsing because gen logs are not flushed.")Also applies to: 1620-1639
🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 1368 - 1373,
Update the callers of wait_for_gen_log_sentinels(), including the run_cmd() path
and the corresponding logic around the later referenced block, to check its
boolean result before parsing gen_server logs. Only compute the device-step-time
metric when every sentinel exists; otherwise skip it or mark it unavailable
through the existing result-handling path, and do not consume or parse partial
log contents.
|
PR_Github #63114 [ run ] triggered by Bot. Commit: |
|
PR_Github #63115 [ run ] triggered by Bot. Commit: |
|
PR_Github #63114 [ run ] completed with state |
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-1" |
|
PR_Github #63102 [ run ] completed with state |
|
PR_Github #63118 [ run ] triggered by Bot. Commit: |
|
PR_Github #63115 [ run ] completed with state |
|
PR_Github #63118 [ run ] completed with state |
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-3" |
|
PR_Github #63135 [ run ] triggered by Bot. Commit: |
chienchunhung
changed the title
|
PR_Github #63135 [ run ] completed with state |
6 participants
Summary
e2eperf-sanity case tracked by NVBUG 6490049kv_transfer_timeout_ms=60000; this PR has no net timeout overridegen_onlycase unchanged; that case is tracked separately by NVBUG 6487038Why
mainstill skips this exacte2ecase under NVBUG 6490049. The correspondinggen_onlycase is already unwaived onmainby #16717, so it is not part of this PR's waiver diff.This PR was originally opened during the NVBUG 6487038 investigation. After rebasing onto the changes from #16717 and #16832, the remaining net waiver change is specifically the
e2ecase owned by NVBUG 6490049. The PR is therefore retargeted to that bug and does not claim to fix or verify the GEN-only teardown path.Stack
e2e, where the GEN-only device-step-time metric is not collectedThe verification below is for the combined stack at commit
e0bfe69.Verification
A targeted run with test reuse disabled passed the exact unwaived test:
e0bfe69c484e2986528bca0ec6dfab1632d263c2GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-1perf/test_perf_sanity.py::test_e2e[disagg_upload-e2e-gb300_kimi-k25-thinking-fp4_8k1k_con4096_ctx1_dep4_gen1_dep16_eplb0_mtp0_ccb-NIXL]PASSED;1 passed, 2 skipped, 4153 deselected1941.99s; pytest total:1951.11sThe YAML has no explicit GEN or CTX
kv_transfer_timeout_msoverride, so this validation uses the Python transceiver's existing 60-second default.Scope
The PR removes one
e2ewaiver. It does not change the global KV-transfer timeout, add a GB300 timeout override, or modify the inherited GEN-only waiver state.