[None][fix] Bound GEN log sentinel wait - #17140
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
40e3ff1 to
9259db9
Compare
chienchunhung
changed the title
|
/bot run --disable-fail-fast |
WalkthroughThe PR adds pytest-split duration-balanced test selection to the CI performance submit flow. It also bounds generation-log sentinel waits and limits device-step-time 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 test durations
Submit->>Submit: Assign tests to least-duration groups
Submit-->>CI: Return selected test line and test name
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
| 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). | ||
| """ | ||
| sentinels = [ | ||
| os.path.join(self.test_output_dir, f"gen_server_{i}.done") | ||
| for i in range(self.num_gen_servers) | ||
| ] | ||
| start_time = time.time() | ||
| start_time = time.monotonic() | ||
| while True: | ||
| missing = [p for p in sentinels if not os.path.exists(p)] | ||
| if not missing: | ||
| print_info("All gen worker log sentinels present; log flush complete.") | ||
| return True | ||
| elapsed_time = time.time() - start_time | ||
| if elapsed_time > self.timeout: | ||
| elapsed_time = time.monotonic() - start_time | ||
| if elapsed_time > timeout: | ||
| print_info( | ||
| f"Timeout ({self.timeout}s) waiting for gen worker log " | ||
| f"Timeout ({timeout}s) waiting for gen worker log " | ||
| f"sentinels {missing}; parsing current log contents." | ||
| ) | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not report a device-step metric from an unverified log.
When wait_for_gen_log_sentinels() returns False, run_cmd() still calls parse_gen_worker_device_step_time() and appends a normal summary. The parser relies on completed sentinel files and explicitly avoids partially written or truncated logs.
If the timeout expires, skip or mark the metric unavailable unless the parser has an independent completion check. Do not present a potentially truncated value as a valid measurement.
Also applies to: 1620-1623
🤖 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 - 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.
1 participant
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
Validation
python3 -m py_compile tests/integration/defs/perf/test_perf_sanity.pyDev Engineer Review
jenkins/scripts/perf/submit.pyadds pytest-split-aware test selection.parse_test_case_nameremains backward-compatible through the optionalselected_lineargument.tests/integration/defs/perf/test_perf_sanity.pylimits GEN sentinel waits togen_onlyruns.GEN_LOG_SENTINEL_TIMEOUTprovides a dedicated 120-second bound.QA Engineer Review
tests/unittest/scripts/test_perf_submit.py.tests/integration/test_lists/,test-db/, orqa/entries changed.