This repository was archived by the owner on Apr 20, 2026. It is now read-only.
feat: add job name override#181
Open
cquil11 wants to merge 1 commit into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR consolidates warmup behavior from a separate invocation into the main benchmark call, adds a job-name CLI override parameter that propagates through the submission pipeline, removes legacy dataset sampling helpers from the benchmark script, and extends runtime reporting with detailed statistics. Changes
Sequence DiagramsequenceDiagram
participant Bench as bench.sh
participant BenchServe as benchmark_serving.py
participant Metrics as Metrics Collector
Bench->>BenchServe: Invoke with --num-warmups N
BenchServe->>BenchServe: Execute warmup phase (N preliminary requests)
BenchServe->>Metrics: Skip warmup results from final metrics
BenchServe->>BenchServe: Execute main benchmark phase (with profiler)
BenchServe->>Metrics: Collect & aggregate main benchmark results
Metrics-->>BenchServe: Return statistics (latency, tokens, throughput)
BenchServe-->>Bench: Output results (with optional per-request details)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
cquil11
force-pushed
the
feat/add-job-name-override
branch
from
February 19, 2026 17:37
83c812d to
51bb94b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/srtctl/cli/submit.py (1)
363-424:⚠️ Potential issue | 🟠 Major
--job-nameCLI override is silently discarded in sweep mode —job_nameparameter is shadowed by loop variables.The parameter
job_name(holding the CLI override) is overwritten by local loop assignments at lines 364, 388, and 406 before the value is ever used. By line 423,job_namealways holds the last-iterated config's name fromconfig_dict, not the user-supplied override.🐛 Proposed fix — rename loop-local variable
for i, (config_dict, params) in enumerate(configs, 1): - job_name = config_dict.get("name", f"job_{i}") + sweep_job_name = config_dict.get("name", f"job_{i}") params_str = ", ".join(f"{k}={v}" for k, v in params.items()) - table.add_row(str(i), job_name, params_str) + table.add_row(str(i), sweep_job_name, params_str) ... if dry_run: ... for i, (config_dict, _params) in enumerate(configs, 1): - job_name = config_dict.get("name", f"job_{i}") - job_dir = sweep_dir / f"job_{i:03d}_{job_name}" + sweep_job_name = config_dict.get("name", f"job_{i}") + job_dir = sweep_dir / f"job_{i:03d}_{sweep_job_name}" ... ... for i, (config_dict, _params) in enumerate(configs, 1): - job_name = config_dict.get("name", f"job_{i}") - progress.update(task, description=f"[{i}/{len(configs)}] {job_name}") + sweep_job_name = config_dict.get("name", f"job_{i}") + progress.update(task, description=f"[{i}/{len(configs)}] {sweep_job_name}") ... submit_single( ... job_name=job_name, # now correctly carries the CLI override )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/cli/submit.py` around lines 363 - 424, The CLI-provided job_name is being overwritten by loop-local assignments (the for loops over enumerate(configs) assign job_name from config_dict), so rename the loop-local variable (e.g., cfg_job_name or iter_job_name) and resolve a single final job_name per-iteration that prefers the CLI override when present (job_name = cli_job_name or config_dict.get("name", ...)). Also move the temp-config creation, load_config, and submit_single(...) into the per-job loop (inside the Progress loop) so each config is written and submitted individually using the resolved job_name; update references to submit_single, load_config, and temp_config_path accordingly.
🧹 Nitpick comments (6)
tests/test_configs.py (2)
585-617: Fragile hardcodedcall_args_list[1]index in both override tests.The test assumes the Panel call is always the second
console.print()invocation. Any reordering or addition insubmit_with_orchestrator's dry-run branch will silently break the assertion or check the wrong object.♻️ More robust alternative — search for the Panel by type
from rich.panel import Panel # Locate the first Panel print call regardless of position panel_calls = [ call[0][0] for call in mock_console.print.call_args_list if call[0] and isinstance(call[0][0], Panel) and call[0][0].title is not None ] assert panel_calls, "No titled Panel was printed" assert panel_calls[0].title == "overridden-name"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_configs.py` around lines 585 - 617, The tests use a fragile hardcoded index into mock_console.print.call_args_list to find the Panel—change both tests (the override and non-override cases around submit_with_orchestrator) to locate the printed Panel by inspecting mock_console.print.call_args_list for entries where the first positional arg is a rich.panel.Panel with a non-None title, then assert on that Panel.title; update references to submit_with_orchestrator, mock_console.print.call_args_list and Panel in the test so it finds the first titled Panel regardless of call order and fails clearly if none are found.
505-618: Missing test coverage forsubmit_sweepwithjob_nameoverride.The new tests only exercise
submit_with_orchestratorin dry-run mode. There is no test that verifies--job-namepropagates correctly throughsubmit_sweep, which is precisely where the shadowing bug lives. A test targetingsubmit_sweepwould catch the regression described above.Would you like me to draft a
test_submit_sweep_applies_job_name_overridetest? As per coding guidelines, "when making a new significant feature change, always add a new test."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_configs.py` around lines 505 - 618, Add a new unit test named test_submit_sweep_applies_job_name_override that exercises submit_sweep with a job_name override: construct an SrtConfig (use ModelConfig and ResourceConfig as in existing tests), patch srtctl.cli.submit.console (via unittest.mock.patch) and call submit_sweep(config_path=Path("/tmp/test.yaml"), config=config, dry_run=True, job_name="overridden-name"); then inspect mock_console.print.call_args_list to find the rendered Panel object and assert its title == "overridden-name" (and that the original config.name is not present). This will verify submit_sweep correctly propagates the job_name override instead of being shadowed.src/srtctl/cli/submit.py (1)
149-149: Movefrom dataclasses import replaceto top-level imports.
dataclassesis a stdlib module with no circular-import risk here. Burying this inside the function body is non-idiomatic.♻️ Proposed refactor
import argparse import contextlib +import dataclasses import json ...Then at the call site:
- from dataclasses import replace - if job_name: - config = replace(config, name=job_name) + config = dataclasses.replace(config, name=job_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/cli/submit.py` at line 149, Move the local "from dataclasses import replace" import to the module top-level imports and remove the buried import inside the function; update any call sites that use replace (e.g., where replace(...) is invoked in this module) to rely on the module-level import so the symbol "replace" is available throughout the file and follows normal import conventions.src/srtctl/benchmarks/scripts/sa-bench/benchmark_serving.py (3)
1052-1052:--num-warmupsis missing ahelp=stringEvery other argument in the parser includes a
help=string; this one is inconsistent.✏️ Proposed fix
- parser.add_argument("--num-warmups", type=int, default=0) + parser.add_argument( + "--num-warmups", + type=int, + default=0, + help="Number of warmup requests to send before the main benchmark. " + "Warmup results are discarded and do not affect reported metrics.", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/benchmarks/scripts/sa-bench/benchmark_serving.py` at line 1052, The CLI argument definition parser.add_argument("--num-warmups", type=int, default=0) is missing a help string; update that call to include a concise help= description (e.g., "number of warmup iterations before measurement" or similar) so it matches the other parser.add_argument entries and displays proper usage information.
778-787: Dead CLI argument groups and misleading--dataset-pathhelp textNow that
--dataset-nameonly accepts"random"and the non-random branches were removed frommain(), thesonnet_group(lines 951–968),sharegpt_group(lines 970–976), andhf_group(lines 1012–1020) arguments are never consumed. They pollute--helpoutput and may confuse users. Additionally,--dataset-path's help text still refers to ShareGPT/Sonnet/HF datasets (line 786).Consider removing these argument groups (or at minimum updating
--dataset-path's help text) to keep the CLI surface consistent with the supported code paths.Also applies to: 950-1020
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/benchmarks/scripts/sa-bench/benchmark_serving.py` around lines 778 - 787, Remove the dead CLI argument groups (sonnet_group, sharegpt_group, hf_group) since --dataset-name only accepts "random" and those branches were removed from main(), and update the --dataset-path parser.add_argument help text to no longer reference ShareGPT/Sonnet/HF datasets (make it generic, e.g., "optional path for dataset inputs" or describe usage for random dataset if applicable); locate the argument definitions by their group names (sonnet_group, sharegpt_group, hf_group) and the --dataset-path parser.add_argument call in benchmark_serving.py and either delete the unused groups or prune their flags, then edit the --dataset-path help string to reflect the current supported CLI behavior.
880-887:default=Falseis redundant; help text should document--save-resultdependency
action="store_true"already defaults toFalse. The explicitdefault=Falseis harmless but noisy. Additionally,--save-detailedsilently does nothing when--save-resultis not also set — worth documenting.✏️ Proposed fix
parser.add_argument( "--save-detailed", action="store_true", - default=False, - help="When saving results, include detailed per-request data " - "(input_lens, output_lens, ttfts, itls, generated_texts, errors). " - "By default, only aggregated metrics are saved to reduce file size.", + help="When saving results (requires --save-result), include detailed per-request data " + "(input_lens, output_lens, ttfts, itls, generated_texts, errors). " + "By default, only aggregated metrics are saved to reduce file size.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/benchmarks/scripts/sa-bench/benchmark_serving.py` around lines 880 - 887, Remove the redundant default=False from the parser.add_argument call for "--save-detailed" (since action="store_true" implies False by default) and update its help text in the parser.add_argument for "--save-detailed" to explicitly mention that it only has effect when "--save-result" is also set; optionally add a runtime check after parsing (e.g., in the main or args handling code that processes save_result/save_detailed) to warn or raise if args.save_detailed is True while args.save_result is False to make the dependency explicit at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/srtctl/benchmarks/scripts/sa-bench/benchmark_serving.py`:
- Around line 119-123: sample_uniform can crash when range_ratio > 1.0 because
lower = int(seq_len * range_ratio) may exceed upper (seq_len); update
sample_uniform to clamp lower to at most upper (e.g., set lower =
min(int(seq_len * range_ratio), seq_len) and ensure lower >= 1) before calling
np.random.randint, and/or add validation for the CLI arg --random-range-ratio in
main() so range_ratio is constrained to a sensible [0,1] range; refer to
sample_uniform, range_ratio, seq_len, and num_prompts when making the change.
- Around line 736-737: The deletion of keys from benchmark_result after merging
it into result_json is dead code; remove the loop/lines that call del
benchmark_result[field] so benchmark_result is left untouched (since result_json
= {**result_json, **benchmark_result} already copies values and subsequent
json.dump and save_to_pytorch_benchmark_format use result_json). Update around
the code that handles result_json and benchmark_result (referencing the
variables benchmark_result, result_json and the functions json.dump and
save_to_pytorch_benchmark_format) to simply merge and persist result_json
without mutating benchmark_result.
---
Outside diff comments:
In `@src/srtctl/cli/submit.py`:
- Around line 363-424: The CLI-provided job_name is being overwritten by
loop-local assignments (the for loops over enumerate(configs) assign job_name
from config_dict), so rename the loop-local variable (e.g., cfg_job_name or
iter_job_name) and resolve a single final job_name per-iteration that prefers
the CLI override when present (job_name = cli_job_name or
config_dict.get("name", ...)). Also move the temp-config creation, load_config,
and submit_single(...) into the per-job loop (inside the Progress loop) so each
config is written and submitted individually using the resolved job_name; update
references to submit_single, load_config, and temp_config_path accordingly.
---
Nitpick comments:
In `@src/srtctl/benchmarks/scripts/sa-bench/benchmark_serving.py`:
- Line 1052: The CLI argument definition parser.add_argument("--num-warmups",
type=int, default=0) is missing a help string; update that call to include a
concise help= description (e.g., "number of warmup iterations before
measurement" or similar) so it matches the other parser.add_argument entries and
displays proper usage information.
- Around line 778-787: Remove the dead CLI argument groups (sonnet_group,
sharegpt_group, hf_group) since --dataset-name only accepts "random" and those
branches were removed from main(), and update the --dataset-path
parser.add_argument help text to no longer reference ShareGPT/Sonnet/HF datasets
(make it generic, e.g., "optional path for dataset inputs" or describe usage for
random dataset if applicable); locate the argument definitions by their group
names (sonnet_group, sharegpt_group, hf_group) and the --dataset-path
parser.add_argument call in benchmark_serving.py and either delete the unused
groups or prune their flags, then edit the --dataset-path help string to reflect
the current supported CLI behavior.
- Around line 880-887: Remove the redundant default=False from the
parser.add_argument call for "--save-detailed" (since action="store_true"
implies False by default) and update its help text in the parser.add_argument
for "--save-detailed" to explicitly mention that it only has effect when
"--save-result" is also set; optionally add a runtime check after parsing (e.g.,
in the main or args handling code that processes save_result/save_detailed) to
warn or raise if args.save_detailed is True while args.save_result is False to
make the dependency explicit at runtime.
In `@src/srtctl/cli/submit.py`:
- Line 149: Move the local "from dataclasses import replace" import to the
module top-level imports and remove the buried import inside the function;
update any call sites that use replace (e.g., where replace(...) is invoked in
this module) to rely on the module-level import so the symbol "replace" is
available throughout the file and follows normal import conventions.
In `@tests/test_configs.py`:
- Around line 585-617: The tests use a fragile hardcoded index into
mock_console.print.call_args_list to find the Panel—change both tests (the
override and non-override cases around submit_with_orchestrator) to locate the
printed Panel by inspecting mock_console.print.call_args_list for entries where
the first positional arg is a rich.panel.Panel with a non-None title, then
assert on that Panel.title; update references to submit_with_orchestrator,
mock_console.print.call_args_list and Panel in the test so it finds the first
titled Panel regardless of call order and fails clearly if none are found.
- Around line 505-618: Add a new unit test named
test_submit_sweep_applies_job_name_override that exercises submit_sweep with a
job_name override: construct an SrtConfig (use ModelConfig and ResourceConfig as
in existing tests), patch srtctl.cli.submit.console (via unittest.mock.patch)
and call submit_sweep(config_path=Path("/tmp/test.yaml"), config=config,
dry_run=True, job_name="overridden-name"); then inspect
mock_console.print.call_args_list to find the rendered Panel object and assert
its title == "overridden-name" (and that the original config.name is not
present). This will verify submit_sweep correctly propagates the job_name
override instead of being shadowed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.