[None][fix] Align perf launcher with pytest shard - #17134
Conversation
chienchunhung
changed the title
|
/bot run --disable-fail-fast |
|
PR_Github #63099 [ run ] triggered by Bot. Commit: |
05880f6 to
7a2e0ec
Compare
|
/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 performance launcher uses pytest-split duration data to select one test for each split group. It validates split settings and reuses the selected test line for test-name and output-directory generation. Tests compare selection with pytest’s algorithm and cover invalid inputs. ChangesPytest-split test selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant select_test_case_line
participant pytestCommand
participant durationFile
participant testList
main->>select_test_case_line: provide script prefix and split group
select_test_case_line->>pytestCommand: parse split and duration options
select_test_case_line->>durationFile: load test durations
select_test_case_line->>testList: parse candidate tests
select_test_case_line-->>main: return one selected test line
main->>parse_test_case_name: reuse selected line
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (3)
jenkins/scripts/perf/submit.py (2)
77-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the
pytestCommandline lookup.
_pytest_command_tokensandget_pytest_commands(Line 536) each locate theexport pytestCommand=line with their ownnext(...)expression. Extract one helper that returns the line. Both call sites then stay in sync if the prefix format changes.🤖 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 - 86, Extract the shared pytestCommand export-line lookup from _pytest_command_tokens and get_pytest_commands into a helper that returns the matching line or an empty value. Update both functions to use this helper, preserving their existing parsing and fallback behavior.
211-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider removing the second selection path.
mainalways passesselected_line, so thesplit_groupbranch at Lines 221-226 is unused in the CI flow. Two selection paths for one concept can diverge after a later change. Makeselected_linerequired, or have this fallback callselect_test_case_lineinstead of re-implementing positional selection.🤖 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 211 - 228, The parse_test_case_name selection logic duplicates positional test selection even though main always supplies selected_line. Make selected_line required and remove the test_list_path, split_group, and fallback branch, or delegate fallback selection to select_test_case_line instead of indexing lines directly; preserve the existing selected-line parsing behavior.tests/unittest/scripts/test_perf_submit.py (1)
113-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary
Changed test functions:
test_ci_submit_selects_same_least_duration_shard_as_pytest_split(added) — verifies least-duration group selection and the--durations-pathfallback tollm_src.test_ci_submit_rejects_split_group_disagreement(added) — verifies the--groupconsistency check.Test list registration: these are unit tests under
tests/unittest/scripts/, not integration tests, so no entry undertests/integration/test_lists/test-db/ortests/integration/test_lists/qa/is required. Confirm thattests/unittest/scripts/is already collected by an existing L0 unit-test stage.Verdict: sufficient for the primary fix, with these gaps worth closing:
- Equal durations. With
--splits 4and four tests, every group receives one test, so the tie-break order atsubmit.pyLine 155 is not exercised. Add a case with two identical durations.len(selected) != 1. Add a case where one group receives two tests, and assert the "requires exactly one test"ValueError.- Missing
--splits/--group. Add a case that asserts the positional fallback still returnslines[split_group - 1].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 - 145, Extend tests around select_test_case_line and test_ci_submit_selects_same_least_duration_shard_as_pytest_split to cover equal-duration tie-breaking, asserting the expected deterministic group selection. Add a case where one group contains multiple tests and assert the exact-one-test ValueError. Add a case without --splits or --group and verify positional fallback returns lines[split_group - 1]. Confirm the existing L0 unit-test collection includes tests/unittest/scripts/.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 `@jenkins/scripts/perf/submit.py`:
- Around line 107-131: Update _load_pytest_split_durations so a missing
durations file is not silently converted to an empty dictionary: emit a clear
warning containing durations_path, or propagate an error when split_group > 0.
Preserve normal loading and validation for existing files, and ensure callers
can identify that duration-based selection is unavailable.
- Around line 134-165: Pin pytest-split to exactly version 0.10.0 in
requirements-dev.txt, preserving the existing dependency entry while adding the
explicit version constraint so CI uses the algorithm matched by
_select_least_duration_group.
---
Nitpick comments:
In `@jenkins/scripts/perf/submit.py`:
- Around line 77-86: Extract the shared pytestCommand export-line lookup from
_pytest_command_tokens and get_pytest_commands into a helper that returns the
matching line or an empty value. Update both functions to use this helper,
preserving their existing parsing and fallback behavior.
- Around line 211-228: The parse_test_case_name selection logic duplicates
positional test selection even though main always supplies selected_line. Make
selected_line required and remove the test_list_path, split_group, and fallback
branch, or delegate fallback selection to select_test_case_line instead of
indexing lines directly; preserve the existing selected-line parsing behavior.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 113-145: Extend tests around select_test_case_line and
test_ci_submit_selects_same_least_duration_shard_as_pytest_split to cover
equal-duration tie-breaking, asserting the expected deterministic group
selection. Add a case where one group contains multiple tests and assert the
exact-one-test ValueError. Add a case without --splits or --group and verify
positional fallback returns lines[split_group - 1]. Confirm the existing L0
unit-test collection includes tests/unittest/scripts/.
🪄 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: 272c70dc-8d52-4c8a-aac5-c830373da971
📒 Files selected for processing (2)
jenkins/scripts/perf/submit.pytests/unittest/scripts/test_perf_submit.py
|
PR_Github #63099 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #63248 [ run ] triggered by Bot. Commit: |
|
PR_Github #63248 [ 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>
c018511 to
63f2737
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
jenkins/scripts/perf/submit.py (2)
213-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider removing the duplicate selection path in
parse_test_case_name.
parse_test_case_namenow holds a second selection mechanism.mainalways passesselected_line, so theelsebranch and thesplit_groupbounds check are unreachable in production. That branch duplicates the index-based fallback and the bounds check ofselect_test_case_line, and it can drift from the duration-balanced selection.If no caller needs the legacy path, require
selected_lineand droptest_list_pathandsplit_group. Keep the change small if the unit tests still use the legacy signature.♻️ Proposed simplification
-def parse_test_case_name(test_list_path, llm_src, split_group=0, selected_line=None): - """Parse the selected line of the test list. +def parse_test_case_name(llm_src, selected_line): + """Parse the selected test-list line. Returns (config_yaml_path, server_name, benchmark_mode, runtime_mode). See the module docstring for the supported test name shapes. """ - if selected_line is not None: - line = selected_line - else: - lines = _read_test_list_lines(test_list_path) - if split_group > 0: - if split_group > len(lines): - raise ValueError( - f"split_group {split_group} exceeds number of tests in test list ({len(lines)})" - ) - line = lines[split_group - 1] - else: - line = lines[0] + line = selected_lineUpdate the call in
mainaccordingly:config_yaml, server_name, benchmark_mode, runtime_mode = parse_test_case_name( args.llm_src, selected_test_line, )🤖 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 213 - 230, Remove the unreachable legacy selection path from parse_test_case_name by requiring selected_line and dropping test_list_path and split_group from its signature, then update main to pass only args.llm_src and selected_test_line. If unit tests still depend on the legacy signature, preserve compatibility there while keeping production selection delegated to select_test_case_line.
136-167: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin
pytest-splitto0.10.0.requirements-dev.txtcurrently leaves it unversioned, so a dependency upgrade can change shard assignment.unittest/scriptsis already included in thel0_a10CI test list.🤖 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 136 - 167, The dependency configuration must pin pytest-split to version 0.10.0 so shard assignment remains stable across upgrades. Update the pytest-split entry in requirements-dev.txt, preserving the existing unittest/scripts CI coverage.
🤖 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 `@jenkins/scripts/perf/submit.py`:
- Around line 69-74: Update _read_test_list_lines to exclude lines whose
stripped content is a comment beginning with “#”, matching parse_test_list_lines
behavior before validating and returning the test list. Preserve filtering of
blank lines and the existing empty-list ValueError.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 33-38: The added functions in _FakePytestItem and the test cases
lack complete type annotations. Add -> None to _FakePytestItem.__init__ and
every added test function, and annotate each fixture parameter with its precise
fixture-provided type.
- Around line 157-183: Extend
test_ci_submit_selector_matches_installed_pytest_split to cover invalid selector
boundaries by asserting _select_least_duration_group rejects splits=0, group=0,
and group values greater than splits. Use the test’s existing lines and
durations, and verify the expected validation exception for each case.
- Around line 18-23: Pin the pytest-split dependency to version 0.10.0 in
requirements-dev.txt, ensuring CI resolves that exact version rather than
relying on the attribution entry or an unbounded dependency.
- Around line 122-223: Add focused tests for _select_least_duration_group
covering splits less than 1, group less than 1, and group greater than splits,
asserting each raises the expected validation error. Keep these cases direct
against the helper so they do not rely on select_test_case_line or get masked by
split-group disagreement handling.
---
Nitpick comments:
In `@jenkins/scripts/perf/submit.py`:
- Around line 213-230: Remove the unreachable legacy selection path from
parse_test_case_name by requiring selected_line and dropping test_list_path and
split_group from its signature, then update main to pass only args.llm_src and
selected_test_line. If unit tests still depend on the legacy signature, preserve
compatibility there while keeping production selection delegated to
select_test_case_line.
- Around line 136-167: The dependency configuration must pin pytest-split to
version 0.10.0 so shard assignment remains stable across upgrades. Update the
pytest-split entry in requirements-dev.txt, preserving the existing
unittest/scripts CI coverage.
🪄 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: 9fddf316-e66e-449e-979b-caa10896fd55
📒 Files selected for processing (2)
jenkins/scripts/perf/submit.pytests/unittest/scripts/test_perf_submit.py
|
/bot run --disable-fail-fast |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #63517 [ run ] triggered by Bot. Commit: |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #63523 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jenkins/scripts/perf/submit.py (1)
174-186: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject partial pytest-split configuration.
When only one of
--splitsor--groupis present, raise an error. Use positional selection only when both options are absent.🤖 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 174 - 186, Update select_test_case_line so it raises a ValueError when exactly one of splits_option or group_option is present; only use positional selection when both --splits and --group are absent, while preserving the existing behavior when both are provided.
🤖 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.
Outside diff comments:
In `@jenkins/scripts/perf/submit.py`:
- Around line 174-186: Update select_test_case_line so it raises a ValueError
when exactly one of splits_option or group_option is present; only use
positional selection when both --splits and --group are absent, while preserving
the existing behavior when both are provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd219d68-ffa4-455e-8ad6-6059591a6beb
📒 Files selected for processing (1)
jenkins/scripts/perf/submit.py
|
PR_Github #63517 [ run ] completed with state |
|
PR_Github #63526 [ run ] triggered by Bot. Commit: |
|
PR_Github #63523 [ run ] completed with state |
|
PR_Github #63526 [ run ] completed with state
|
3 participants
Summary
Root cause
The launcher interpreted
--split-group Nas the Nth line in the test list. Pytest instead applies pytest-split'sleast_durationalgorithm using.test_durations, so a shard can select a different test from the raw Nth line.That allowed the launcher to configure servers and the output directory for one test while pytest executed another test. During the NVBUG#6481034 investigation, this caused the targeted Kimi shard to write and collect artifacts under a different test identity.
Fix
Parse the pytest-split options from the generated
pytestCommand, load the same duration data, and mirror the installed pytest-splitLeastDurationAlgorithm. The launcher now validates that its group matches pytest's group and requires one selected test for each multi-node perf shard. A contract unit test compares every selected group with the installed plugin across skewed, equal, missing, irrelevant, and empty duration data, so dependency upgrades fail visibly if the behavior changes.This PR contains no transfer tracing, runtime product changes, KV-cache fraction changes, timeout changes, test configuration changes, or waiver changes.
Validation
pre-commit run --files jenkins/scripts/perf/submit.py tests/unittest/scripts/test_perf_submit.pypython -m py_compile jenkins/scripts/perf/submit.py tests/unittest/scripts/test_perf_submit.pypython -m pytest -q --confcutdir=tests/unittest/scripts tests/unittest/scripts/test_perf_submit.py -k ci_submit— 8 passed against unpinned pytest-split 0.11.0Related investigation: NVBUG#6481034 and #16918.
Dev Engineer Review
jenkins/scripts/perf/submit.py..test_durations, validates split-group consistency, and requires one selected test per shard.QA Engineer Review
pytest-splitcompatibility, invalid split and group values, test-list comments, split-group disagreement, and missing duration files.tests/integration/test_lists/test-db/orqa/.