Skip to content

fix(config): propagate export level to benchmark plans - #1452

Open
shm197 wants to merge 3 commits into
ai-dynamo:mainfrom
shm197:shm197/fix-convergence-export-level
Open

shm197 wants to merge 3 commits into
ai-dynamo:mainfrom
shm197:shm197/fix-convergence-export-level

Conversation

@shm197

@shm197 shm197 commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

fix(config): propagate export level to benchmark plans

Summary

  • Copy the resolved benchmark artifact export level into BenchmarkPlan during plan assembly.
  • Add regression coverage for CLI configuration resolution and YAML loading with summary, records, and raw exports.
  • Document the YAML artifact settings needed for distribution convergence.

Problem

Multi-run distribution convergence requires per-request JSONL data. Although
--export-level records and --export-level raw correctly populate the benchmark's
artifact configuration, plan assembly leaves BenchmarkPlan.export_level at its
default, summary.

As a result, convergence validation rejects valid configurations before any
benchmark runs:

--convergence-mode distribution requires per-request JSONL data, but --export-level is set to 'summary'. Use --export-level records or --export-level raw.

This change propagates the resolved export level rather than weakening that
validation. Genuine summary-only exports remain rejected for distribution
convergence. No convergence algorithms or thresholds are changed.

Validation

Base: e34f7e59c2df5bb20042ed99cda36b5d4c6d6733.

  • Before the fix: the four new records/raw cases failed because the plan used
    summary; both summary-only rejection cases passed.
  • After the fix: 79 focused tests passed, including all six new regression cases,
    existing plan tests, convergence validation, and distribution criterion tests.
uv run --offline --no-sync python -m pytest \
  tests/unit/config/test_benchmark_plan.py \
  tests/unit/config/test_benchmark_plan_shape.py \
  tests/unit/orchestrator/test_cli_convergence.py::TestValidateConvergenceConfig \
  tests/unit/orchestrator/convergence/test_distribution.py \
  -n 0 -p no:rerunfailures --timeout=30 --tb=short -q
  • Changed-file pre-commit static checks passed. Repository-wide generation and
    import hooks were skipped locally; license, baselined Ruff, and ergonomics
    checks were run directly on the changed files and passed.
  • Tests ran serially with numerical-library thread counts limited to one. The
    rerun plugin was disabled because its local socket initialization is restricted
    in the test sandbox.
  • No model requests were made. The full suite and live-service end-to-end
    distribution convergence were not run locally.

Summary by CodeRabbit

  • Bug Fixes

    • Distribution convergence now consistently validates artifact export settings for CLI and YAML configurations.
    • Summary-only exports are rejected when incompatible with distribution convergence; record and raw exports remain supported.
    • Sweep configurations validate every expanded variation before execution and identify incompatible settings.
    • Dynamically proposed search configurations are validated before trials begin, preventing incompatible proposals from being dispatched.
  • Documentation

    • Added guidance for configuring JSONL records and optional raw payload exports in YAML.
    • Documented summary-only export behavior when record and raw payload options are disabled.

Signed-off-by: Shiwei Ma <mashiwei19971226@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown

Try out this PR

Quick install:

pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@d384ad56bcd5488ca68d61cea8651cf2badb6ec6

Recommended with virtual environment (using uv):

uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@d384ad56bcd5488ca68d61cea8651cf2badb6ec6

Last updated for commit: d384ad5Browse code

@github-actions github-actions Bot added the fix label Sep 20, 2026
@shm197
shm197 marked this pull request as ready for review September 20, 2026 02:22
@shm197
shm197 requested a review from a team as a code owner September 20, 2026 02:22
@shm197
shm197 requested a review from ajcasagrande September 20, 2026 02:22
@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/aiperf/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 10332e48-880b-45da-82fa-39d81c3b9d28

📥 Commits

Reviewing files that changed from the base of the PR and between 86eb52e and d384ad5.

📒 Files selected for processing (4)
  • docs/tutorials/multi-run-confidence.md
  • src/aiperf/cli_runner/_strategy.py
  • src/aiperf/orchestrator/orchestrator.py
  • tests/unit/orchestrator/test_adaptive_convergence_exports.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The change propagates artifact export levels into BenchmarkPlan, validates static, sweep, and adaptive configurations for distribution convergence, adds coverage, and documents YAML export rules.

Changes

Distribution Export Validation

Layer / File(s) Summary
Plan export-level propagation
src/aiperf/config/loader/plan.py
BenchmarkPlan now stores config.benchmark.artifacts.export_level.
Per-configuration convergence validation
src/aiperf/cli_runner/_strategy.py
Distribution validation checks each benchmark configuration and rejects summary-only export.
Adaptive proposal validation
src/aiperf/orchestrator/orchestrator.py, tests/unit/orchestrator/test_adaptive_convergence_exports.py
Adaptive search validates each proposed configuration before execution and reports the search iteration index for rejected proposals.
Configuration coverage and documentation
tests/unit/config/test_benchmark_plan.py, tests/unit/orchestrator/test_cli_convergence.py, docs/tutorials/multi-run-confidence.md
Tests cover CLI, YAML, grid, zip, scenario, and adaptive configurations. The tutorial documents YAML export settings and sweep validation behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: propagating the artifact export level into benchmark plans.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI

A rabbit checks the records in a row
Summary-only paths now clearly show
Sweeps pause before they run
Raw and JSONL join the fun
New plans guide each trial where to go

Comment @coderabbitai help to get the list of available commands.

Comment thread src/aiperf/config/loader/plan.py
Signed-off-by: Shiwei Ma <mashiwei19971226@gmail.com>
Comment thread src/aiperf/cli_runner/_strategy.py Outdated
Signed-off-by: Shiwei Ma <mashiwei19971226@gmail.com>
True, "records", [["jsonl"], False], ["records", "summary"],
id="summary-last",
),
param(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary-first sweep row only rejects a summary-only variation at config 1, which is already protected by the retained single-config summary export coverage. The sweep-specific regression is distinguished by the retained summary-last row, because that fails if validation checks only the base or first config; summary-base-overridden and mixed-records-raw preserve the supported non-summary paths.

🤖 AI Fix

Remove the summary-first parameter row from this sweep-export matrix.

param("distribution", False, [1, 1], None, id="raw-points"),
param("distribution", True, [0, 1], None, id="mixed-records-raw"),
param("ci_width", False, [1, 0], None, id="ci-width-allows-summary"),
param("cv", False, [1, 0], None, id="cv-allows-summary"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cv-allows-summary adaptive-search case exercises the same runtime export-validation branch as the retained ci-width-allows-summary case: any non-distribution convergence mode may keep summary exports. CV strategy construction is already covered separately, and this test only needs one non-distribution convergence case plus the fixed-trials case to preserve the supported behavior.

🤖 AI Fix

Drop the cv-allows-summary parameter case from this adaptive-search export-validation test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant