feat: add parameter sweeping support

[Lokiiiiii]

Summary

This PR introduces comprehensive parameter sweeping functionality to AIPerf, enabling users to benchmark across a static parameter list (e.g., concurrency levels) in a single run. The implementation includes new aggregation strategies, exporters, extensive test coverage, and complete documentation. This builds on top of #623 which introduced multi-run orchestration.

Changes

Core Features

• Parameter Sweep Orchestration: New sweep strategy in orchestrator that executes benchmarks across parameter combinations
• Sweep Aggregation: Aggregate metrics across parameter values with statistical analysis (mean, std, CV, confidence intervals)
• Best Configuration Detection: Automatic identification of optimal parameter values for throughput and latency
• Pareto Optimal Analysis: Multi-objective optimization to find non-dominated parameter configurations

Configuration & CLI

• Enhanced LoadgenConfig with sweep parameter validation and combination generation
• Support for both repeated and independent sweep modes with multi-run confidence intervals
• Backward compatibility maintained for single benchmark run

Documentation

• Complete parameter sweeping tutorial with examples and use cases
• API reference for sweep aggregate JSON/CSV formats
• Troubleshooting guide for common parameter sweep errors

Future Work

• Plugin: Will be refactored as a plugin in the next PR
• Dashboard: Dashboard support for Multi-run benchmarks will come in the future

Summary by CodeRabbit

Release Notes

• New Features

  • Added parameter sweep capability to run benchmarks across multiple parameter values sequentially or independently.
  • Introduced Pareto optimal configuration identification and best configuration tracking for sweep results.
  • Added sweep modes (repeated/independent) with configurable cooldowns between sweep values.

• Documentation

  • Added comprehensive parameter sweeping tutorial and API reference guide.
  • Added parameter sweeping troubleshooting guide covering common errors and solutions.
  • Updated CLI documentation with new sweep options and parameter sweep section.

• Improvements

  • Renamed internal trial naming from "run_" to "trial_" prefix for better clarity.
  • Enhanced configuration validation to prevent incompatible sweep combinations.

[github-actions]

Try out this PR

Quick install:

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

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@d6d96061eaaeb948b7c8bd9fae44ce2a6df5355b

---

Last updated for commit: d6d9606 • Browse code

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Introduces parameter sweeping functionality for AIPerf with new CLI options (--parameter-sweep-mode, --concurrency list support), sweep result aggregation and exporters, orchestration support for sweep and confidence combinations, Pareto-optimal analysis, updated run-to-trial naming throughout, comprehensive documentation, and extensive test coverage.

Changes

Cohort / File(s)	Summary
Build & Environment .gitignore, pyproject.toml	Added hypothesis dependency and ignore patterns for temporary directories.
Core Documentation docs/api/sweep-aggregates.md, docs/cli_options.md, docs/metrics_reference.md, docs/troubleshooting/parameter-sweeping-errors.md	Added comprehensive API reference and troubleshooting guides for parameter sweeping; updated CLI options documentation with new sweep parameters; updated example naming from run_ to trial_ prefix.
Tutorial Documentation docs/tutorials/parameter-sweeping.md, docs/tutorials/multi-run-confidence.md, docs/tutorials/plot.md	Added parameter sweeping tutorial covering modes, seeds, cooldowns, and Pareto analysis; updated multi-run and plot tutorials to use new trial_ naming convention.
Configuration & Validation src/aiperf/common/config/loadgen_config.py, src/aiperf/common/config/user_config.py, src/aiperf/common/config/groups.py	Added sweep parameter fields (mode, cooldown, same_seed), concurrency list parsing, and comprehensive validation for sweep configurations; added Parameter Sweep group constant; enhanced concurrency and prefill validation to support list forms.
Aggregation Core src/aiperf/orchestrator/aggregation/sweep.py, src/aiperf/orchestrator/aggregation/__init__.py	Implemented Pareto-optimal configuration analysis and sweep aggregation computation with support for multi-objective optimization; exported new aggregation components.
Aggregation Exporters src/aiperf/exporters/aggregate/aggregate_sweep_csv_exporter.py, src/aiperf/exporters/aggregate/aggregate_sweep_json_exporter.py, src/aiperf/exporters/aggregate/__init__.py	Added JSON and CSV exporters for sweep aggregation results with per-combination metrics, best configurations, and Pareto optimal points; exported new exporter classes.
Orchestration & Execution src/aiperf/orchestrator/orchestrator.py, src/aiperf/orchestrator/strategies.py, src/aiperf/orchestrator/models.py, src/aiperf/cli_runner.py	Expanded orchestration to support parameter sweeps combined with confidence trials; introduced ParameterSweepStrategy with seed derivation and cooldown handling; updated FixedTrialsStrategy with trial_ naming; added UI compatibility validation; enhanced multi-run execution path with aggregation and export logic; added metadata field to RunResult.
Test Configuration & Validation tests/unit/common/config/test_loadgen_config_validators.py, tests/unit/common/config/test_user_config.py	Expanded validation tests for concurrency lists, sweep parameters, and incompatible configurations; added error message text updates.
Test Aggregation & Exporters tests/unit/exporters/aggregate/test_sweep_exporters.py, tests/unit/orchestrator/aggregation/test_sweep.py, tests/unit/orchestrator/test_orchestrator_aggregation.py, tests/unit/orchestrator/test_orchestrator_sweep_aggregation.py	Added comprehensive tests for sweep exporters, Pareto analysis, aggregation computation, and orchestrator aggregation/export workflows.
Test Orchestration & Strategies tests/unit/orchestrator/test_orchestrator.py, tests/unit/orchestrator/test_orchestrator_execution.py, tests/unit/orchestrator/test_strategies.py, tests/unit/orchestrator/test_parameter_sweep_properties.py, tests/unit/test_cli_runner.py, tests/unit/test_cli_runner_aggregation.py	Updated orchestrator tests to use explicit label/path parameters; added comprehensive execution and strategy composition tests; expanded CLI runner tests for sweep UI validation; added property-based tests for sweep behavior.
Integration Tests tests/integration/test_multi_run_confidence.py, tests/unit/plot/test_mode_detector.py	Updated multi-run confidence tests to use trial_ naming convention; refined mode detector test filtering.
Test Utilities tests/harness/utils.py	Enhanced error message formatting to conditionally include STDERR/STDOUT contents.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 Whiskers twitch with glee so bright,
Parameter sweeps dance through the night,
Pareto points bloom like garden greens,
Trial by trial, the best machines!
Cooldowns soothe, and seeds align,
This feature makes the benchmarks shine! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: add parameter sweeping support' clearly and concisely describes the main feature addition introduced in this comprehensive pull request.
Docstring Coverage	✅ Passed	Docstring coverage is 99.20% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (6)

tests/unit/plot/test_mode_detector.py (1)

244-250: Consider filtering by _is_run_directory instead of hardcoding __pycache__.

The current filter only excludes __pycache__ but could still pick up other system-generated directories (e.g., .DS_Store, .pytest_cache) as children[0], making the test fragile in different environments.

♻️ Proposed refactor

-        # Get first child run directory (excluding __pycache__)
-        children = [
-            d
-            for d in parent_dir_with_runs.iterdir()
-            if d.is_dir() and d.name != "__pycache__"
-        ]
+        # Get first valid child run directory
+        children = [
+            d
+            for d in parent_dir_with_runs.iterdir()
+            if mode_detector._is_run_directory(d)
+        ]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/plot/test_mode_detector.py` around lines 244 - 250, The test
currently builds children by filtering out only "__pycache__", which is fragile;
change the list comprehension that defines children to use the project helper
predicate _is_run_directory (i.e., filter with _is_run_directory(d)) instead of
d.name != "__pycache__" so only valid run directories under parent_dir_with_runs
are returned and child = children[0] picks a real run directory.

docs/cli_options.md (1)

670-673: Optional: fix the fragment sentence.

"Can be combined with --request-rate to control the request rate." is missing a subject ("It can be…").

📝 Proposed fix

-Number of concurrent requests to maintain OR list of concurrency values for parameter sweep. AIPerf issues a new request immediately when one completes, maintaining this level of in-flight requests. Can be combined with `--request-rate` to control the request rate. When a list is provided (e.g., [10, 20, 30]), AIPerf runs benchmarks sequentially for each value.
+Number of concurrent requests to maintain OR list of concurrency values for parameter sweep. AIPerf issues a new request immediately when one completes, maintaining this level of in-flight requests. It can be combined with `--request-rate` to control the request rate. When a list is provided (e.g., [10, 20, 30]), AIPerf runs benchmarks sequentially for each value.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/cli_options.md` around lines 670 - 673, The sentence fragment under the
`--concurrency` option should be made grammatically complete: change "Can be
combined with `--request-rate` to control the request rate." to include a
subject (for example, "It can be combined with `--request-rate` to control the
request rate.") so the description for the `--concurrency` option reads as a
full sentence and clearly references combination with `--request-rate`.

tests/unit/orchestrator/test_orchestrator.py (1)

551-669: New tests are thorough; note that _collect_failed_sweep_values hardcodes "concurrency".

The four new tests correctly validate the current implementation. However, _collect_failed_sweep_values (line 973–976 in orchestrator.py) checks only for "concurrency" in metadata and sets param_name = "concurrency" unconditionally, with an explicit comment: "Currently only concurrency is supported." The codebase includes get_sweep_parameter() in LoadGenConfig (returns the parameter name dynamically), but _collect_failed_sweep_values does not use it. This means request_rate or other sweep parameters would silently fail without being collected.

Consider either extending the function to use get_sweep_parameter() and detect any sweep parameter dynamically, or adding a test that explicitly documents this limitation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_orchestrator.py` around lines 551 - 669, The
helper _collect_failed_sweep_values currently hardcodes the sweep parameter name
("concurrency"), so failures for other sweep params are ignored; update
_collect_failed_sweep_values to call LoadGenConfig.get_sweep_parameter() (via
the orchestrator's config/service_config) to determine the sweep parameter name
dynamically, use that returned name instead of the literal "concurrency" when
inspecting result.metadata and populating param_name, and keep a safe fallback
(e.g., skip if get_sweep_parameter() is None) to preserve existing behavior for
non-sweep runs; ensure references to _collect_failed_sweep_values and
get_sweep_parameter are used so tests for request_rate or other parameters will
be picked up.

src/aiperf/exporters/aggregate/aggregate_sweep_json_exporter.py (1)

11-52: Align the docstring/output schema (per_value_metrics vs per_combination_metrics).

The docstring and example payload mention per_value_metrics and trends, but the actual output uses per_combination_metrics and does not emit trends. Either update the docstring to match the output or include the missing section to avoid confusion for downstream consumers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/exporters/aggregate/aggregate_sweep_json_exporter.py` around lines
11 - 52, The docstring for the aggregate sweep JSON exporter is inconsistent
with the actual output produced by _generate_content: update the documentation
to match the code (or vice‑versa). Specifically, in the class docstring and the
_generate_content docblock replace references to "per_value_metrics" and
"trends" with the actual keys used by the implementation (e.g.,
"per_combination_metrics" and omit or mark "trends" as not emitted), or modify
the _generate_content implementation to emit "per_value_metrics" and include a
"trends" section from result.metadata; ensure references to result.metrics,
result.metadata, best_configurations, and pareto_optimal in _generate_content
remain consistent with the chosen scheme so consumers and docs match.

src/aiperf/common/config/user_config.py (1)

337-368: Avoid re-reading mooncake trace files during sweep validation.

validate_timing_mode() already calls _should_use_fixed_schedule_for_mooncake_trace(). The new sweep validator calls it again, causing a second file scan. Consider reusing the earlier result (e.g., self._timing_mode == TimingMode.FIXED_SCHEDULE combined with CustomDatasetType.MOONCAKE_TRACE) to avoid extra blocking I/O.

♻️ Suggested refactor

-            # Also check if mooncake_trace will auto-enable fixed schedule
-            if self._should_use_fixed_schedule_for_mooncake_trace():
+            # Also check if mooncake_trace already auto-enabled fixed schedule
+            if (
+                self.input.custom_dataset_type == CustomDatasetType.MOONCAKE_TRACE
+                and self._timing_mode == TimingMode.FIXED_SCHEDULE
+            ):
                 param_name, param_values = sweep_param
                 raise ValueError(
                     f"Parameter sweeps (e.g., --{param_name} {','.join(map(str, param_values))}) cannot be used with mooncake_trace datasets "

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/common/config/user_config.py` around lines 337 - 368, The sweep
validator validate_sweep_incompatibilities currently calls
_should_use_fixed_schedule_for_mooncake_trace() causing a second scan of
mooncake trace files; instead reuse the result computed earlier by
validate_timing_mode by checking self._timing_mode == TimingMode.FIXED_SCHEDULE
and the dataset type (CustomDatasetType.MOONCAKE_TRACE) rather than calling
_should_use_fixed_schedule_for_mooncake_trace() again; update
validate_sweep_incompatibilities to reference self._timing_mode and dataset type
to decide incompatibility, and remove the extra call to
_should_use_fixed_schedule_for_mooncake_trace() to avoid duplicate blocking I/O.

src/aiperf/orchestrator/orchestrator.py (1)

115-117: Silence unused config parameters.
Ruff flags these as unused; renaming to _config is the smallest fix.

Suggested change

-def _export_confidence_aggregate(
-    self, aggregate_result: Any, config: UserConfig
-) -> None:
+def _export_confidence_aggregate(
+    self, aggregate_result: Any, _config: UserConfig
+) -> None:
...
-def _export_sweep_aggregates(
-    self,
-    per_value_aggregates: dict[int, Any],
-    sweep_aggregate: Any,
-    config: UserConfig,
-) -> None:
+def _export_sweep_aggregates(
+    self,
+    per_value_aggregates: dict[int, Any],
+    sweep_aggregate: Any,
+    _config: UserConfig,
+) -> None:

Also applies to: 159-164

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 115 - 117, Rename the
unused parameter name config to _config in the method definition of
_export_confidence_aggregate (def _export_confidence_aggregate(self,
aggregate_result: Any, _config: UserConfig) -> None:) to silence the linter;
apply the same rename for the other function(s) flagged (change any unused
parameter named config to _config) and keep the type annotations intact so only
the parameter name changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/troubleshooting/parameter-sweeping-errors.md`:
- Line 10: Several fenced code blocks that show plain error/warning output are
missing a language specifier; update each plain output fence from ``` to ```text
so Markdown linter (MD040) recognizes them as text. Locate the ten
"error-message" code fences in the document (the blocks that display CLI
errors/warnings) and change their opening fence to ```text and keep the existing
closing fence unchanged; ensure you apply the same change as in the example
snippet that shows the Invalid concurrency value message.

In `@docs/tutorials/parameter-sweeping.md`:
- Around line 188-223: The tutorial's directory tree shows sweep_aggregate/
nested under aggregate/, but the API doc shows sweep_aggregate/ as a sibling of
aggregate/; update the tutorial snippet so sweep_aggregate/ is a sibling (i.e.,
move the sweep_aggregate/ block out of the aggregate/ block) to match the
runtime output and the API reference; modify the tree under the example
artifacts/ → ... → profile_runs/ aggregate/ and sweep_aggregate/ entries so both
appear at the same indentation level.
- Line 383: Update the example description for "pareto_optimal" to match the
actual schema: instead of describing it as a list of ints like "pareto_optimal":
[10, 30, 40], show it as a list of objects with concurrency keys e.g.
[{"concurrency": 10}, {"concurrency": 30}, {"concurrency": 40}] and update the
explanatory text so it matches usage elsewhere in this file (see the
programmatic example that uses [p['concurrency'] for p in pareto_optimal]) and
the API reference in docs/api/sweep-aggregates.md.
- Around line 780-795: Update the CLI examples to use the actual option names
documented in the CLI reference: replace `--gpu-telemetry-url` with
`--gpu-telemetry` and `--server-metrics-url` with `--server-metrics` in the
`aiperf profile` examples so the URL strings are passed as list values (e.g.,
`--gpu-telemetry http://...` and `--server-metrics http://...`), ensuring the
examples match the parser that recognizes `--gpu-telemetry` and
`--server-metrics`.

In `@docs/tutorials/plot.md`:
- Line 21: Replace the phrase "not compatible with" with the single-word
"incompatible with" in the Plot CLI docs text that describes the Multi-Run
Profile case (the sentence mentioning the plot CLI, the --num-profile-runs > 1
option, and the profile_runs/ directory structure such as
artifacts/my_run/profile_runs/trial_0001/); update the sentence so it reads "The
plot CLI is incompatible with the `--num-profile-runs > 1` option." to satisfy
the LanguageTool style nit.

In `@src/aiperf/common/config/loadgen_config.py`:
- Around line 21-100: The parse_concurrency_list field_validator for concurrency
must normalize list inputs so string numbers don't later cause TypeErrors: when
v is a list in parse_concurrency_list, coerce each element to int (validating
each is an integer >= 1) and return the int list, and if any element fails
conversion or is <= 0 raise a clear ValueError mentioning the offending element
and expected format; also apply the same >=1 validation for the single-int
return path from string parsing so parse_concurrency_list consistently returns
either an int (>=1), a list[int] (all >=1), or raises a descriptive ValueError.

In `@src/aiperf/exporters/aggregate/aggregate_sweep_csv_exporter.py`:
- Around line 160-177: The _format_number function is missing a parameter type
annotation for value; add a proper type hint such as value: Optional[float] or
value: Optional[Union[int, float]] (or use float | int | None for 3.10+),
update/import typing (Optional/Union) as needed, and keep the existing return
type str so the signature becomes e.g. def _format_number(self, value:
Optional[Union[int, float]], decimals: int = 2) -> str: to satisfy the project's
"type hints on ALL functions" rule while preserving current behavior in
_format_number.

In `@src/aiperf/exporters/aggregate/aggregate_sweep_json_exporter.py`:
- Around line 5-86: Replace usage of json.dumps in
AggregateSweepJsonExporter._generate_content with orjson: add an import for
orjson at the top, then call orjson.dumps(output, option=orjson.OPT_INDENT_2)
and decode the returned bytes to a str (e.g., .decode("utf-8")) to preserve
indentation and UTF-8 output; update the import removal of json if no longer
used and keep changes scoped to class AggregateSweepJsonExporter and its
_generate_content method.

In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 182-195: The code currently infers the sweep parameter but
continues to use hardcoded "concurrency" elsewhere; determine and persist the
parameter name (param_name) once and replace every hardcoded
"concurrency"/assumed key with this variable in all downstream
logic—specifically update _export_sweep_aggregates,
_has_multiple_trials_per_value, _aggregate_per_sweep_value,
_compute_sweep_aggregates, and the sweep-only branch in _execute_strategy to
read/write metadata and build keys/paths using param_name (and fall back to
"request_rate" or default where detection fails); ensure grouping, filename/path
composition, and aggregate computations reference aggregate.metadata[param_name]
rather than aggregate.metadata["concurrency"] so request_rate (and future
parameters) are handled correctly.
- Around line 79-83: Remove or rephrase the “what” comments that simply restate
code around the calls to execute and _aggregate_and_export in orchestrator.py;
either delete the two-line comment block entirely or replace it with a short
rationale explaining why aggregation/export happens after execution (e.g., to
ensure results are complete and final before aggregation). Locate the comment
near the calls to self.execute(base_config, strategy) and
self._aggregate_and_export(results, base_config) and update it accordingly.
- Around line 228-235: The nested async function export_all_per_value lacks a
return type annotation; add an explicit return type (e.g., ->
List[YourExportResultType] or -> List[Any] if the concrete result type is
unknown) to comply with the "type hints on ALL functions" rule, and import the
necessary typing symbol(s) (List, Any or the specific type) at the top of the
module; reference export_per_value and per_value_aggregates to determine the
correct element/result type to use for the list return annotation.

In `@tests/unit/common/config/test_loadgen_config_validators.py`:
- Around line 264-399: Rename the three misleading test functions to reflect
that they expect a ValidationError: change
test_parameter_sweep_mode_with_single_concurrency_succeeds ->
test_parameter_sweep_mode_with_single_concurrency_raises_error,
test_parameter_sweep_cooldown_with_single_concurrency_succeeds ->
test_parameter_sweep_cooldown_with_single_concurrency_raises_error, and
test_parameter_sweep_same_seed_with_single_concurrency_succeeds ->
test_parameter_sweep_same_seed_with_single_concurrency_raises_error; update
their docstrings to match the new names and ensure any references (e.g., in test
collection or comments) use the new names so they follow the test naming
convention test_<function>_<scenario>_<expected>.

In `@tests/unit/common/config/test_user_config.py`:
- Around line 1549-1670: Rename the test methods in the
TestConcurrencyListParsing class to follow the
test_<function>_<scenario>_<expected> convention; for example rename
test_parse_single_integer_string to
test_parse_concurrency_single_integer_string_returns_int,
test_parse_large_values to test_parse_concurrency_large_values_returns_list, and
test_parse_many_values to test_parse_concurrency_many_values_returns_list, and
similarly adjust other methods like test_parse_comma_separated_list ->
test_parse_concurrency_comma_separated_list_returns_list and
test_parse_single_integer -> test_parse_concurrency_single_integer_returns_int
so all test method names reflect function (concurrency parsing), scenario, and
expected outcome.

In `@tests/unit/exporters/aggregate/test_sweep_exporters.py`:
- Around line 124-577: Rename the test functions to follow the
test_<function>_<scenario>_<expected> convention: update names like
test_json_export_creates_file -> test_export_json_creates_file_file_created,
test_json_export_schema_compliance ->
test_export_json_schema_compliant_valid_data, test_json_export_with_failed_runs
-> test_export_json_with_failed_runs_includes_failed_runs,
test_json_export_creates_directory ->
test_export_json_creates_directory_directory_created,
test_csv_export_creates_file -> test_export_csv_creates_file_file_created,
test_csv_export_format -> test_export_csv_format_contains_sections,
test_csv_per_combination_metrics_table ->
test_export_csv_per_combination_metrics_table_correct_values,
test_csv_best_configurations_section ->
test_export_csv_best_configurations_section_present,
test_csv_pareto_optimal_section ->
test_export_csv_pareto_optimal_section_present, test_csv_metadata_section ->
test_export_csv_metadata_section_present, test_csv_number_formatting ->
test_export_csv_number_formatting_two_decimal_places,
test_csv_empty_pareto_optimal ->
test_export_csv_empty_pareto_optimal_reports_none,
test_both_exporters_produce_consistent_data ->
test_exporters_consistency_json_csv_matching_values,
test_exporters_handle_minimal_data ->
test_exporters_handle_minimal_data_no_exceptions; keep pytest.mark.asyncio and
signatures unchanged and update any references/imports or test names used
elsewhere (e.g., in fixtures or CI filters). Ensure names remain unique and
descriptive.

---

Nitpick comments:
In `@docs/cli_options.md`:
- Around line 670-673: The sentence fragment under the `--concurrency` option
should be made grammatically complete: change "Can be combined with
`--request-rate` to control the request rate." to include a subject (for
example, "It can be combined with `--request-rate` to control the request
rate.") so the description for the `--concurrency` option reads as a full
sentence and clearly references combination with `--request-rate`.

In `@src/aiperf/common/config/user_config.py`:
- Around line 337-368: The sweep validator validate_sweep_incompatibilities
currently calls _should_use_fixed_schedule_for_mooncake_trace() causing a second
scan of mooncake trace files; instead reuse the result computed earlier by
validate_timing_mode by checking self._timing_mode == TimingMode.FIXED_SCHEDULE
and the dataset type (CustomDatasetType.MOONCAKE_TRACE) rather than calling
_should_use_fixed_schedule_for_mooncake_trace() again; update
validate_sweep_incompatibilities to reference self._timing_mode and dataset type
to decide incompatibility, and remove the extra call to
_should_use_fixed_schedule_for_mooncake_trace() to avoid duplicate blocking I/O.

In `@src/aiperf/exporters/aggregate/aggregate_sweep_json_exporter.py`:
- Around line 11-52: The docstring for the aggregate sweep JSON exporter is
inconsistent with the actual output produced by _generate_content: update the
documentation to match the code (or vice‑versa). Specifically, in the class
docstring and the _generate_content docblock replace references to
"per_value_metrics" and "trends" with the actual keys used by the implementation
(e.g., "per_combination_metrics" and omit or mark "trends" as not emitted), or
modify the _generate_content implementation to emit "per_value_metrics" and
include a "trends" section from result.metadata; ensure references to
result.metrics, result.metadata, best_configurations, and pareto_optimal in
_generate_content remain consistent with the chosen scheme so consumers and docs
match.

In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 115-117: Rename the unused parameter name config to _config in the
method definition of _export_confidence_aggregate (def
_export_confidence_aggregate(self, aggregate_result: Any, _config: UserConfig)
-> None:) to silence the linter; apply the same rename for the other function(s)
flagged (change any unused parameter named config to _config) and keep the type
annotations intact so only the parameter name changes.

In `@tests/unit/orchestrator/test_orchestrator.py`:
- Around line 551-669: The helper _collect_failed_sweep_values currently
hardcodes the sweep parameter name ("concurrency"), so failures for other sweep
params are ignored; update _collect_failed_sweep_values to call
LoadGenConfig.get_sweep_parameter() (via the orchestrator's
config/service_config) to determine the sweep parameter name dynamically, use
that returned name instead of the literal "concurrency" when inspecting
result.metadata and populating param_name, and keep a safe fallback (e.g., skip
if get_sweep_parameter() is None) to preserve existing behavior for non-sweep
runs; ensure references to _collect_failed_sweep_values and get_sweep_parameter
are used so tests for request_rate or other parameters will be picked up.

In `@tests/unit/plot/test_mode_detector.py`:
- Around line 244-250: The test currently builds children by filtering out only
"__pycache__", which is fragile; change the list comprehension that defines
children to use the project helper predicate _is_run_directory (i.e., filter
with _is_run_directory(d)) instead of d.name != "__pycache__" so only valid run
directories under parent_dir_with_runs are returned and child = children[0]
picks a real run directory.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e358413 and 12ecf19b4adcbcf62a75cfe16c332e3b38ec1aaf.

📒 Files selected for processing (34)

• .gitignore
• docs/api/sweep-aggregates.md
• docs/cli_options.md
• docs/metrics_reference.md
• docs/troubleshooting/parameter-sweeping-errors.md
• docs/tutorials/multi-run-confidence.md
• docs/tutorials/parameter-sweeping.md
• docs/tutorials/plot.md
• pyproject.toml
• src/aiperf/cli_runner.py
• src/aiperf/common/config/groups.py
• src/aiperf/common/config/loadgen_config.py
• src/aiperf/common/config/user_config.py
• src/aiperf/exporters/aggregate/__init__.py
• src/aiperf/exporters/aggregate/aggregate_sweep_csv_exporter.py
• src/aiperf/exporters/aggregate/aggregate_sweep_json_exporter.py
• src/aiperf/orchestrator/aggregation/__init__.py
• src/aiperf/orchestrator/aggregation/sweep.py
• src/aiperf/orchestrator/models.py
• src/aiperf/orchestrator/orchestrator.py
• src/aiperf/orchestrator/strategies.py
• tests/harness/utils.py
• tests/integration/test_multi_run_confidence.py
• tests/integration/test_parameter_sweep.py
• tests/unit/common/config/test_loadgen_config_validators.py
• tests/unit/common/config/test_user_config.py
• tests/unit/exporters/aggregate/test_sweep_exporters.py
• tests/unit/orchestrator/aggregation/test_sweep.py
• tests/unit/orchestrator/test_orchestrator.py
• tests/unit/orchestrator/test_parameter_sweep_properties.py
• tests/unit/orchestrator/test_strategies.py
• tests/unit/plot/test_mode_detector.py
• tests/unit/test_cli_runner.py
• tests/unit/test_cli_runner_aggregation.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Sweep metadata is hardcoded to concurrency, which breaks non‑concurrency sweeps.
ParameterSweepStrategy is parameter‑agnostic, but metadata/aggregation/export paths assume concurrency (or request_rate) and the composed paths always write concurrency. This will misclassify request_rate sweeps and skip/incorrectly compute aggregates. Store the sweep parameter name explicitly and use it everywhere downstream.

Suggested change (core pattern)

-                result.metadata.update(
-                    {
-                        "trial_index": trial_index,
-                        "value_index": value_index,
-                        "concurrency": run_config.loadgen.concurrency,
-                        "sweep_mode": "repeated",
-                    }
-                )
+                param_name = sweep_strategy.parameter_name
+                param_value = getattr(run_config.loadgen, param_name)
+                result.metadata.update(
+                    {
+                        "trial_index": trial_index,
+                        "value_index": value_index,
+                        "parameter_name": param_name,
+                        param_name: param_value,
+                        "sweep_mode": "repeated",
+                    }
+                )
...
-                result.metadata.update(
-                    {
-                        "trial_index": trial_index,
-                        "value_index": value_index,
-                        "concurrency": run_config.loadgen.concurrency,
-                        "sweep_mode": "independent",
-                    }
-                )
+                param_name = sweep_strategy.parameter_name
+                param_value = getattr(run_config.loadgen, param_name)
+                result.metadata.update(
+                    {
+                        "trial_index": trial_index,
+                        "value_index": value_index,
+                        "parameter_name": param_name,
+                        param_name: param_value,
+                        "sweep_mode": "independent",
+                    }
+                )
...
-            if not result.success and "concurrency" in result.metadata:
-                param_value = result.metadata.get("concurrency")
-                param_name = "concurrency"  # Currently only concurrency is supported
+            param_name = result.metadata.get("parameter_name")
+            if not result.success and param_name and param_name in result.metadata:
+                param_value = result.metadata.get(param_name)
...
-        for r in results:
-            for key in ["concurrency", "request_rate"]:
-                if key in r.metadata:
-                    sweep_param_values.add(r.metadata[key])
-                    break
+        for r in results:
+            param_name = r.metadata.get("parameter_name")
+            if param_name and param_name in r.metadata:
+                sweep_param_values.add(r.metadata[param_name])

Please apply the same parameter_name-based lookup in _export_sweep_aggregates, _has_multiple_trials_per_value, _aggregate_per_sweep_value, _compute_sweep_aggregates, and the sweep-only path in _execute_strategy so request_rate (and future params) don’t silently drop out of aggregation/export.

Also applies to: 497-506, 587-593, 712-718, 973-987, 1067-1073, 1092-1099, 1131-1137, 1205-1217

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 182 - 195, The code
currently infers the sweep parameter but continues to use hardcoded
"concurrency" elsewhere; determine and persist the parameter name (param_name)
once and replace every hardcoded "concurrency"/assumed key with this variable in
all downstream logic—specifically update _export_sweep_aggregates,
_has_multiple_trials_per_value, _aggregate_per_sweep_value,
_compute_sweep_aggregates, and the sweep-only branch in _execute_strategy to
read/write metadata and build keys/paths using param_name (and fall back to
"request_rate" or default where detection fails); ensure grouping, filename/path
composition, and aggregate computations reference aggregate.metadata[param_name]
rather than aggregate.metadata["concurrency"] so request_rate (and future
parameters) are handled correctly.

[codecov]

Codecov Report

❌ Patch coverage is 86.89384% with 100 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/aiperf/orchestrator/orchestrator.py	85.63%	31 Missing and 22 partials ⚠️
src/aiperf/cli_runner.py	53.84%	14 Missing and 4 partials ⚠️
...xporters/aggregate/aggregate_sweep_csv_exporter.py	83.33%	6 Missing and 6 partials ⚠️
src/aiperf/common/config/loadgen_config.py	87.64%	6 Missing and 5 partials ⚠️
src/aiperf/orchestrator/aggregation/sweep.py	93.90%	3 Missing and 2 partials ⚠️
src/aiperf/common/config/user_config.py	96.77%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (3)

docs/tutorials/parameter-sweeping.md (1)

351-433: ⚠️ Potential issue | 🟡 Minor

Incomplete fix: JSON example, ASCII chart, and caption still contradict the (now-correct) Pareto text.

The past review's Pareto fix was only partially applied. Three locations still disagree with the updated explanation at lines 383 and 399–402:

1. Lines 351–355 – pareto_optimal JSON is [10, 30, 40]; should include {"concurrency": 20}.
2. Line 423 – ASCII chart marks concurrency 20 as ○ (dominated by 30); it should be ● (Pareto optimal).
3. Line 433 – Caption "Point 20 (○) is dominated because 30 is better on both axes" is factually wrong (concurrency 30 latency = 180.5 ms > 20's 145.2 ms, so 30 does not dominate 20).

Additionally, line 757 in the Best Practices section lists Pareto optimal points: 10, 30, 40, also omitting 20.

📝 Proposed fix across all four locations

Lines 351–355 – add concurrency 20 to JSON array:

 "pareto_optimal": [
     {"concurrency": 10},
+    {"concurrency": 20},
     {"concurrency": 30},
     {"concurrency": 40}
 ]

Line 423 – fix ASCII chart marker:

-180 |        ○ 20 (dominated by 30)
+180 |        ● 20 (Pareto optimal)

Line 433 – fix caption:

-Points on the frontier (●) are Pareto optimal. Point 20 (○) is dominated because 30 is better on both axes.
+All four points (●) are Pareto optimal. No configuration strictly dominates another on both objectives simultaneously.

Line 757 – fix Best Practices example:

-- Pareto optimal points: 10, 30, 40
+- Pareto optimal points: 10, 20, 30, 40

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/tutorials/parameter-sweeping.md` around lines 351 - 433, The pareto
documentation wasn't fully updated: add {"concurrency": 20} into the
"pareto_optimal" JSON array (refer to the pareto_optimal block), change the
ASCII chart marker for concurrency 20 from ○ to ● so it shows as Pareto optimal,
update the caption sentence that currently reads "Point 20 (○) is dominated..."
to reflect that 20 is Pareto optimal (not dominated), and update the Best
Practices example list that currently shows "Pareto optimal points: 10, 30, 40"
to include 20.

tests/unit/common/config/test_loadgen_config_validators.py (1)

427-464: Tests named _raises_error only assert defaults — misleading names persist.

test_parameter_sweep_mode_with_single_concurrency_raises_error, test_parameter_sweep_cooldown_with_single_concurrency_raises_error, and test_parameter_sweep_same_seed_with_single_concurrency_raises_error in TestSweepParamsValidation all simply construct LoadGeneratorConfig(concurrency=10) and assert the default value — none of them raise or verify any error. The _raises_error suffix directly contradicts what they do. As per coding guidelines, tests must be named test_<function>_<scenario>_<expected>.

🔧 Proposed fix — rename to reflect actual behavior

-    def test_parameter_sweep_mode_with_single_concurrency_raises_error(self):
-        """Test that parameter_sweep_mode defaults work with single concurrency.
-        ...
-        """
-        config = LoadGeneratorConfig(concurrency=10)
-        assert config.parameter_sweep_mode == "repeated"  # default value works
+    def test_parameter_sweep_mode_default_with_single_concurrency_succeeds(self):
+        """Test that default parameter_sweep_mode does not raise with single concurrency."""
+        config = LoadGeneratorConfig(concurrency=10)
+        assert config.parameter_sweep_mode == "repeated"

-    def test_parameter_sweep_cooldown_with_single_concurrency_raises_error(self):
+    def test_parameter_sweep_cooldown_default_with_single_concurrency_succeeds(self):
+        """Test that default parameter_sweep_cooldown_seconds does not raise with single concurrency."""
         config = LoadGeneratorConfig(concurrency=10)
-        assert config.parameter_sweep_cooldown_seconds == 0.0  # default value works
+        assert config.parameter_sweep_cooldown_seconds == 0.0

-    def test_parameter_sweep_same_seed_with_single_concurrency_raises_error(self):
+    def test_parameter_sweep_same_seed_default_with_single_concurrency_succeeds(self):
+        """Test that default parameter_sweep_same_seed does not raise with single concurrency."""
         config = LoadGeneratorConfig(concurrency=10)
-        assert config.parameter_sweep_same_seed is False  # default value works
+        assert config.parameter_sweep_same_seed is False

As per coding guidelines: "Name tests as test_<function>_<scenario>_<expected>".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/common/config/test_loadgen_config_validators.py` around lines 427
- 464, Rename the three misleading tests so their names reflect that they assert
default values rather than raising errors: change
test_parameter_sweep_mode_with_single_concurrency_raises_error to
test_parameter_sweep_mode_with_single_concurrency_defaults_to_repeated (or
similar), change
test_parameter_sweep_cooldown_with_single_concurrency_raises_error to
test_parameter_sweep_cooldown_with_single_concurrency_defaults_to_0 (or
similar), and change
test_parameter_sweep_same_seed_with_single_concurrency_raises_error to
test_parameter_sweep_same_seed_with_single_concurrency_defaults_to_false (or
similar); update any references/imports accordingly so TestSweepParamsValidation
still runs the renamed tests and ensure names follow the
test_<function>_<scenario>_<expected> guideline.

src/aiperf/orchestrator/orchestrator.py (1)

583-590: Metadata keyed as "concurrency" breaks non-concurrency sweeps.

Lines 587 and 712 set "concurrency": run_config.loadgen.concurrency unconditionally. Line 494 then gates the failed-sweep logging on "concurrency" in r.metadata. For a request_rate sweep (or any future parameter), these results would carry no "concurrency" key — the failed-value warning at Lines 494–503 would silently be dead code and downstream aggregation would receive wrong metadata.

Use sweep_strategy.parameter_name to populate and check the correct key. A complete fix was proposed in a prior review for _compose_trials_then_sweep, _compose_sweep_then_trials, _execute_strategy, _collect_failed_sweep_values, and all aggregation helpers.

Also applies to: 708-714, 494-503

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 583 - 590, The metadata
key is hardcoded as "concurrency" which breaks non-concurrency sweeps; change
all places that set or check result.metadata["concurrency"] (notably the
result.metadata.update block, the failed-sweep logging check in
_collect_failed_sweep_values, and similar logic in _compose_trials_then_sweep,
_compose_sweep_then_trials, _execute_strategy and aggregation helpers) to use
sweep_strategy.parameter_name as the metadata key and to populate value from the
corresponding run_config field (e.g., use the dynamic key
sweep_strategy.parameter_name: actual_value instead of "concurrency":
run_config.loadgen.concurrency) and update the existence checks to look for
sweep_strategy.parameter_name in r.metadata so failed-value warnings and
downstream aggregation work for request_rate and other sweep parameters.

🧹 Nitpick comments (3)

docs/tutorials/parameter-sweeping.md (1)

101-101: Add text language specifier to plain-text fenced blocks (MD040).

Five fenced code blocks (directory trees, execution-pattern diagrams, and the ASCII Pareto chart) are missing a language identifier, triggering MD040 warnings. Specifying a language improves content rendering by using the correct syntax highlighting for code. Use text for non-language preformatted content:

-```
+```text

Affected lines: 101, 146, 164, 188, 416.

Also applies to: 146-146, 164-164, 188-188, 416-416

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/tutorials/parameter-sweeping.md` at line 101, Several plain-text fenced
code blocks in the parameter-sweeping tutorial (the directory trees, the
execution-pattern diagrams, and the ASCII Pareto chart) are missing a language
specifier and triggering MD040; update each of those fenced blocks by changing
the opening fence from ``` to ```text so the blocks use the text language
identifier (e.g., the three directory tree blocks, the two
execution-pattern/diagram blocks, and the ASCII Pareto chart fenced blocks).

src/aiperf/orchestrator/orchestrator.py (1)

367-368: Redundant lazy imports — ParameterSweepStrategy and FixedTrialsStrategy are already at module level.

Lines 367 and 396 re-import symbols that are already unconditionally imported at Lines 16–20. Python caches these so it won't error, but the inline imports are dead weight.

♻️ Proposed fix

-    def _create_sweep_strategy(self, config: UserConfig) -> "ParameterSweepStrategy":
+    def _create_sweep_strategy(self, config: UserConfig) -> ParameterSweepStrategy:
         ...
-        from aiperf.orchestrator.strategies import ParameterSweepStrategy
-
         sweep_info = config.loadgen.get_sweep_parameter()

-    def _create_confidence_strategy(self, config: UserConfig) -> "FixedTrialsStrategy":
+    def _create_confidence_strategy(self, config: UserConfig) -> FixedTrialsStrategy:
         ...
-        from aiperf.orchestrator.strategies import FixedTrialsStrategy
-
         return FixedTrialsStrategy(

Also applies to: 396-397

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 367 - 368, The inline
imports of ParameterSweepStrategy and FixedTrialsStrategy in orchestrator.py are
redundant because those classes are already imported at module top-level; remove
the duplicate local import statements (the ones importing ParameterSweepStrategy
and FixedTrialsStrategy within functions/blocks) so the module-level imports are
used instead, leaving any other needed imports intact and running tests to
ensure no leftover references depend on the removed local import lines.

tests/unit/orchestrator/test_parameter_sweep_properties.py (1)

134-138: Unescaped regex metacharacters in match= — use raw strings.

Ruff RUF043 flags these patterns because (list|values) and similar use (, |, ) which are regex metacharacters. The patterns are intentionally regex, but should be written as raw strings to make this explicit and to satisfy the linter:

-        with pytest.raises(
-            ValueError,
-            match="Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",
-        ):
+        with pytest.raises(
+            ValueError,
+            match=r"Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",
+        ):

Apply the same fix to Line 146 (match="Invalid concurrency value|Concurrency must be at least 1") and Line 509.

Also applies to: 145-148, 506-511

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py` around lines 134
- 138, The failing assertions use regex strings in pytest.raises(match=...)
without raw-string notation; update the match arguments in the pytest.raises
calls (the one shown and the other occurrences around the same test) to raw
strings (prefix the string with r, e.g. r"Invalid concurrency
(list|values)|Parameter sweep requires at least 2 values") so regex
metacharacters are not interpreted as escape sequences; do the same for the
other two matches referenced (e.g. the one currently written as "Invalid
concurrency value|Concurrency must be at least 1" and the one near line 509) in
the test functions so all match patterns are raw strings.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.kiro/specs/code-review-fixes/COVERAGE_IMPROVEMENT.md:
- Line 9: The phrase "parameter sweep related files" should use a hyphenated
compound modifier; update the text to read "parameter-sweep related files" (or
"parameter‑sweep related files") so the heading becomes "**Goal**: Achieve 85%+
coverage for all parameter-sweep related files".

In `@docs/tutorials/parameter-sweeping.md`:
- Around line 680-693: Replace the incorrect JSON example: change top-level keys
to match the implementation by using "num_combinations" instead of "num_values",
"num_successful_runs" instead of "num_successful_values", and replace
"failed_values" with "failed_runs" where each failure entry contains "run_index"
and "error" (e.g., { "run_index": <int>, "error": "<message>" }); ensure the
metadata structure mirrors the example referenced earlier (line 247) so the
sweep aggregate uses num_combinations, num_successful_runs, and a failed_runs
array with run_index and error fields.

In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 387-403: The _create_confidence_strategy function currently
hardcodes auto_set_seed=True and disable_warmup_after_first=True, overriding
user settings; change it to pass the user's values from config (e.g., use
config.loadgen.set_consistent_seed for the auto_set_seed argument and
config.loadgen.profile_run_disable_warmup_after_first for the
disable_warmup_after_first argument) when constructing FixedTrialsStrategy so
the strategy honors user configuration instead of always forcing True.
- Around line 1037-1048: The return value from _compute_sweep_aggregates can be
None and that None is being put into the aggregate_results dict and passed into
_export_sweep_aggregates, causing AttributeError; fix by ensuring you only
include and pass a non-None sweep_aggregate: either have
_compute_sweep_aggregates always return a dict (never None) or, simpler, change
the code path in _aggregate_and_export to check for a truthy sweep_aggregate
before adding it to the returned dict and before calling
_export_sweep_aggregates (i.e., only set "sweep_aggregate" in aggregate_results
and call _export_sweep_aggregates when _compute_sweep_aggregates(...) is not
None). Ensure references to _compute_sweep_aggregates, _aggregate_and_export,
and _export_sweep_aggregates are used to locate and update the logic.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py`:
- Around line 480-511: The test_pbt_rejects_invalid_values uses random.randint
to insert invalid_value into valid_values which breaks Hypothesis
reproducibility; replace the use of random.randint by drawing the insertion
position (or the entire test_values) from a Hypothesis strategy using the draw
API (e.g., draw(st.integers(...)) or a composite strategy) inside
test_pbt_rejects_invalid_values so the position is part of the example space and
deterministic during shrinking — remove random.randint and build test_values
from drawn position/strategy instead.

---

Duplicate comments:
In `@docs/tutorials/parameter-sweeping.md`:
- Around line 351-433: The pareto documentation wasn't fully updated: add
{"concurrency": 20} into the "pareto_optimal" JSON array (refer to the
pareto_optimal block), change the ASCII chart marker for concurrency 20 from ○
to ● so it shows as Pareto optimal, update the caption sentence that currently
reads "Point 20 (○) is dominated..." to reflect that 20 is Pareto optimal (not
dominated), and update the Best Practices example list that currently shows
"Pareto optimal points: 10, 30, 40" to include 20.

In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 583-590: The metadata key is hardcoded as "concurrency" which
breaks non-concurrency sweeps; change all places that set or check
result.metadata["concurrency"] (notably the result.metadata.update block, the
failed-sweep logging check in _collect_failed_sweep_values, and similar logic in
_compose_trials_then_sweep, _compose_sweep_then_trials, _execute_strategy and
aggregation helpers) to use sweep_strategy.parameter_name as the metadata key
and to populate value from the corresponding run_config field (e.g., use the
dynamic key sweep_strategy.parameter_name: actual_value instead of
"concurrency": run_config.loadgen.concurrency) and update the existence checks
to look for sweep_strategy.parameter_name in r.metadata so failed-value warnings
and downstream aggregation work for request_rate and other sweep parameters.

In `@tests/unit/common/config/test_loadgen_config_validators.py`:
- Around line 427-464: Rename the three misleading tests so their names reflect
that they assert default values rather than raising errors: change
test_parameter_sweep_mode_with_single_concurrency_raises_error to
test_parameter_sweep_mode_with_single_concurrency_defaults_to_repeated (or
similar), change
test_parameter_sweep_cooldown_with_single_concurrency_raises_error to
test_parameter_sweep_cooldown_with_single_concurrency_defaults_to_0 (or
similar), and change
test_parameter_sweep_same_seed_with_single_concurrency_raises_error to
test_parameter_sweep_same_seed_with_single_concurrency_defaults_to_false (or
similar); update any references/imports accordingly so TestSweepParamsValidation
still runs the renamed tests and ensure names follow the
test_<function>_<scenario>_<expected> guideline.

---

Nitpick comments:
In `@docs/tutorials/parameter-sweeping.md`:
- Line 101: Several plain-text fenced code blocks in the parameter-sweeping
tutorial (the directory trees, the execution-pattern diagrams, and the ASCII
Pareto chart) are missing a language specifier and triggering MD040; update each
of those fenced blocks by changing the opening fence from ``` to ```text so the
blocks use the text language identifier (e.g., the three directory tree blocks,
the two execution-pattern/diagram blocks, and the ASCII Pareto chart fenced
blocks).

In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 367-368: The inline imports of ParameterSweepStrategy and
FixedTrialsStrategy in orchestrator.py are redundant because those classes are
already imported at module top-level; remove the duplicate local import
statements (the ones importing ParameterSweepStrategy and FixedTrialsStrategy
within functions/blocks) so the module-level imports are used instead, leaving
any other needed imports intact and running tests to ensure no leftover
references depend on the removed local import lines.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py`:
- Around line 134-138: The failing assertions use regex strings in
pytest.raises(match=...) without raw-string notation; update the match arguments
in the pytest.raises calls (the one shown and the other occurrences around the
same test) to raw strings (prefix the string with r, e.g. r"Invalid concurrency
(list|values)|Parameter sweep requires at least 2 values") so regex
metacharacters are not interpreted as escape sequences; do the same for the
other two matches referenced (e.g. the one currently written as "Invalid
concurrency value|Concurrency must be at least 1" and the one near line 509) in
the test functions so all match patterns are raw strings.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e008f3 and 7b4fa22cd29f8470d107630d65cc02b2ce577909.

📒 Files selected for processing (8)

• .kiro/specs/code-review-fixes/COVERAGE_IMPROVEMENT.md
• docs/api/sweep-aggregates.md
• docs/tutorials/parameter-sweeping.md
• src/aiperf/common/config/loadgen_config.py
• src/aiperf/orchestrator/orchestrator.py
• tests/unit/common/config/test_loadgen_config_validators.py
• tests/unit/orchestrator/test_orchestrator_sweep_aggregation.py
• tests/unit/orchestrator/test_parameter_sweep_properties.py

✅ Files skipped from review due to trivial changes (1)

• docs/api/sweep-aggregates.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Hyphenate compound modifier.

Use “parameter-sweep” (or “parameter‑sweep”) before “related files” for correct compound-modifier usage.

🧰 Tools

🪛 LanguageTool

[grammar] ~9-~9: Use a hyphen to join words.
Context: ...ve 85%+ coverage for all parameter sweep related files --- ## Coverage Summary ...

(QB_NEW_EN_HYPHEN)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.kiro/specs/code-review-fixes/COVERAGE_IMPROVEMENT.md at line 9, The phrase
"parameter sweep related files" should use a hyphenated compound modifier;
update the text to read "parameter-sweep related files" (or "parameter‑sweep
related files") so the heading becomes "**Goal**: Achieve 85%+ coverage for all
parameter-sweep related files".

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (3)

tests/unit/orchestrator/test_parameter_sweep_properties.py (1)

480-511: ⚠️ Potential issue | 🟡 Minor

random.randint inside @given still unresolved — breaks Hypothesis reproducibility.

random.randint at line 496 is not controlled by Hypothesis, so the insertion position differs between Hypothesis's initial run and its shrinking/replay passes, making a failing example impossible to reproduce deterministically.

🐛 Proposed fix (draw position from Hypothesis)

 `@given`(
     st.lists(st.integers(min_value=1, max_value=100), min_size=0, max_size=10),
     st.integers(max_value=0),
+    st.integers(min_value=0, max_value=10),
 )
 `@settings`(max_examples=100)
-def test_pbt_rejects_invalid_values(self, valid_values, invalid_value):
+def test_pbt_rejects_invalid_values(self, valid_values, invalid_value, position):
     # Insert invalid value at random position
     if valid_values:
-        import random
-        position = random.randint(0, len(valid_values))
-        test_values = (
-            valid_values[:position] + [invalid_value] + valid_values[position:]
-        )
+        position = position % (len(valid_values) + 1)
+        test_values = [*valid_values[:position], invalid_value, *valid_values[position:]]
     else:
         test_values = [invalid_value]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py` around lines 480
- 511, The test test_pbt_rejects_invalid_values uses random.randint to pick an
insertion position (random.randint) which breaks Hypothesis reproducibility;
replace the uncontrolled random call by drawing the position via Hypothesis
(either add a data=st.data() parameter and use data.draw(st.integers(...)) or
add an explicit st.integers(...) to the `@given` and accept it as a parameter) so
the insertion index is controlled by Hypothesis and shrinks/replays
deterministically; update the decorator and the code that builds test_values
(which uses valid_values and invalid_value) to use that drawn position instead
of random.randint.

src/aiperf/orchestrator/orchestrator.py (2)

92-105: ⚠️ Potential issue | 🟡 Minor

Trim or rephrase “what” comments.

These inline comments restate the next line(s); consider removing them or switching to rationale-only notes.

💡 Example cleanup

-        # Aggregate results
         aggregates = self.aggregate_results(results, config)
...
-        # Export aggregates
         if "aggregate" in aggregates:

As per coding guidelines, "Comments should explain 'why?' not 'what'".

Also applies to: 320-349

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 92 - 105, The comments
in orchestrator.py that restate code flow (e.g., the inline comments above the
aggregates handling and the comments "# No aggregation needed (e.g., sweep-only
mode)", "# Confidence-only mode", and "# Sweep + confidence mode") should be
removed or rewritten to explain why the branches exist rather than what the code
does; update the comments around the call to aggregate_results and the
conditional branches that call _export_confidence_aggregate and
_export_sweep_aggregates to either delete the redundant "what" comments or
replace them with brief rationale notes describing the intent/conditions for
each branch (e.g., why aggregation may be skipped, what higher-level workflow
yields a confidence-only aggregate), and apply the same cleanup to the similar
comment block in the section covering lines referenced around 320-349.

---

180-187: ⚠️ Potential issue | 🟠 Major

Persist the sweep parameter name/value instead of hardcoding concurrency.

Composed sweep runs currently write only concurrency into metadata, so non‑concurrency sweeps (e.g., request_rate) are misclassified and downstream aggregation/export/logging won’t find the correct key. Store parameter_name plus the parameter value, then key off that everywhere downstream (export, failed-value reporting, sweep detection, aggregation).

🐛 Suggested core fix (apply similarly in both composed modes)

-                result.metadata.update(
-                    {
-                        "trial_index": trial_index,
-                        "value_index": value_index,
-                        "concurrency": run_config.loadgen.concurrency,
-                        "sweep_mode": "repeated",
-                    }
-                )
+                param_name = sweep_strategy.parameter_name
+                param_value = getattr(run_config.loadgen, param_name)
+                result.metadata.update(
+                    {
+                        "trial_index": trial_index,
+                        "value_index": value_index,
+                        "parameter_name": param_name,
+                        param_name: param_value,
+                        "sweep_mode": "repeated",
+                    }
+                )

Also applies to: 493-503, 583-590, 708-714, 969-973, 1065-1069, 1090-1095, 1129-1134, 1203-1209

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 180 - 187, The
composed-sweep metadata currently hardcodes "concurrency" which mislabels
non-concurrency sweeps; update the code that detects and writes sweep metadata
(the param_name detection loop using per_value_aggregates and the code paths
that write metadata for composed runs) to persist both parameter_name (e.g.,
"concurrency" or "request_rate") and the corresponding parameter_value into the
run metadata instead of always writing "concurrency"; then update downstream
places that read the metadata to key off parameter_name (use the existing
param_name variable) when exporting, aggregating, reporting failed-values, and
detecting sweeps—apply the same change in the other composed-mode code paths
referenced (the other composed-run handlers mentioned in the comment).

🧹 Nitpick comments (3)

tests/unit/orchestrator/test_parameter_sweep_properties.py (2)

843-852: Remove the empty TestProperty14PBT class.

The class body contains only a comment and no test methods. pytest collects it silently, but it's dead code that adds noise. Either add the planned tests or delete the class skeleton.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py` around lines 843
- 852, Remove the dead test class TestProperty14PBT from the file: delete the
entire class definition (including its docstring and the NOTE about
analyze_trends()) since it contains no test methods and is unused; if you
intended to add tests instead, replace the class skeleton with actual pytest
test functions that validate trend detection logic (referencing the planned
analyze_trends behavior) otherwise simply remove the class to eliminate dead
code noise.

---

97-103: Several test names don't follow the test_<function>_<scenario>_<expected> convention.

Examples of violations (function under test is absent or the expected outcome is missing):

Current name	Suggested rename
test_single_value_remains_integer	test_loadgen_concurrency_single_int_assignment_preserves_int_type
test_both_cooldowns_independent	test_get_cooldown_seconds_independent_strategies_return_respective_values
test_zero_cooldown_default	test_get_cooldown_seconds_no_cooldown_specified_returns_zero

As per coding guidelines: "Name tests as test_<function>_<scenario>_<expected>".

Also applies to: 398-412, 414-421

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py` around lines 97 -
103, Rename tests to follow the test_<function>_<scenario>_<expected>
convention: change test_single_value_remains_integer to a name like
test_loadgen_concurrency_single_int_assignment_preserves_int_type (the symbol
under test is config.loadgen.concurrency), rename
test_both_cooldowns_independent to
test_get_cooldown_seconds_independent_strategies_return_respective_values and
test_zero_cooldown_default to
test_get_cooldown_seconds_no_cooldown_specified_returns_zero (the symbol under
test is get_cooldown_seconds); apply the same renaming pattern to the other
tests mentioned around the later blocks so each test name clearly states the
function, scenario and expected outcome.

tests/unit/common/config/test_loadgen_config_validators.py (1)

264-273: Add tests for explicitly passing default values with single concurrency to complete validation coverage.

Lines 264–273, 326–336, and 389–399 test explicit non-default values ("independent", 3.0, True) with single concurrency and correctly verify that validators reject them. However, in Pydantic v2, any field explicitly passed to the constructor is added to model_fields_set, including when the value equals the default. This means LoadGeneratorConfig(concurrency=10, parameter_sweep_mode="repeated"), …parameter_sweep_cooldown_seconds=0.0, and …parameter_sweep_same_seed=False also populate model_fields_set and trigger the same validator error, but these cases are not tested.

➕ Suggested additions (one per class)

+    def test_parameter_sweep_mode_explicit_default_with_single_concurrency_raises_error(self):
+        """Test that explicitly passing the default parameter_sweep_mode with single concurrency raises error."""
+        with pytest.raises(ValidationError) as exc_info:
+            LoadGeneratorConfig(concurrency=10, parameter_sweep_mode="repeated")
+        assert "--parameter-sweep-mode only applies when sweeping parameters" in str(exc_info.value)

+    def test_parameter_sweep_cooldown_explicit_zero_with_single_concurrency_raises_error(self):
+        """Test that explicitly passing 0.0 cooldown with single concurrency raises error."""
+        with pytest.raises(ValidationError) as exc_info:
+            LoadGeneratorConfig(concurrency=10, parameter_sweep_cooldown_seconds=0.0)
+        assert "--parameter-sweep-cooldown-seconds only applies when sweeping parameters" in str(exc_info.value)

+    def test_parameter_sweep_same_seed_explicit_false_with_single_concurrency_raises_error(self):
+        """Test that explicitly passing same_seed=False with single concurrency raises error."""
+        with pytest.raises(ValidationError) as exc_info:
+            LoadGeneratorConfig(concurrency=10, parameter_sweep_same_seed=False)
+        assert "--parameter-sweep-same-seed only applies when sweeping parameters" in str(exc_info.value)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/common/config/test_loadgen_config_validators.py` around lines 264
- 273, Add three tests to mirror the existing validators but pass the default
values explicitly to ensure Pydantic v2's model_fields_set triggers the same
validation: create tests similar to
test_parameter_sweep_mode_with_single_concurrency_raises_error that call
LoadGeneratorConfig(concurrency=10, parameter_sweep_mode="repeated"),
LoadGeneratorConfig(concurrency=10, parameter_sweep_cooldown_seconds=0.0), and
LoadGeneratorConfig(concurrency=10, parameter_sweep_same_seed=False), each
wrapped with pytest.raises(ValidationError) and asserting the same error message
text (e.g., "--parameter-sweep-mode only applies when sweeping parameters" or
the corresponding messages used by the validators) to confirm validators on
LoadGeneratorConfig reject explicitly provided default values.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/tutorials/parameter-sweeping.md`:
- Around line 101-119: The fenced code blocks in parameter-sweeping.md (the
directory tree block starting with "artifacts/" and the ASCII/trace blocks like
"Trial 1: [10 → 20 → 30]" and the throughput/latency chart) lack language
identifiers and trigger MD040; update each triple-backtick fence to use the text
language (```text) for those blocks so markdownlint accepts them—search for the
blocks containing "artifacts/", "Trial 1:", "Value 10:", and the throughput
ASCII chart and change their opening fences to ```text.

In `@tests/unit/common/config/test_loadgen_config_validators.py`:
- Around line 430-464: The three test docstrings in methods
test_parameter_sweep_mode_with_single_concurrency_defaults_to_repeated,
test_parameter_sweep_cooldown_with_single_concurrency_defaults_to_zero, and
test_parameter_sweep_same_seed_with_single_concurrency_defaults_to_false contain
a stale note about needing CLI/cyclopts to populate model_fields_set; update
each docstring to remove that incorrect statement and instead state briefly that
passing fields to the LoadGeneratorConfig constructor populates model_fields_set
(as shown in TestParameterSweepModeValidation,
TestParameterSweepCooldownValidation, TestParameterSweepSameSeedValidation), so
explicit-default tests can be added or adjusted accordingly.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py`:
- Around line 129-148: Update the pytest.raises calls in the tests to use
raw-string regex literals for the match= argument: in
test_rejects_values_less_than_one and test_rejects_zero_concurrency (and the
similar case around lines 506-511) change the match="..." patterns to raw
strings (e.g., match=r"...") so the parentheses, pipes and other regex
metacharacters are interpreted as regex rather than escape sequences; modify the
match arguments in the pytest.raises inside the
test_rejects_values_less_than_one and test_rejects_zero_concurrency functions
and the analogous test near line 509 accordingly.

---

Duplicate comments:
In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 92-105: The comments in orchestrator.py that restate code flow
(e.g., the inline comments above the aggregates handling and the comments "# No
aggregation needed (e.g., sweep-only mode)", "# Confidence-only mode", and "#
Sweep + confidence mode") should be removed or rewritten to explain why the
branches exist rather than what the code does; update the comments around the
call to aggregate_results and the conditional branches that call
_export_confidence_aggregate and _export_sweep_aggregates to either delete the
redundant "what" comments or replace them with brief rationale notes describing
the intent/conditions for each branch (e.g., why aggregation may be skipped,
what higher-level workflow yields a confidence-only aggregate), and apply the
same cleanup to the similar comment block in the section covering lines
referenced around 320-349.
- Around line 180-187: The composed-sweep metadata currently hardcodes
"concurrency" which mislabels non-concurrency sweeps; update the code that
detects and writes sweep metadata (the param_name detection loop using
per_value_aggregates and the code paths that write metadata for composed runs)
to persist both parameter_name (e.g., "concurrency" or "request_rate") and the
corresponding parameter_value into the run metadata instead of always writing
"concurrency"; then update downstream places that read the metadata to key off
parameter_name (use the existing param_name variable) when exporting,
aggregating, reporting failed-values, and detecting sweeps—apply the same change
in the other composed-mode code paths referenced (the other composed-run
handlers mentioned in the comment).

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py`:
- Around line 480-511: The test test_pbt_rejects_invalid_values uses
random.randint to pick an insertion position (random.randint) which breaks
Hypothesis reproducibility; replace the uncontrolled random call by drawing the
position via Hypothesis (either add a data=st.data() parameter and use
data.draw(st.integers(...)) or add an explicit st.integers(...) to the `@given`
and accept it as a parameter) so the insertion index is controlled by Hypothesis
and shrinks/replays deterministically; update the decorator and the code that
builds test_values (which uses valid_values and invalid_value) to use that drawn
position instead of random.randint.

---

Nitpick comments:
In `@tests/unit/common/config/test_loadgen_config_validators.py`:
- Around line 264-273: Add three tests to mirror the existing validators but
pass the default values explicitly to ensure Pydantic v2's model_fields_set
triggers the same validation: create tests similar to
test_parameter_sweep_mode_with_single_concurrency_raises_error that call
LoadGeneratorConfig(concurrency=10, parameter_sweep_mode="repeated"),
LoadGeneratorConfig(concurrency=10, parameter_sweep_cooldown_seconds=0.0), and
LoadGeneratorConfig(concurrency=10, parameter_sweep_same_seed=False), each
wrapped with pytest.raises(ValidationError) and asserting the same error message
text (e.g., "--parameter-sweep-mode only applies when sweeping parameters" or
the corresponding messages used by the validators) to confirm validators on
LoadGeneratorConfig reject explicitly provided default values.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py`:
- Around line 843-852: Remove the dead test class TestProperty14PBT from the
file: delete the entire class definition (including its docstring and the NOTE
about analyze_trends()) since it contains no test methods and is unused; if you
intended to add tests instead, replace the class skeleton with actual pytest
test functions that validate trend detection logic (referencing the planned
analyze_trends behavior) otherwise simply remove the class to eliminate dead
code noise.
- Around line 97-103: Rename tests to follow the
test_<function>_<scenario>_<expected> convention: change
test_single_value_remains_integer to a name like
test_loadgen_concurrency_single_int_assignment_preserves_int_type (the symbol
under test is config.loadgen.concurrency), rename
test_both_cooldowns_independent to
test_get_cooldown_seconds_independent_strategies_return_respective_values and
test_zero_cooldown_default to
test_get_cooldown_seconds_no_cooldown_specified_returns_zero (the symbol under
test is get_cooldown_seconds); apply the same renaming pattern to the other
tests mentioned around the later blocks so each test name clearly states the
function, scenario and expected outcome.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b4fa22cd29f8470d107630d65cc02b2ce577909 and 518e4a8.

📒 Files selected for processing (4)

• docs/tutorials/parameter-sweeping.md
• src/aiperf/orchestrator/orchestrator.py
• tests/unit/common/config/test_loadgen_config_validators.py
• tests/unit/orchestrator/test_parameter_sweep_properties.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add raw-string prefix to match= regex patterns (Ruff RUF043).

Lines 136, 146, and 509 pass strings containing (, ), and | to match= without a raw-string prefix, triggering Ruff RUF043. The patterns are valid regex (the alternation/grouping is intentional), but r"" makes the intent explicit.

🔧 Proposed fix

-            match="Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",
+            match=r"Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",

-            ValueError, match="Invalid concurrency value|Concurrency must be at least 1"
+            ValueError, match=r"Invalid concurrency value|Concurrency must be at least 1"

-            match="Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",
+            match=r"Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",

Also applies to: 506-511

🧰 Tools

🪛 Ruff (0.15.2)

[warning] 136-136: Pattern passed to match= contains metacharacters but is neither escaped nor raw

(RUF043)

---

[warning] 146-146: Pattern passed to match= contains metacharacters but is neither escaped nor raw

(RUF043)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py` around lines 129
- 148, Update the pytest.raises calls in the tests to use raw-string regex
literals for the match= argument: in test_rejects_values_less_than_one and
test_rejects_zero_concurrency (and the similar case around lines 506-511) change
the match="..." patterns to raw strings (e.g., match=r"...") so the parentheses,
pipes and other regex metacharacters are interpreted as regex rather than escape
sequences; modify the match arguments in the pytest.raises inside the
test_rejects_values_less_than_one and test_rejects_zero_concurrency functions
and the analogous test near line 509 accordingly.

[Lokiiiiii]

@ajcasagrande Could you help review this PR ?

I am still working on the unit tests and NIT comments but the core logic is finalized.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

tests/unit/orchestrator/test_parameter_sweep_properties.py (1)

134-148: ⚠️ Potential issue | 🟡 Minor

Use raw-string literals for match= regex patterns (if RUF043 is enforced).
These patterns include regex metacharacters; raw strings avoid escaping surprises and keep Ruff happy when RUF043 is enabled.

✅ Suggested fix

-            match="Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",
+            match=r"Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",

-            ValueError, match="Invalid concurrency value|Concurrency must be at least 1"
+            ValueError, match=r"Invalid concurrency value|Concurrency must be at least 1"

-            match="Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",
+            match=r"Invalid concurrency (list|values)|Parameter sweep requires at least 2 values",

Please confirm whether RUF043 is enabled in the repo’s Ruff config:

#!/bin/bash
# Check Ruff configuration for RUF043 enablement
fd -a pyproject.toml
rg -n "ruff|RUF043|select|ignore|extend-select|extend-ignore" pyproject.toml

Based on learnings: Always check the project's Ruff configuration in pyproject.toml before suggesting fixes for specific Ruff rules.

Also applies to: 506-510

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py` around lines 134
- 148, The regex patterns passed to pytest.raises in
test_rejects_zero_concurrency (and the similar assertion at lines 506-510)
should be raw-string literals to avoid accidental escapes and satisfy Ruff
RUF043; update the match= arguments so they use raw strings (e.g., r"Invalid
concurrency value|Concurrency must be at least 1") in the pytest.raises calls
inside test_rejects_zero_concurrency and the other failing test assertion.

src/aiperf/orchestrator/orchestrator.py (1)

488-499: ⚠️ Potential issue | 🟡 Minor

Failed sweep logging is hardcoded to concurrency.
For non-concurrency sweeps, failed values won’t be surfaced. Pass the sweep parameter name into _collect_failed_sweep_values and use it instead of hardcoding.

🐛 Suggested fix

-        if any("concurrency" in r.metadata for r in results):
-            failed_values = self._collect_failed_sweep_values(results)
+        sweep_param = getattr(strategy, "parameter_name", None)
+        if sweep_param and any(sweep_param in r.metadata for r in results):
+            failed_values = self._collect_failed_sweep_values(results, sweep_param)
@@
-        failed_values = self._collect_failed_sweep_values(all_results)
+        failed_values = self._collect_failed_sweep_values(
+            all_results, sweep_strategy.parameter_name
+        )
@@
-        failed_values = self._collect_failed_sweep_values(all_results)
+        failed_values = self._collect_failed_sweep_values(
+            all_results, sweep_strategy.parameter_name
+        )
@@
-    def _collect_failed_sweep_values(
-        self, results: list[RunResult]
-    ) -> list[dict[str, Any]]:
+    def _collect_failed_sweep_values(
+        self, results: list[RunResult], param_name: str | None = None
+    ) -> list[dict[str, Any]]:
@@
-        for result in results:
-            if not result.success and "concurrency" in result.metadata:
-                # Extract parameter info from metadata
-                param_value = result.metadata.get("concurrency")
-                param_name = "concurrency"  # Currently only concurrency is supported
+        for result in results:
+            name = param_name
+            if name is None:
+                for key in ["concurrency", "request_rate"]:
+                    if key in result.metadata:
+                        name = key
+                        break
+            if not result.success and name and name in result.metadata:
+                param_value = result.metadata.get(name)
 
                 # Create unique key to avoid duplicate entries
-                failure_key = (param_name, param_value)
+                failure_key = (name, param_value)
@@
-                            "parameter_name": param_name,
+                            "parameter_name": name,

Also applies to: 619-627, 746-754, 943-987

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 488 - 499, The logging
currently checks for "concurrency" in result.metadata and hardcodes that
parameter when collecting failed sweep values; change the callers around the
logger.warning blocks to pass the actual sweep parameter name (e.g., obtain the
sweep key from the run context or a variable used to start the sweep) into
_collect_failed_sweep_values instead of using "concurrency", and use that
returned parameter name (or the passed-in name) when formatting the warnings
(replace the hardcoded "concurrency" check and the metadata test with a dynamic
check using the passed parameter name). Update all occurrences that follow this
pattern (blocks around the calls to _collect_failed_sweep_values and the
subsequent logger.warning loops) so the function signature
_collect_failed_sweep_values(sweep_param, results) accepts the parameter name
and the warnings print fv['parameter_name'] or the passed sweep_param
consistently. Ensure callers at the four flagged locations pass the correct
sweep parameter name.

🧹 Nitpick comments (1)

src/aiperf/orchestrator/orchestrator.py (1)

453-461: Consider dropping “what” comments that restate the code.
These add noise without explaining rationale.

♻️ Suggested cleanup

-            # Strategy decides next config (including warmup handling)
             run_config = strategy.get_next_config(config, results)
 
-            # Strategy provides label
             label = strategy.get_run_label(run_index)
 
-            # Strategy determines artifact path
             artifact_path = strategy.get_run_path(self.base_dir, run_index)

As per coding guidelines: Comments should explain 'why?' not 'what'.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/aiperf/orchestrator/orchestrator.py` around lines 453 - 461, Remove the
redundant "what" comments that merely restate the code around
strategy.get_next_config, strategy.get_run_label, and strategy.get_run_path (the
comments above run_config, label, and artifact_path). Keep or replace comments
only if they add rationale or intent (the "why") — e.g., explain why a specific
strategy call is used, why warmup handling matters, or what invariants are
expected when passing self.base_dir and run_index. Update the file to delete the
three one-line comments or replace them with concise rationale comments
referencing the same symbols if additional context is necessary.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/tutorials/parameter-sweeping.md`:
- Around line 36-41: Remove the duplicated/nested ```bash fences and repeated
"aiperf profile" lines so each shell example starts with a single ```bash fence
followed by the comment and then the commands; specifically replace blocks that
currently have back-to-back fences and duplicated command lines with a single
opening ```bash, the comment "# These are equivalent - simple UI is
auto-selected" (where present), and one set of the "aiperf profile" commands
(including the backslash-continued forms) per fenced block; apply the same
cleanup pattern to the other affected blocks that show the same duplication (the
examples around the other occurrences of "aiperf profile" noted in the comment).

---

Duplicate comments:
In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 488-499: The logging currently checks for "concurrency" in
result.metadata and hardcodes that parameter when collecting failed sweep
values; change the callers around the logger.warning blocks to pass the actual
sweep parameter name (e.g., obtain the sweep key from the run context or a
variable used to start the sweep) into _collect_failed_sweep_values instead of
using "concurrency", and use that returned parameter name (or the passed-in
name) when formatting the warnings (replace the hardcoded "concurrency" check
and the metadata test with a dynamic check using the passed parameter name).
Update all occurrences that follow this pattern (blocks around the calls to
_collect_failed_sweep_values and the subsequent logger.warning loops) so the
function signature _collect_failed_sweep_values(sweep_param, results) accepts
the parameter name and the warnings print fv['parameter_name'] or the passed
sweep_param consistently. Ensure callers at the four flagged locations pass the
correct sweep parameter name.

In `@tests/unit/orchestrator/test_parameter_sweep_properties.py`:
- Around line 134-148: The regex patterns passed to pytest.raises in
test_rejects_zero_concurrency (and the similar assertion at lines 506-510)
should be raw-string literals to avoid accidental escapes and satisfy Ruff
RUF043; update the match= arguments so they use raw strings (e.g., r"Invalid
concurrency value|Concurrency must be at least 1") in the pytest.raises calls
inside test_rejects_zero_concurrency and the other failing test assertion.

---

Nitpick comments:
In `@src/aiperf/orchestrator/orchestrator.py`:
- Around line 453-461: Remove the redundant "what" comments that merely restate
the code around strategy.get_next_config, strategy.get_run_label, and
strategy.get_run_path (the comments above run_config, label, and artifact_path).
Keep or replace comments only if they add rationale or intent (the "why") —
e.g., explain why a specific strategy call is used, why warmup handling matters,
or what invariants are expected when passing self.base_dir and run_index. Update
the file to delete the three one-line comments or replace them with concise
rationale comments referencing the same symbols if additional context is
necessary.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 518e4a8 and d6d9606.

📒 Files selected for processing (5)

• docs/tutorials/parameter-sweeping.md
• src/aiperf/orchestrator/orchestrator.py
• tests/unit/common/config/test_loadgen_config_validators.py
• tests/unit/orchestrator/test_orchestrator_execution.py
• tests/unit/orchestrator/test_parameter_sweep_properties.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix duplicated code fences in bash examples.
There are nested/duplicated ```bash fences and repeated aiperf profile \ lines, which breaks rendering and CLI copy/paste.

💡 Suggested fix

-```bash
-# These are equivalent - simple UI is auto-selected
-```bash
+```bash
+# These are equivalent - simple UI is auto-selected
 aiperf profile --concurrency 10,20,30,40 ...
 aiperf profile --concurrency 10,20,30,40 --ui simple ...

@@
-bash -aiperf profile \ -bash
-aiperf profile
+```bash
+aiperf profile
--concurrency 10,20,30,40
--ui simple
...

@@
-```bash
-aiperf profile \
-  --concurrency 10,20,30,40 \
-```bash
-aiperf profile \
+```bash
+aiperf profile \
  --concurrency 10,20,30,40 \
  --ui none \
  ...

</details>



Also applies to: 51-58, 63-70

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/tutorials/parameter-sweeping.md around lines 36 - 41, Remove the
duplicated/nested bash fences and repeated "aiperf profile" lines so each shell example starts with a single bash fence followed by the comment and
then the commands; specifically replace blocks that currently have back-to-back
fences and duplicated command lines with a single opening ```bash, the comment
"# These are equivalent - simple UI is auto-selected" (where present), and one
set of the "aiperf profile" commands (including the backslash-continued forms)
per fenced block; apply the same cleanup pattern to the other affected blocks
that show the same duplication (the examples around the other occurrences of
"aiperf profile" noted in the comment).

</details>

<!-- fingerprinting:phantom:poseidon:eagle -->

<!-- This is an auto-generated comment by CodeRabbit -->

[ajcasagrande]

@Lokiiiiii thank you for this PR! It is quite large so it may take some time to get through. I probed it a bit with AI and overall it sounds well put together. As opposed to submitting all feedback after I get through all of it, I will submit them in chunks if that works for you, that way we can keep the momentum moving.

[Lokiiiiii]

@ajcasagrande Sounds good. We can do this piece-meal.

[Lokiiiiii]

@ajcasagrande Any updates ?

[ajcasagrande]

HI @Lokiiiiii , apologies on the delay. I started reviewing this last week, but got pulled off to support some high profile assignment, and have not been able to look at it much more.

My main concerns with the PR currently is the with the way some of the orchaestrator is setup. I was going back and forth with claude, seeing if there was a cleaner design. I did not have time to finish it, but here is the most recent version of the document:

Orchestrator Refactor: Strategy-Owned Polymorphism

Problem

MultiRunOrchestrator currently acts as both coordinator and dispatcher. It detects
execution mode from config, manually composes strategies, reverse-engineers mode from
result metadata, and hardcodes export path logic — all with dict[str, Any] return
types and string-key dispatch. This produces ~800 lines of conditional logic that
belongs in typed strategy classes.

Specific Issues

1. aggregate_results() returns dict[str, Any] with string keys ("aggregate",
   "per_value_aggregates", "sweep_aggregate") dispatched by if/elif in
   _aggregate_and_export(). No type safety, no polymorphism.

2. Mode detection from metadata introspection. _has_sweep_metadata() and
   _has_multiple_trials_per_value() scan result metadata for hardcoded keys
   ("concurrency", "request_rate") to figure out what mode was used — even though
   the mode was already known at execute() time.

3. Two near-identical composition methods. _compose_trials_then_sweep() and
   _compose_sweep_then_trials() share ~95% structure. The only difference is
   outer/inner loop order.

4. _collect_failed_sweep_values() hardcodes "concurrency" and won't work for
   request_rate sweeps despite the rest of the code supporting them.

5. Export methods reimplement path logic that strategies already own via
   get_run_path() and get_aggregate_path().

6. Sweep-only mode skips aggregation entirely (aggregate_results() returns {}),
   but sweep-only still needs Pareto optimal and best-config analysis.

7. Multiple asyncio.run() calls in export methods create/destroy event loops
   repeatedly instead of batching.

---

Single Run Is Not an Orchestrator Concern

Single-run benchmarks do not go through MultiRunOrchestrator. The entry point
in cli_runner.py branches at the top:

# cli_runner.py:run_system_controller()
if is_multi_run or is_sweep:
    _run_multi_benchmark(user_config, service_config)  # → orchestrator
else:
    _run_single_benchmark(user_config, service_config)  # → direct bootstrap

_run_single_benchmark calls bootstrap_and_run_service() in-process, which is
required for the Textual dashboard UI (log queues, macOS FD_CLOEXEC handling, mouse
event passthrough). The orchestrator runs each benchmark in a subprocess via
subprocess.run(), which cannot support the dashboard UI — this is why multi-run
mode forces UIType.SIMPLE and rejects UIType.DASHBOARD.

There should be no SingleRunStrategy. The orchestrator only handles the three
multi-run modes. The execute() method's current single-run fallback
(else: result = self._execute_single_run(...)) should be removed — if execute()
is called, there must be a sweep, confidence trials, or both.

---

Aggregation Matrix (Target Behavior)

The orchestrator handles three multi-run modes. Currently, sweep-only skips
aggregation entirely (aggregate_results() returns {}). This refactor adds
Pareto/best-config analysis for sweep-only mode.

Mode	Per-value aggregation	Cross-value aggregation
Confidence-only	Confidence stats (one "value")	None
Sweep-only	None	Pareto + best configs from raw
Sweep + Confidence	Confidence stats per value	Pareto + best configs from means

Sweep-only feeds single-run JsonMetricResult directly into SweepAggregation.
Sweep + Confidence feeds ConfidenceMetric.mean per value into SweepAggregation.
Different input shapes, same output type.

---

Design: Composition as a First-Class Strategy

The core insight: composition is a strategy, not something the orchestrator does
to two strategies. The three multi-run modes map to three strategy types, each owning
execution, aggregation, and export.

Strategy Hierarchy

ExecutionStrategy (ABC)
├── FixedTrialsStrategy        → execute: N identical runs, aggregate: confidence
├── ParameterSweepStrategy     → execute: 1 run per value, aggregate: sweep (Pareto)
└── SweepConfidenceStrategy    → execute: N runs per value, aggregate: both
        holds: ParameterSweepStrategy + FixedTrialsStrategy + SweepMode enum

SweepConfidenceStrategy is the composed case. It holds references to both inner
strategies and a SweepMode enum that controls loop nesting. The orchestrator never
sees composition logic.

New Abstract Methods on ExecutionStrategy

class ExecutionStrategy(ABC):
    # --- existing ---
    @abstractmethod
    def should_continue(self, results: list[RunResult]) -> bool: ...

    @abstractmethod
    def get_next_config(self, base_config: UserConfig, results: list[RunResult]) -> UserConfig: ...

    @abstractmethod
    def get_run_label(self, run_index: int) -> str: ...

    @abstractmethod
    def get_cooldown_seconds(self) -> float: ...

    @abstractmethod
    def get_run_path(self, base_dir: Path, run_index: int) -> Path: ...

    @abstractmethod
    def get_aggregate_path(self, base_dir: Path) -> Path: ...

    # --- new ---
    @abstractmethod
    def aggregate(self, results: list[RunResult], config: UserConfig) -> AggregateResult | None:
        """Compute aggregate statistics. Returns None if no aggregation needed."""

    @abstractmethod
    def export_aggregates(self, aggregate: AggregateResult, base_dir: Path) -> None:
        """Export aggregate artifacts to disk."""

SweepMode Enum

Replace Literal["repeated", "independent"] with a proper enum:

class SweepMode(CaseInsensitiveStrEnum):
    REPEATED = "repeated"      # for trial in trials: for value in values
    INDEPENDENT = "independent"  # for value in values: for trial in trials

This is used internally by SweepConfidenceStrategy to decide loop order.
loadgen_config.parameter_sweep_mode can migrate to this enum or remain a
Literal with conversion at strategy creation time.

---

Strategy Implementations

FixedTrialsStrategy Changes

Add aggregate() and export_aggregates(). Move confidence export logic from
orchestrator's _export_confidence_aggregate():

def aggregate(self, results: list[RunResult], config: UserConfig) -> AggregateResult | None:
    aggregation = ConfidenceAggregation(confidence_level=config.loadgen.confidence_level)
    result = aggregation.aggregate(results)
    result.metadata["cooldown_seconds"] = config.loadgen.profile_run_cooldown_seconds
    return result

def export_aggregates(self, aggregate: AggregateResult, base_dir: Path) -> None:
    aggregate_dir = self.get_aggregate_path(base_dir)
    _export_confidence(aggregate, aggregate_dir)

ParameterSweepStrategy Changes

Add sweep-level aggregation (Pareto + best configs) using single-run metrics.
Move _compute_sweep_aggregates logic from orchestrator, adapted for single-run
input (no confidence wrapping):

def aggregate(self, results: list[RunResult], config: UserConfig) -> AggregateResult | None:
    per_combination_stats = self._build_per_combination_stats(results)
    sweep_parameters = [{"name": self.parameter_name, "values": sorted(self.parameter_values)}]
    sweep_dict = SweepAggregation.compute(per_combination_stats, sweep_parameters)
    return _build_sweep_aggregate_result(results, sweep_dict)

def export_aggregates(self, aggregate: AggregateResult, base_dir: Path) -> None:
    sweep_dir = self.get_aggregate_path(base_dir)
    _export_sweep(aggregate, sweep_dir)

def _build_per_combination_stats(
    self, results: list[RunResult]
) -> dict[ParameterCombination, dict]:
    """Convert single-run JsonMetricResult into SweepAggregation input format.

    For sweep-only, each value has exactly one run. Extract metrics directly
    from summary_metrics without confidence wrapping.
    """
    stats = {}
    for i, result in enumerate(results):
        if not result.success:
            continue
        value = self.parameter_values[i]
        coord = ParameterCombination({self.parameter_name: value})
        metrics_dict = {}
        for metric_name, metric_result in result.summary_metrics.items():
            metrics_dict[metric_name] = {
                "mean": metric_result.avg,
                "unit": metric_result.unit,
            }
        stats[coord] = metrics_dict
    return stats

SweepConfidenceStrategy (New)

The composed case as a first-class strategy:

class SweepConfidenceStrategy(ExecutionStrategy):
    """Strategy for parameter sweep with confidence trials at each value.

    Composes ParameterSweepStrategy and FixedTrialsStrategy. Owns the
    nested execution loop, per-value confidence aggregation, and
    cross-value sweep aggregation.
    """

    def __init__(
        self,
        sweep: ParameterSweepStrategy,
        confidence: FixedTrialsStrategy,
        mode: SweepMode,
    ) -> None:
        self.sweep = sweep
        self.confidence = confidence
        self.mode = mode

Execution: should_continue / get_next_config track a flattened index that
maps to (value_index, trial_index) or (trial_index, value_index) depending on
self.mode. Alternatively, since the orchestrator's _execute_strategy loop is
already generic, SweepConfidenceStrategy can override the full iteration by
yielding (config, label, path) tuples from an iterator — but the simpler approach
is to keep the existing loop and let the strategy manage its internal counters.

Aggregation:

def aggregate(self, results: list[RunResult], config: UserConfig) -> AggregateResult | None:
    confidence_level = config.loadgen.confidence_level

    # Step 1: Group results by parameter value
    results_by_value = self._group_by_value(results)

    # Step 2: Per-value confidence aggregation
    aggregation = ConfidenceAggregation(confidence_level=confidence_level)
    per_value_aggregates = {}
    for value, value_results in sorted(results_by_value.items()):
        successful = [r for r in value_results if r.success]
        if len(successful) < 2:
            continue
        per_value_aggregates[value] = aggregation.aggregate(value_results)

    # Step 3: Sweep aggregation over confidence means
    per_combination_stats = self._confidence_to_sweep_stats(per_value_aggregates)
    sweep_parameters = [{"name": self.sweep.parameter_name, "values": sorted(per_value_aggregates.keys())}]
    sweep_dict = SweepAggregation.compute(per_combination_stats, sweep_parameters)

    return _build_sweep_aggregate_result(results, sweep_dict, per_value_aggregates)

Export:

def export_aggregates(self, aggregate: AggregateResult, base_dir: Path) -> None:
    # Export per-value confidence aggregates
    for value, conf_aggregate in aggregate.metadata["per_value_aggregates"].items():
        if self.mode == SweepMode.REPEATED:
            agg_dir = base_dir / "aggregate" / f"{self.sweep.parameter_name}_{value}"
        else:
            agg_dir = base_dir / f"{self.sweep.parameter_name}_{value}" / "aggregate"
        _export_confidence(conf_aggregate, agg_dir)

    # Export sweep-level aggregate
    if self.mode == SweepMode.REPEATED:
        sweep_dir = base_dir / "aggregate" / "sweep_aggregate"
    else:
        sweep_dir = base_dir / "sweep_aggregate"
    _export_sweep(aggregate, sweep_dir)

Note: The path logic here still needs to know about mode because the directory
structure differs between repeated and independent. This is inherent to the
feature, not a code smell — the strategy owns this decision.

---

Shared Export Helpers

Extract the asyncio.run() + exporter instantiation into module-level helpers
that any strategy can call. Single asyncio.run() per export call:

def _export_confidence(aggregate: AggregateResult, output_dir: Path) -> None:
    """Export confidence aggregate to JSON and CSV."""
    import asyncio
    from aiperf.exporters.aggregate import (
        AggregateConfidenceCsvExporter,
        AggregateConfidenceJsonExporter,
        AggregateExporterConfig,
    )

    config = AggregateExporterConfig(result=aggregate, output_dir=output_dir)

    async def _export() -> None:
        await asyncio.to_thread(output_dir.mkdir, parents=True, exist_ok=True)
        await asyncio.gather(
            AggregateConfidenceJsonExporter(config).export(),
            AggregateConfidenceCsvExporter(config).export(),
        )

    asyncio.run(_export())


def _export_sweep(aggregate: AggregateResult, output_dir: Path) -> None:
    """Export sweep aggregate to JSON and CSV."""
    # Same pattern with AggregateSweepJsonExporter / AggregateSweepCsvExporter

---

Simplified Orchestrator

After the refactor, MultiRunOrchestrator becomes a thin coordinator:

class MultiRunOrchestrator:
    def __init__(self, base_dir: Path, service_config: ServiceConfig) -> None:
        self.base_dir = Path(base_dir)
        self.service_config = service_config

    def execute_and_export(self, base_config: UserConfig) -> list[RunResult]:
        strategy = self._resolve_strategy(base_config)
        results = self._execute_loop(base_config, strategy)
        aggregate = strategy.aggregate(results, base_config)
        if aggregate is not None:
            strategy.export_aggregates(aggregate, self.base_dir)
        return results

    def _resolve_strategy(self, config: UserConfig) -> ExecutionStrategy:
        """Detect mode from config and return the appropriate strategy.

        Only called from multi-run paths (sweep, confidence, or both).
        Single-run benchmarks go through _run_single_benchmark() directly
        and never reach the orchestrator.
        """
        has_sweep = config.loadgen.get_sweep_parameter() is not None
        has_confidence = config.loadgen.num_profile_runs > 1

        if has_sweep and has_confidence:
            return SweepConfidenceStrategy(
                sweep=self._create_sweep_strategy(config),
                confidence=self._create_confidence_strategy(config),
                mode=SweepMode(config.loadgen.parameter_sweep_mode),
            )
        if has_sweep:
            return self._create_sweep_strategy(config)
        if has_confidence:
            return self._create_confidence_strategy(config)
        raise ValueError(
            "MultiRunOrchestrator requires sweep or confidence mode. "
            "Single-run benchmarks should use _run_single_benchmark() directly."
        )

    def _execute_loop(
        self, config: UserConfig, strategy: ExecutionStrategy
    ) -> list[RunResult]:
        """Generic execution loop driven entirely by strategy."""
        results: list[RunResult] = []
        run_index = 0
        strategy.validate_config(config)

        while strategy.should_continue(results):
            run_config = strategy.get_next_config(config, results)
            label = strategy.get_run_label(run_index)
            artifact_path = strategy.get_run_path(self.base_dir, run_index)

            result = self._execute_single_run(run_config, label, artifact_path)
            results.append(result)
            run_index += 1

            if strategy.should_continue(results):
                cooldown = strategy.get_cooldown_seconds()
                if cooldown > 0:
                    time.sleep(cooldown)

        return results

    # _execute_single_run() and _extract_summary_metrics() remain unchanged

What's eliminated:

• _compose_trials_then_sweep() (~128 lines)
• _compose_sweep_then_trials() (~127 lines)
• aggregate_results() with string-key dispatch (~66 lines)
• _aggregate_and_export() with if/elif dispatch (~23 lines)
• _export_confidence_aggregate() (~44 lines)
• _export_sweep_aggregates() (~146 lines)
• _has_sweep_metadata() (~17 lines)
• _has_multiple_trials_per_value() (~33 lines)
• _aggregate_per_sweep_value() (~73 lines)
• _compute_sweep_aggregates() (~118 lines)
• _collect_failed_sweep_values() (~46 lines)

Total: ~820 lines removed from orchestrator, replaced by typed methods on
strategies that are individually testable.

---

SweepConfidenceStrategy Execution Detail

The main design question: how does SweepConfidenceStrategy fit into the generic
_execute_loop?

Option A: Flattened Index Mapping

The strategy pre-computes a flat schedule of (value_index, trial_index) pairs
based on SweepMode, then maps run_index into that schedule:

class SweepConfidenceStrategy(ExecutionStrategy):
    def __init__(self, sweep, confidence, mode):
        self.sweep = sweep
        self.confidence = confidence
        self.mode = mode
        self._schedule = self._build_schedule()

    def _build_schedule(self) -> list[tuple[int, int]]:
        """Pre-compute (value_index, trial_index) execution order."""
        n_values = len(self.sweep.parameter_values)
        n_trials = self.confidence.num_trials
        if self.mode == SweepMode.REPEATED:
            # for trial in trials: for value in values
            return [(vi, ti) for ti in range(n_trials) for vi in range(n_values)]
        else:
            # for value in values: for trial in trials
            return [(vi, ti) for vi in range(n_values) for ti in range(n_trials)]

    def should_continue(self, results: list[RunResult]) -> bool:
        return len(results) < len(self._schedule)

    def get_next_config(self, base_config: UserConfig, results: list[RunResult]) -> UserConfig:
        value_index, trial_index = self._schedule[len(results)]
        # Apply sweep config (sets parameter value, derives seed)
        sweep_results_stub = [None] * value_index  # len == value_index
        config = self.sweep.get_next_config(base_config, sweep_results_stub)
        # Apply trial config (handles warmup, seed adjustments)
        trial_results_stub = [None] * trial_index
        config = self.confidence.get_next_config(config, trial_results_stub)
        return config

    def get_run_label(self, run_index: int) -> str:
        value_index, trial_index = self._schedule[run_index]
        sweep_label = self.sweep.get_run_label(value_index)
        trial_label = self.confidence.get_run_label(trial_index)
        return f"{sweep_label}_{trial_label}"

    def get_run_path(self, base_dir: Path, run_index: int) -> Path:
        value_index, trial_index = self._schedule[run_index]
        if self.mode == SweepMode.REPEATED:
            trial_dir = self.confidence.get_run_path(base_dir, trial_index)
            return self.sweep.get_run_path(trial_dir, value_index)
        else:
            sweep_dir = self.sweep.get_run_path(base_dir, value_index)
            return self.confidence.get_run_path(sweep_dir, trial_index)

    def get_cooldown_seconds(self) -> float:
        # Use sweep cooldown between values, trial cooldown between trials
        # The generic loop calls this between every run, so we need context
        # See "Cooldown" section below
        ...

Option B: Strategy Owns Its Loop

Add an optional execute() method that strategies can override to run their own
loop. The orchestrator checks for it:

class ExecutionStrategy(ABC):
    def execute(
        self, config: UserConfig, run_fn: Callable[[UserConfig, str, Path], RunResult], base_dir: Path
    ) -> list[RunResult] | None:
        """Override to run a custom execution loop. Return None to use default loop."""
        return None

SweepConfidenceStrategy overrides this to run nested loops with proper cooldown
logic (sweep cooldown between values, trial cooldown between trials within a value).

Recommendation: Option A (flattened schedule) keeps the orchestrator's single
loop, which is simpler to test and reason about. The cooldown issue is solvable (see
below).

Cooldown in Flattened Mode

The generic loop calls get_cooldown_seconds() between every pair of runs. But
the composed case needs different cooldowns at different boundaries:

• Between trials of the same value → trial cooldown
• Between the last trial of one value and the first trial of the next → sweep cooldown

Solution: get_cooldown_seconds() takes the current run index so the strategy can
check whether the next run crosses a value boundary:

def get_cooldown_seconds(self, run_index: int | None = None) -> float:
    if run_index is None:
        return 0.0
    current = self._schedule[run_index]
    next_idx = run_index + 1
    if next_idx >= len(self._schedule):
        return 0.0
    next_entry = self._schedule[next_idx]
    # Crossing a value boundary?
    if current[0] != next_entry[0]:
        return self.sweep.get_cooldown_seconds()
    return self.confidence.get_cooldown_seconds()

The orchestrator passes run_index to get_cooldown_seconds(). Existing strategies
ignore the parameter (backward compatible with a default of None).

---

Metadata Tagging

Results are tagged with execution context at creation time in get_next_config() or
via a new tag_result() method, instead of being reverse-engineered from metadata
in the aggregation step:

class SweepConfidenceStrategy(ExecutionStrategy):
    def tag_result(self, result: RunResult, run_index: int) -> RunResult:
        value_index, trial_index = self._schedule[run_index]
        result.metadata.update({
            "trial_index": trial_index,
            "value_index": value_index,
            self.sweep.parameter_name: self.sweep.parameter_values[value_index],
            "sweep_mode": self.mode.value,
        })
        return result

The orchestrator calls strategy.tag_result(result, run_index) after each run.
Default implementation is a no-op. This eliminates _has_sweep_metadata() and
_has_multiple_trials_per_value() entirely.

---

Failed Value Collection

Replace the hardcoded "concurrency" check with strategy-aware failure tracking.
Move _collect_failed_sweep_values into strategies that have sweep parameters:

class ParameterSweepStrategy(ExecutionStrategy):
    def collect_failed_values(self, results: list[RunResult]) -> list[dict[str, Any]]:
        failed = []
        seen = set()
        for result in results:
            if not result.success and self.parameter_name in result.metadata:
                value = result.metadata[self.parameter_name]
                if value not in seen:
                    seen.add(value)
                    failed.append({
                        "value": value,
                        "parameter_name": self.parameter_name,
                        "error": result.error or "Unknown error",
                    })
        return failed

---

Migration Path

Phase 1: Add new methods to ExecutionStrategy

• Add aggregate(), export_aggregates(), tag_result() as abstract methods
• Implement on FixedTrialsStrategy and ParameterSweepStrategy
• Add SweepMode enum

Phase 2: Create SweepConfidenceStrategy

• Implement with flattened schedule
• Add aggregate() with per-value confidence + sweep aggregation
• Add export_aggregates() with mode-aware path logic

Phase 3: Simplify orchestrator

• Replace execute() branching with _resolve_strategy()
• Replace _aggregate_and_export() with strategy.aggregate() + strategy.export_aggregates()
• Remove single-run fallback from execute() (single runs never reach orchestrator)
• Delete composition methods, metadata introspection, export dispatch

Phase 4: Extract shared export helpers

• _export_confidence() and _export_sweep() as module-level functions
• Single asyncio.run() per export call

Phase 5: Update tests

• Test each strategy's aggregate() and export_aggregates() independently
• Test SweepConfidenceStrategy schedule generation for both modes
• Simplify orchestrator tests to verify delegation, not implementation

---

Files Changed

File	Change
orchestrator/strategies.py	Add SweepMode enum, SweepConfidenceStrategy. Add aggregate(), export_aggregates(), tag_result() to base and all strategies.
orchestrator/orchestrator.py	Remove ~730 lines of dispatch/composition/export. Replace with _resolve_strategy() + generic loop + delegation.
orchestrator/export_helpers.py	New file: _export_confidence(), _export_sweep() shared helpers.
common/config/loadgen_config.py	Optional: migrate parameter_sweep_mode to SweepMode enum.
tests/unit/orchestrator/	Update strategy tests, add SweepConfidenceStrategy tests, simplify orchestrator tests.

[ajcasagrande]

this is something I had been looking at adding, I'm glad to see it come in, and hope the project as a whole can start using more of it. Thanks!

[ajcasagrande]

this needs the check for is_muilti_run as well, right? recommend possibly moving the check out of here, look at my other comment.

[ajcasagrande]

I reccomend maybe puting the ui validation call direcly above _run_multi_benchmark, then it doesnt need to do the checks. alternatively, if you prefer it seperate for testing, pass the is_sweep and is_multi_run directly to the validator

[ajcasagrande]

hmm, i see this is somewhat duplicate now with the one above. should find a single place for it i think.

[ajcasagrande]

it it worthwhile to have an else block raise an exception? since I assume _run_multi should not be called if both are false.

[ajcasagrande]

Like I said, that document was not complete, but I posted it so you could get an idea of where I was thinking with it.