[None][fix] Bound GEN log sentinel wait - #17140
Conversation
40e3ff1 to
9259db9
Compare
chienchunhung
changed the title
|
/bot run --disable-fail-fast |
|
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:
WalkthroughThe PR adds pytest-split duration-balanced test selection to the CI submit flow. It also bounds generation-log sentinel waits and limits device-step timing collection to ChangesPytest split selection
Generation-only timing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI submit flow
participant Submit as submit.py
participant TestList as Test list
participant Durations as Duration data
CI->>Submit: Provide pytestCommand and split group
Submit->>TestList: Read test cases
Submit->>Durations: Load duration data
Submit->>Submit: Assign tests to least-duration groups
Submit-->>CI: Return selected test line and test name
sequenceDiagram
participant Teardown as gen_only teardown
participant Waiter as wait_for_gen_log_sentinels
participant Logs as Generation logs
participant Output as Benchmark output
Teardown->>Waiter: Wait with bounded timeout
Waiter->>Logs: Check sentinel state
Waiter-->>Teardown: Return synchronization status
Teardown->>Logs: Parse pending client segments
Teardown->>Output: Append available device-step timing
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.
Actionable comments posted: 1
🧹 Nitpick comments (5)
jenkins/scripts/perf/submit.py (1)
645-667: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
split_groupargument.
select_test_case_linealways returns a line.parse_test_case_nametherefore always takes theselected_linebranch, andargs.split_groupis never used in that call.parse_test_case_namealso raises when the line has no bracket, so the"[" in selected_test_lineguard at line 666 is always true.♻️ Proposed simplification
config_yaml, server_name, benchmark_mode, runtime_mode = parse_test_case_name( args.test_list, args.llm_src, - args.split_group, selected_line=selected_test_line, ) with open(config_yaml, "r") as f: config = yaml.safe_load(f) - test_case_name = ( - selected_test_line.split("[")[-1].split("]")[0] if "[" in selected_test_line else "" - ) + test_case_name = selected_test_line.split("[")[-1].split("]")[0]🤖 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 645 - 667, Update the parse_test_case_name call to remove the redundant args.split_group argument and rely on selected_line. Simplify test_case_name extraction by removing the unnecessary bracket-presence guard, since select_test_case_line always returns a valid bracketed line and parse_test_case_name enforces that contract.tests/unittest/scripts/test_perf_submit.py (3)
113-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the tie-breaking and error paths.
I traced the assertion and it is correct: group 3 of 4 resolves to
test_lines[1]. However all four durations are distinct, so the name pre-sort in_select_least_duration_groupis never exercised. That pre-sort is the part most likely to diverge from pytest-split. Ties occur whenever tests are missing from.test_durations, because each such test receives the sameaverage_duration.Add cases for:
- Two or more tests with equal or unknown durations, to pin the tie order.
- A missing durations file.
- A
--splitting-algorithmvalue other thanleast_duration, which must raiseValueError.- A group that selects more than one test, which must raise
ValueError.🤖 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 - 145, Extend test_ci_submit_selects_same_least_duration_shard_as_pytest_split or add focused tests covering _select_least_duration_group: verify deterministic name-based ordering for equal durations and missing-duration entries, assert missing .test_durations handling, assert ValueError for a non-least_duration --splitting-algorithm, and assert ValueError when the selected group contains multiple tests.
113-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary.
- Changed test functions:
- Added
test_ci_submit_selects_same_least_duration_shard_as_pytest_split.- Added
test_ci_submit_rejects_split_group_disagreement.- Added the
ci_submit_modulefixture.- No test functions were modified or removed.
- Test list registration: both tests live under
tests/unittest/scripts/. Unit tests in this directory are collected by the unit-test stage and are not registered per-function intests/integration/test_lists/test-db/ortests/integration/test_lists/qa/. No list file change is required. Confirm thattests/unittest/scripts/is already inside the collected unit-test scope.- Coverage verdict: insufficient.
The happy path and one validation error are covered. The following behaviours in
select_test_case_lineand_select_least_duration_grouphave no test: equal or unknown durations (the tie order that must match pytest-split), a missing.test_durationsfile, a non-least_durationalgorithm, the positional fallback when--splitsor--groupis absent, and a group that selects more than one test. The tie case is the highest priority, because it determines whether the launcher and pytest agree on the selected test.As per path instructions "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 - 164, Add coverage for select_test_case_line and _select_least_duration_group beyond the existing happy path and group validation. Prioritize equal-duration ties and unknown durations, asserting selection order matches pytest-split; also cover missing .test_durations, non-least_duration algorithms, positional fallback without --splits or --group, and groups selecting multiple tests. Keep tests under the existing tests/unittest/scripts/ unit-test scope without modifying integration test-list registrations.Source: Path instructions
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the CI submit module by path, not by list index.
SUBMIT_PATHS[0]depends on list order. Select the path that identifies the CI implementation so reordering cannot load the local implementation.🤖 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 66 - 70, Update the ci_submit_module fixture to select the CI implementation from SUBMIT_PATHS by its identifying path or predicate instead of relying on index 0, ensuring list reordering cannot load the local implementation.tests/integration/defs/perf/test_perf_sanity.py (1)
1355-1359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new timeout arguments.
Add Google-style
Argsentries fortimeoutandpoll_interval, and aReturnsentry for the boolean result.As per coding guidelines, public function arguments must be documented in Google-style docstrings.
Suggested docstring update
"""Block until every gen worker signals that its log is fully written. + Args: + timeout: Maximum time to wait for all sentinel files. + poll_interval: Delay between sentinel checks. + + Returns: + True when all sentinels exist; otherwise False after timeout. +🤖 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 - 1359, Add a Google-style docstring to the public method wait_for_gen_log_sentinels documenting the timeout and poll_interval arguments under Args, and the boolean return value under Returns. Keep the existing behavior and default values unchanged.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 `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 1368-1391: Update run_cmd and the corresponding path around
parse_gen_worker_device_step_time to check the result of
wait_for_gen_log_sentinels before parsing or appending the device-step metric.
When the sentinel wait returns False, skip the metric or mark it unavailable
unless an independent completion check confirms the log is complete; never
report a parsed value as a normal measurement from an unverified log.
---
Nitpick comments:
In `@jenkins/scripts/perf/submit.py`:
- Around line 645-667: Update the parse_test_case_name call to remove the
redundant args.split_group argument and rely on selected_line. Simplify
test_case_name extraction by removing the unnecessary bracket-presence guard,
since select_test_case_line always returns a valid bracketed line and
parse_test_case_name enforces that contract.
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 1355-1359: Add a Google-style docstring to the public method
wait_for_gen_log_sentinels documenting the timeout and poll_interval arguments
under Args, and the boolean return value under Returns. Keep the existing
behavior and default values unchanged.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 113-145: Extend
test_ci_submit_selects_same_least_duration_shard_as_pytest_split or add focused
tests covering _select_least_duration_group: verify deterministic name-based
ordering for equal durations and missing-duration entries, assert missing
.test_durations handling, assert ValueError for a non-least_duration
--splitting-algorithm, and assert ValueError when the selected group contains
multiple tests.
- Around line 113-164: Add coverage for select_test_case_line and
_select_least_duration_group beyond the existing happy path and group
validation. Prioritize equal-duration ties and unknown durations, asserting
selection order matches pytest-split; also cover missing .test_durations,
non-least_duration algorithms, positional fallback without --splits or --group,
and groups selecting multiple tests. Keep tests under the existing
tests/unittest/scripts/ unit-test scope without modifying integration test-list
registrations.
- Around line 66-70: Update the ci_submit_module fixture to select the CI
implementation from SUBMIT_PATHS by its identifying path or predicate instead of
relying on index 0, ensuring list reordering cannot load the local
implementation.
🪄 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: 1716d257-3da3-4026-b114-4a40031784fe
📒 Files selected for processing (3)
jenkins/scripts/perf/submit.pytests/integration/defs/perf/test_perf_sanity.pytests/unittest/scripts/test_perf_submit.py
|
/bot run --disable-fail-fast |
|
PR_Github #63247 [ run ] triggered by Bot. Commit: |
|
PR_Github #63247 [ run ] completed with state |
|
Held on one concrete behavior change.
Smaller: The rest checks out. |
|
@BowenFu Thanks for catching that; agreed on the sentinel-timeout behavior. I restored the bounded current-log fallback in 66ab918: gen-only still waits up to 120 seconds for the completion sentinels, but a timeout now parses the GEN logs already on disk instead of automatically omitting the metric. If no usable metric is present, the existing I’m keeping the missing-durations failure intentionally. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/scripts/test_perf_sanity_helpers.py (1)
29-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to both functions.
Annotate
test_sentinel_timeout_falls_back_to_current_gen_logsandparse_device_step_time. The Python coding guidelines require annotations on every function.Proposed change
-def test_sentinel_timeout_falls_back_to_current_gen_logs(monkeypatch, tmp_path): +def test_sentinel_timeout_falls_back_to_current_gen_logs( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: @@ - parse_calls = [] + parse_calls: list[tuple[str, int, list[int]]] = [] - def parse_device_step_time(output_dir, num_gen_servers, start_offsets): + def parse_device_step_time( + output_dir: str, + num_gen_servers: int, + start_offsets: list[int], + ) -> float:🤖 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_sanity_helpers.py` around lines 29 - 61, 添加类型注解到测试函数 test_sentinel_timeout_falls_back_to_current_gen_logs 及其内部函数 parse_device_step_time，涵盖参数和返回值，并遵循项目现有的类型标注风格。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.
Nitpick comments:
In `@tests/unittest/scripts/test_perf_sanity_helpers.py`:
- Around line 29-61: 添加类型注解到测试函数
test_sentinel_timeout_falls_back_to_current_gen_logs 及其内部函数
parse_device_step_time，涵盖参数和返回值，并遵循项目现有的类型标注风格。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e446bcbf-e66d-4810-b223-6567b74cde59
📒 Files selected for processing (2)
tests/integration/defs/perf/test_perf_sanity.pytests/unittest/scripts/test_perf_sanity_helpers.py
66ab918 to
ee22c04
Compare
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
|
PR_Github #63815 [ run ] triggered by Bot. Commit: |
|
PR_Github #63815 [ run ] completed with state |
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>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
1cefe9c to
8733174
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #63847 [ run ] triggered by Bot. Commit: |
|
PR_Github #63847 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64084 [ run ] triggered by Bot. Commit: |
|
PR_Github #64084 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64130 [ run ] triggered by Bot. Commit: |
|
PR_Github #64130 [ run ] completed with state |
The perf-sanity disagg gen_only cases were waived after builds 2873/2884/2876 recorded "Test terminated unexpectedly" (nvbugs 6541361 / 6542166 / 6561566 / 6550133 / 6551021). Those were not workload failures: the gen_only benchmark PASSED end-to-end (all ranks exit 0, "N passed", benchmark_status="Done"), then the GEN slurm step idled ~76-78 min in teardown until the 90-min stage watchdog killed it, emitting results-timeout.xml with an empty passed_test_list. Root cause was the gen-log sentinel wait added in 99bdffc (NVIDIA#16717): wait_for_gen_log_sentinels() polled gen_server_{i}.done under a timeout of DEFAULT_TIMEOUT (10800s), longer than the 5400s stage watchdog, so a stuck / un-reaped multi-node gen srun turned an optional log-flush sync into a stage cancellation. A companion defect derived testOutputDir from a positional test-list index that could disagree with the pytest-split shard, so the sentinel was polled in the wrong directory. Both defects are fixed and merged to main: - NVIDIA#17134 (721c46b) aligns the launcher's testOutputDir with the pytest shard. - NVIDIA#17140 (22184ba) bounds the wait to GEN_LOG_SENTINEL_TIMEOUT (120s, well below the watchdog), falls back to parsing the current gen log on timeout, and gates the whole path on benchmark_mode == "gen_only". This removes 15 perf/test_perf_sanity.py disagg gen_only SKIP waivers whose bugs tracked only the harness defect above, so post-merge re-runs those cases against the fixed harness. Four gen_only waivers are deliberately KEPT, because their bugs track genuine workload failures that the harness fixes do not address: - gb200_deepseek-v32-fp4_32k4k_con2048_..._eplb288_mtp1 (nvbugs/6374872) - gb200_deepseek-v32-fp4_8k1k_con4096_..._eplb256_mtp0 (nvbugs/6490049) - gb200_gpt-oss-120b-fp4_8k1k_con4_..._tp4_eplb0_mtp0 (nvbugs/6490049) - gb300_kimi-k25-thinking-fp4_8k1k_con1024_..._mtp3 (nvbugs/6490049) Non-gen_only perf-sanity waivers (ctx_only, e2e, aggregated) are left untouched. In particular the e2e gb300_kimi-k25-thinking con4096 waiver that this branch originally removed is no longer part of this change: NVIDIA#16920 (b21a3b4) already unwaived that exact case on main under nvbugs/6490049. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
8 participants
Summary
srundoes not reapProblem
#16717 waits for
gen_server_<n>.donebefore parsing GEN worker logs. The launcher creates that file only after the enclosing GENsrunreturns. Recent targeted and main post-merge runs completed the workload and wrotebenchmark_status=Done, but the GENsrundid not finish reaping, so the benchmark consumed the pytest timeout waiting for a non-critical log-flush sentinel. The benchmark step then exited nonzero and Slurm's--kill-on-bad-exit=1cancelled the remaining job.The sentinel remains the preferred path. This change prevents it from turning a post-workload teardown delay into a one-hour pytest/Slurm cancellation.
Stack dependency
This PR is intentionally stacked on and depends on #17134. Its branch includes #17134's pytest-split shard-alignment commits so the multi-node perf launcher selects the same duration-balanced test node that pytest runs. Without that parent fix, validation can prepare configuration and output paths for a different shard than the one executed by pytest.
Merge order: #17134 → #17140 → #16920. Although this PR still targets
mainin GitHub, reviewers can evaluate the #17140-specific delta relative to #17134. PR #16920 is restacked above this PR for the targeted GB300 validation.Validation
python3 -m py_compile tests/integration/defs/perf/test_perf_sanity.pyDev Engineer Review
submit.pyadds pytest-split duration-based test selection.gen_onlymodes skip GEN metric collection.QA Engineer Review
test_ci_submit_selects_same_least_duration_shard_as_pytest_split.test_ci_submit_selector_matches_installed_pytest_split.test_ci_submit_rejects_invalid_least_duration_groups.test_ci_submit_ignores_test_list_comments.test_ci_submit_rejects_split_group_disagreement.test_ci_submit_rejects_missing_pytest_split_durations.test_sentinel_timeout_falls_back_to_current_gen_logs.test-db/andqa/lists coverperf/test_perf_sanity.py, but coverage for the new unit tests is not shown.