feat: add sizing recommender mode for procurement GPU planning#1369
feat: add sizing recommender mode for procurement GPU planning#1369natoscott wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
WalkthroughAdds a ChangesRecommendation execution
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiconfigurator/cli/report_and_save.py (1)
146-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoad-match/recommend rows still display the search-ceiling GPU count, not the actual
total_gpus_needed.
display_total_gpus(Lines 183 and 277) only special-casespreserve_ranking, falling back to the function'stotal_gpusparameter for everything else — including recommend/load-match rows. Forcli_recommend,preserve_ranking=objective_awareis alwaysFalse(recommend never setsobjective_target), sototal_gpushere is the escalated per-worker search-ceiling budget from_build_recommend_tasks(e.g. 8/16/32/64), not the actual GPU count needed. The printed row's leading number is therefore the arbitrary sweep ceiling while the real answer is buried in the parentheticaltotal_gpus_used. This is the exact fix already correctly applied for the "Overall Best Configuration" section at Lines 526-530 (if load_match and "replicas_needed" in best_conf_details.index: display_total_gpus = int(best_conf_details["total_gpus_needed"])) — it just wasn't threaded through here.Separately,
ranking_label(Line 146) always resolves to"tokens/s/gpu"for load-match rows even though the table is now actually sorted bytotal_gpus_neededascending (Lines 124-127), so the printed header text ("Ranked by tokens/s/gpu") no longer matches the real sort order.For a procurement-sizing tool, showing the wrong headline GPU count is a meaningful correctness/UX problem.
🔧 Proposed fix
- ranking_label = objective_target if preserve_ranking and objective_target else "tokens/s/gpu" + if preserve_ranking and objective_target: + ranking_label = objective_target + elif "replicas_needed" in top_configs.columns: + ranking_label = "total_gpus_needed" + else: + ranking_label = "tokens/s/gpu"Apply the same change at both row-rendering sites:
- display_total_gpus = row["total_gpus_used"] if preserve_ranking else total_gpus + display_total_gpus = row["total_gpus_used"] if preserve_ranking or "replicas_needed" in row else total_gpus(once in the
is_disaggblock near Line 183, once in theaggblock near Line 277)Also applies to: 182-184, 276-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiconfigurator/cli/report_and_save.py` around lines 146 - 147, Update both row-rendering sites in the is_disagg and agg display blocks so load-match rows use total_gpus_needed for display_total_gpus when available, rather than the search-ceiling total_gpus parameter; preserve existing behavior for other rows. Also update ranking_label to report total_gpus_needed for load-match sorting, while retaining the current objective-based or tokens/s/gpu labels for other modes.
🧹 Nitpick comments (1)
src/aiconfigurator/cli/main.py (1)
388-548: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRecommend-mode arguments largely duplicate
_add_default_mode_arguments.
isl,osl,ttft,tpot, image dims,prefix,nextn/nextn-accept-rates,enable-chunked-prefill,free-gpu-memory-fraction,max-seq-len,enable-wideep, andmoe-backendare re-declared here with (presumably) identical defaults/help text to_add_default_mode_arguments. Over time these two argument sets can silently drift (e.g. a default changes in one function but not the other), producing inconsistent CLI behavior betweendefaultandrecommendmodes for the same flags.Consider extracting a shared helper (e.g.
_add_shared_workload_arguments(parser)) that both_add_default_mode_argumentsand_add_recommend_mode_argumentscall, keeping only mode-specific flags (the target group,--total-gpus, etc.) local to each.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiconfigurator/cli/main.py` around lines 388 - 548, The workload-related arguments in _add_recommend_mode_arguments duplicate those in _add_default_mode_arguments and can drift. Extract the shared flags—sequence lengths, image settings, SLA values, prefix/MTP options, chunked prefill, GPU memory, max sequence length, WideEP, and MoE backend—into a helper such as _add_shared_workload_arguments, then call it from both mode-specific functions while retaining only each mode’s unique options locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 112-119: Update the README mode list to include the CLI’s estimate
mode, so it lists all six supported modes: default, estimate, recommend, exp,
generate, and support. Keep the surrounding mode descriptions unchanged.
In `@src/aiconfigurator/cli/api.py`:
- Around line 468-501: Validate gpus_per_node against _MAX_GPUS_PER_WORKER
before constructing budgets or entering the escalation loop, and raise the
existing descriptive ValueError when it exceeds the limit. Remove the
unreachable result-is-None check after the loop while preserving normal
escalation and execution behavior for valid GPU counts.
In `@src/aiconfigurator/webapp/components/recommender_tab.py`:
- Around line 37-47: Update the target_request_rate and target_concurrency
defaults in the recommender UI so neither option is prefilled, requiring the
user to provide exactly one. Preserve the existing labels and guidance, and
ensure the values passed to cli_recommend satisfy its exactly-one validation
when only one field is selected.
In `@src/aiconfigurator/webapp/events/event_fn.py`:
- Around line 1404-1461: Update the recommender UI and parameter flow around
run_recommender so the unused quant-mode controls (gemm_quant_mode,
kvcache_quant_mode, fmha_quant_mode, moe_quant_mode, comm_quant_mode) and
enable_eplb are hidden or disabled rather than presented as configurable inputs.
Preserve the existing supported options forwarded to cli_recommend, including
enable_wideep, nextn, and nextn_accept_rates.
In `@tests/integration/test_picking.py`:
- Around line 224-231: Update the local min_gpus helper to compute the minimum
total_gpus_needed across every eligible dataframe in cli_result.best_configs,
rather than returning from the first matching mode. Ignore missing, empty, or
column-less dataframes, and preserve the existing 0 result when no eligible
values exist.
---
Outside diff comments:
In `@src/aiconfigurator/cli/report_and_save.py`:
- Around line 146-147: Update both row-rendering sites in the is_disagg and agg
display blocks so load-match rows use total_gpus_needed for display_total_gpus
when available, rather than the search-ceiling total_gpus parameter; preserve
existing behavior for other rows. Also update ranking_label to report
total_gpus_needed for load-match sorting, while retaining the current
objective-based or tokens/s/gpu labels for other modes.
---
Nitpick comments:
In `@src/aiconfigurator/cli/main.py`:
- Around line 388-548: The workload-related arguments in
_add_recommend_mode_arguments duplicate those in _add_default_mode_arguments and
can drift. Extract the shared flags—sequence lengths, image settings, SLA
values, prefix/MTP options, chunked prefill, GPU memory, max sequence length,
WideEP, and MoE backend—into a helper such as _add_shared_workload_arguments,
then call it from both mode-specific functions while retaining only each mode’s
unique options locally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fb18b258-fe0a-4c70-a8e1-580efcf60715
📒 Files selected for processing (14)
README.mddocs/cli_user_guide.mdsrc/aiconfigurator/cli/__init__.pysrc/aiconfigurator/cli/api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/cli/report_and_save.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/events/event_fn.pysrc/aiconfigurator/webapp/events/event_handler.pysrc/aiconfigurator/webapp/main.pytests/integration/test_picking.pytests/unit/cli/test_argument_parsing.pytests/unit/cli/test_cli_api.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Build and Test (unit)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Cargo Deny
- GitHub Check: Build and Test (e2e)
⚠️ CI failures not shown inline (2)
GitHub Actions: Lint and Format / 1_Lint and Format (Ruff).txt: feat: add sizing recommender mode for procurement GPU planning
Conclusion: failure
##[group]Run ruff format --check .
�[36;1mruff format --check .�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
Would reformat: tests/integration/test_picking.py
Would reformat: tests/unit/cli/test_argument_parsing.py
2 files would be reformatted, 510 files already formatted
##[error]Process completed with exit code 1.
GitHub Actions: Lint and Format / Lint and Format (Ruff): feat: add sizing recommender mode for procurement GPU planning
Conclusion: failure
##[group]Run ruff format --check .
�[36;1mruff format --check .�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
Would reformat: tests/integration/test_picking.py
Would reformat: tests/unit/cli/test_argument_parsing.py
2 files would be reformatted, 510 files already formatted
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
**/*: Treat this file as the only always-injected rule file; do not add new always-on rules here without human approval. New rule files must includepaths:frontmatter.
When reviewing changes in a governed area, read that area's rule files first, even if path-based auto-loading does not occur during a read-only review. Rule violations are review findings even when the code works.
A task must remain within its module: collector tasks may change onlycollector/and its tests, generator tasks onlysrc/aiconfigurator/generator/and its tests, and SDK tasks must not modify either. Cross-module contract changes require explicit human approval.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/cli/__init__.pysrc/aiconfigurator/webapp/components/readme_tab.pytests/unit/cli/test_argument_parsing.pydocs/cli_user_guide.mdtests/integration/test_picking.pysrc/aiconfigurator/webapp/events/event_fn.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/webapp/events/event_handler.pyREADME.mdsrc/aiconfigurator/cli/api.pytests/unit/cli/test_cli_api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/cli/report_and_save.py
⚙️ CodeRabbit configuration file
**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.
- Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
- If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/cli/__init__.pysrc/aiconfigurator/webapp/components/readme_tab.pytests/unit/cli/test_argument_parsing.pydocs/cli_user_guide.mdtests/integration/test_picking.pysrc/aiconfigurator/webapp/events/event_fn.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/webapp/events/event_handler.pyREADME.mdsrc/aiconfigurator/cli/api.pytests/unit/cli/test_cli_api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/cli/report_and_save.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Run
ruff check .andruff format --check .to validate Python linting and formatting.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/cli/__init__.pysrc/aiconfigurator/webapp/components/readme_tab.pytests/unit/cli/test_argument_parsing.pytests/integration/test_picking.pysrc/aiconfigurator/webapp/events/event_fn.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/webapp/events/event_handler.pysrc/aiconfigurator/cli/api.pytests/unit/cli/test_cli_api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/cli/report_and_save.py
src/aiconfigurator/webapp/**
⚙️ CodeRabbit configuration file
src/aiconfigurator/webapp/**: - Verify UI changes preserve support-matrix navigation, filtering, and rendering behavior across local static previews and the webapp.
- Check that webapp flows handle missing Git LFS performance data gracefully when the change touches default-mode or support data loading.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/events/event_fn.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/webapp/events/event_handler.py
src/aiconfigurator/cli/**
⚙️ CodeRabbit configuration file
src/aiconfigurator/cli/**: - Check that CLI argument changes preserve backward compatibility, validation behavior, defaults, and plain-output expectations.
- For new or changed user-facing options, verify docs updates and generator/SDK wiring.
- Watch for non-TTY assumptions in tests or output formatting.
Files:
src/aiconfigurator/cli/__init__.pysrc/aiconfigurator/cli/api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/cli/report_and_save.py
tests/**/*.{py,yaml,txt,sh}
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Use integration tests for the full input-to-artifacts pipeline, comparing output against golden snapshots without external dependencies.
Files:
tests/unit/cli/test_argument_parsing.pytests/integration/test_picking.pytests/unit/cli/test_cli_api.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run unit tests with
pytest -m unit; usepytest -m "unit or build"for the build-test subset when required data is available.
Files:
tests/unit/cli/test_argument_parsing.pytests/integration/test_picking.pytests/unit/cli/test_cli_api.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: - Check that tests cover the changed behavior rather than only the happy path.
- Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.
Files:
tests/unit/cli/test_argument_parsing.pytests/integration/test_picking.pytests/unit/cli/test_cli_api.py
docs/cli_user_guide.md
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
Document newly added generator parameters in the CLI user guide.
Files:
docs/cli_user_guide.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: - Check that docs match changed CLI, SDK, generator, backend, and support-matrix behavior.
- Flag docs that describe unsupported runtimes, stale command names, or behavior not covered by tests or support-matrix evidence.
Files:
docs/cli_user_guide.md
src/aiconfigurator/cli/main.py
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
When adding a generator parameter, expose it through the CLI, using
--generator-setwhen applicable.
Files:
src/aiconfigurator/cli/main.py
🪛 GitHub Actions: Lint and Format / 1_Lint and Format (Ruff).txt
tests/unit/cli/test_argument_parsing.py
[error] 1-1: ruff format --check failed: file would be reformatted by ruff format.
tests/integration/test_picking.py
[error] 1-1: ruff format --check failed: file would be reformatted by ruff format.
🪛 GitHub Actions: Lint and Format / Lint and Format (Ruff)
tests/unit/cli/test_argument_parsing.py
[error] 1-1: ruff format --check failed: file would be reformatted by Ruff.
tests/integration/test_picking.py
[error] 1-1: ruff format --check failed: file would be reformatted by Ruff.
🔇 Additional comments (20)
src/aiconfigurator/webapp/components/recommender_tab.py (1)
1-35: LGTM!Also applies to: 51-74
src/aiconfigurator/webapp/events/event_fn.py (2)
1374-1384: LGTM!
1463-1485: LGTM!src/aiconfigurator/webapp/events/event_handler.py (1)
508-564: LGTM!src/aiconfigurator/webapp/main.py (1)
77-77: LGTM!Also applies to: 86-86, 127-130, 169-170, 198-199
src/aiconfigurator/cli/main.py (2)
1175-1187: LGTM!Also applies to: 2522-2568
1775-1775: 🎯 Functional CorrectnessThis hint does not apply to recommend mode. Recommend mode calls
cli_recommenddirectly;_execute_tasksis only used by the default/exp flows, so the--total-gpussuggestion won’t appear there.> Likely an incorrect or invalid review comment.tests/unit/cli/test_argument_parsing.py (1)
51-51: LGTM!Also applies to: 427-485
README.md (1)
159-159: LGTM!Also applies to: 179-188
docs/cli_user_guide.md (1)
3-3: LGTM!Also applies to: 347-396
src/aiconfigurator/cli/__init__.py (1)
10-10: LGTM!Also applies to: 19-27, 54-54, 63-63
src/aiconfigurator/webapp/components/readme_tab.py (1)
55-60: LGTM!src/aiconfigurator/cli/api.py (4)
111-127: LGTM!
2079-2079: LGTM!
519-552: 🎯 Functional CorrectnessNo change needed here.
save_results()only callsload_generator_overrides_from_args(), and that helper usesgetattr(...)for the optional fields it reads, so the missing recommend-mode attributes won’t raise or affect this path.total_gpusis not read bysave_results()either.> Likely an incorrect or invalid review comment.
313-334: 🎯 Functional CorrectnessNo action needed
cli_recommendonly calls this helper withgpus_per_nodeand repeated doublings, so it never feeds a non-power-of-two budget here today.> Likely an incorrect or invalid review comment.tests/unit/cli/test_cli_api.py (1)
343-582: LGTM!tests/integration/test_picking.py (1)
182-207: LGTM!src/aiconfigurator/cli/report_and_save.py (2)
92-144: LGTM!
412-413: LGTM!Also applies to: 526-540
| # Build escalation sequence: gpus_per_node, *2, *4, *8, capped at _MAX_GPUS_PER_WORKER. | ||
| budgets = [gpus_per_node] | ||
| budget = gpus_per_node * 2 | ||
| while budget <= _MAX_GPUS_PER_WORKER: | ||
| budgets.append(budget) | ||
| budget *= 2 | ||
|
|
||
| result = None | ||
| for attempt, gpu_budget in enumerate(budgets): | ||
| tasks = _build_recommend_tasks(base_tasks, gpu_budget) | ||
| try: | ||
| result = _execute_and_wrap_result( | ||
| tasks, | ||
| mode="default", | ||
| top_n=top_n, | ||
| strict_sla=strict_sla, | ||
| target_request_rate=target_request_rate, | ||
| target_concurrency=target_concurrency, | ||
| ) | ||
| break | ||
| except SystemExit as exc: | ||
| if exc.code != 1 or attempt == len(budgets) - 1: | ||
| raise | ||
| logger.info( | ||
| "Recommend: no valid config at %d GPUs/worker; escalating to %d.", | ||
| gpu_budget, | ||
| budgets[attempt + 1], | ||
| ) | ||
|
|
||
| if result is None: | ||
| raise ValueError( | ||
| f"System {system} has {gpus_per_node} GPUs/node which exceeds " | ||
| f"the maximum per-worker search limit ({_MAX_GPUS_PER_WORKER})." | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The "exceeds max per-worker search limit" ValueError (Line 498) is unreachable.
budgets always contains at least [gpus_per_node]. On every loop iteration, either _execute_and_wrap_result succeeds and breaks, or it raises SystemExit, and attempt == len(budgets) - 1 is always true on the final iteration — forcing an unconditional raise there. So the for loop can never fall through to if result is None without either breaking or already re-raising. Concretely: if gpus_per_node > _MAX_GPUS_PER_WORKER (the exact scenario this message describes), budgets == [gpus_per_node] has one element, that attempt is simultaneously the first and last, and a failure re-raises the raw SystemExit(1) instead of ever reaching this friendlier ValueError.
🔧 Proposed fix — check the ceiling before searching, drop the dead branch
spec = load_system_spec(system)
gpus_per_node = spec["node"]["num_gpus_per_node"]
+
+ if gpus_per_node > _MAX_GPUS_PER_WORKER:
+ raise ValueError(
+ f"System {system} has {gpus_per_node} GPUs/node which exceeds "
+ f"the maximum per-worker search limit ({_MAX_GPUS_PER_WORKER})."
+ )- if result is None:
- raise ValueError(
- f"System {system} has {gpus_per_node} GPUs/node which exceeds "
- f"the maximum per-worker search limit ({_MAX_GPUS_PER_WORKER})."
- )
-
# Re-select chosen_exp by fewest total GPUs (not highest throughput).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build escalation sequence: gpus_per_node, *2, *4, *8, capped at _MAX_GPUS_PER_WORKER. | |
| budgets = [gpus_per_node] | |
| budget = gpus_per_node * 2 | |
| while budget <= _MAX_GPUS_PER_WORKER: | |
| budgets.append(budget) | |
| budget *= 2 | |
| result = None | |
| for attempt, gpu_budget in enumerate(budgets): | |
| tasks = _build_recommend_tasks(base_tasks, gpu_budget) | |
| try: | |
| result = _execute_and_wrap_result( | |
| tasks, | |
| mode="default", | |
| top_n=top_n, | |
| strict_sla=strict_sla, | |
| target_request_rate=target_request_rate, | |
| target_concurrency=target_concurrency, | |
| ) | |
| break | |
| except SystemExit as exc: | |
| if exc.code != 1 or attempt == len(budgets) - 1: | |
| raise | |
| logger.info( | |
| "Recommend: no valid config at %d GPUs/worker; escalating to %d.", | |
| gpu_budget, | |
| budgets[attempt + 1], | |
| ) | |
| if result is None: | |
| raise ValueError( | |
| f"System {system} has {gpus_per_node} GPUs/node which exceeds " | |
| f"the maximum per-worker search limit ({_MAX_GPUS_PER_WORKER})." | |
| ) | |
| spec = load_system_spec(system) | |
| gpus_per_node = spec["node"]["num_gpus_per_node"] | |
| if gpus_per_node > _MAX_GPUS_PER_WORKER: | |
| raise ValueError( | |
| f"System {system} has {gpus_per_node} GPUs/node which exceeds " | |
| f"the maximum per-worker search limit ({_MAX_GPUS_PER_WORKER})." | |
| ) | |
| # Build escalation sequence: gpus_per_node, *2, *4, *8, capped at _MAX_GPUS_PER_WORKER. | |
| budgets = [gpus_per_node] | |
| budget = gpus_per_node * 2 | |
| while budget <= _MAX_GPUS_PER_WORKER: | |
| budgets.append(budget) | |
| budget *= 2 | |
| result = None | |
| for attempt, gpu_budget in enumerate(budgets): | |
| tasks = _build_recommend_tasks(base_tasks, gpu_budget) | |
| try: | |
| result = _execute_and_wrap_result( | |
| tasks, | |
| mode="default", | |
| top_n=top_n, | |
| strict_sla=strict_sla, | |
| target_request_rate=target_request_rate, | |
| target_concurrency=target_concurrency, | |
| ) | |
| break | |
| except SystemExit as exc: | |
| if exc.code != 1 or attempt == len(budgets) - 1: | |
| raise | |
| logger.info( | |
| "Recommend: no valid config at %d GPUs/worker; escalating to %d.", | |
| gpu_budget, | |
| budgets[attempt + 1], | |
| ) | |
| # Re-select chosen_exp by fewest total GPUs (not highest throughput). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/cli/api.py` around lines 468 - 501, Validate gpus_per_node
against _MAX_GPUS_PER_WORKER before constructing budgets or entering the
escalation loop, and raise the existing descriptive ValueError when it exceeds
the limit. Remove the unreachable result-is-None check after the loop while
preserving normal escalation and execution behavior for valid GPU counts.
| with gr.Row(): | ||
| target_request_rate = gr.Number( | ||
| value=10, | ||
| label="Target request rate (req/s)", | ||
| info="Target system throughput. Fill this OR target concurrency, not both.", | ||
| ) | ||
| target_concurrency = gr.Number( | ||
| value=None, | ||
| label="Target concurrency (concurrent users)", | ||
| info="Target concurrent requests. Fill this OR target request rate, not both.", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Default values violate the "exactly one" contract for rate vs. concurrency.
target_request_rate defaults to 10 while target_concurrency defaults to None. If a user only fills in concurrency (forgetting to clear the pre-filled rate), cli_recommend will always raise ValueError("Exactly one of target_request_rate or target_concurrency must be provided."), and EventFn.run_recommender (event_fn.py) surfaces this only as a raw traceback in the debug box — not a helpful message pointing at the actual mistake.
🛠️ Proposed fix
target_request_rate = gr.Number(
- value=10,
+ value=None,
label="Target request rate (req/s)",
info="Target system throughput. Fill this OR target concurrency, not both.",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with gr.Row(): | |
| target_request_rate = gr.Number( | |
| value=10, | |
| label="Target request rate (req/s)", | |
| info="Target system throughput. Fill this OR target concurrency, not both.", | |
| ) | |
| target_concurrency = gr.Number( | |
| value=None, | |
| label="Target concurrency (concurrent users)", | |
| info="Target concurrent requests. Fill this OR target request rate, not both.", | |
| ) | |
| with gr.Row(): | |
| target_request_rate = gr.Number( | |
| value=None, | |
| label="Target request rate (req/s)", | |
| info="Target system throughput. Fill this OR target concurrency, not both.", | |
| ) | |
| target_concurrency = gr.Number( | |
| value=None, | |
| label="Target concurrency (concurrent users)", | |
| info="Target concurrent requests. Fill this OR target request rate, not both.", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/webapp/components/recommender_tab.py` around lines 37 -
47, Update the target_request_rate and target_concurrency defaults in the
recommender UI so neither option is prefilled, requiring the user to provide
exactly one. Preserve the existing labels and guidance, and ensure the values
passed to cli_recommend satisfy its exactly-one validation when only one field
is selected.
| def min_gpus(cli_result): | ||
| for df in cli_result.best_configs.values(): | ||
| if df is not None and not df.empty and "total_gpus_needed" in df.columns: | ||
| return int(df["total_gpus_needed"].min()) | ||
| return 0 | ||
|
|
||
| low_gpus = min_gpus(result_low) | ||
| high_gpus = min_gpus(result_high) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
min_gpus returns the first non-empty mode's minimum, not the true minimum across all modes.
The loop returns as soon as it hits the first dataframe with total_gpus_needed, so if agg is non-empty and iterated first, disagg's (possibly smaller) minimum is never considered. The monotonicity check then may not actually reflect the overall recommended GPU count.
🔧 Proposed fix
def min_gpus(cli_result):
- for df in cli_result.best_configs.values():
- if df is not None and not df.empty and "total_gpus_needed" in df.columns:
- return int(df["total_gpus_needed"].min())
- return 0
+ totals = [
+ int(df["total_gpus_needed"].min())
+ for df in cli_result.best_configs.values()
+ if df is not None and not df.empty and "total_gpus_needed" in df.columns
+ ]
+ return min(totals) if totals else 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def min_gpus(cli_result): | |
| for df in cli_result.best_configs.values(): | |
| if df is not None and not df.empty and "total_gpus_needed" in df.columns: | |
| return int(df["total_gpus_needed"].min()) | |
| return 0 | |
| low_gpus = min_gpus(result_low) | |
| high_gpus = min_gpus(result_high) | |
| def min_gpus(cli_result): | |
| totals = [ | |
| int(df["total_gpus_needed"].min()) | |
| for df in cli_result.best_configs.values() | |
| if df is not None and not df.empty and "total_gpus_needed" in df.columns | |
| ] | |
| return min(totals) if totals else 0 | |
| low_gpus = min_gpus(result_low) | |
| high_gpus = min_gpus(result_high) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_picking.py` around lines 224 - 231, Update the local
min_gpus helper to compute the minimum total_gpus_needed across every eligible
dataframe in cli_result.best_configs, rather than returning from the first
matching mode. Ignore missing, empty, or column-less dataframes, and preserve
the existing 0 result when no eligible values exist.
Source: Path instructions
b16813a to
8b2fab7
Compare
|
@coderabbitai Thanks for the thorough review. Here's what we addressed: Fixed:
Kept as-is (intentional):
|
|
Tip For best results, initiate chat on the files or code changes.
On the two "kept as-is" items — both rationales make sense to me:
I'll note these as learnings so future reviews of this codebase reflect these decisions. (=^･ω･^=) ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/aiconfigurator/cli/main.py (1)
388-548: 📐 Maintainability & Code Quality | 🔵 TrivialFix the remaining Ruff formatting drift before merge
ruff format --check .still wants to reformattests/integration/test_picking.pyandtests/unit/cli/test_argument_parsing.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiconfigurator/cli/main.py` around lines 388 - 548, Run Ruff formatting and apply its exact formatting changes to the argument definitions in _add_recommend_mode_arguments in src/aiconfigurator/cli/main.py (lines 388-548) and the related code in src/aiconfigurator/webapp/components/readme_tab.py (lines 55-60); also format tests/integration/test_picking.py and tests/unit/cli/test_argument_parsing.py so ruff format --check . passes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiconfigurator/cli/main.py`:
- Around line 398-410: Update the CLI definitions in the load_group for
--target-request-rate and --target-concurrency to use the existing shared
numeric validator, or add one if none exists, that accepts only finite values
greater than zero. Apply validation during argument parsing and add boundary
tests covering zero, negative, NaN, and infinity for both options.
- Line 1775: Update the model-fit failure guidance in cli_recommend so it does
not recommend --total-gpus, since recommend dispatch ignores args.total_gpus and
automatically escalates worker GPU budgets. Replace that suggestion with
actionable guidance relevant to load-target runs, while preserving the
quantized-model recommendation and surrounding message behavior.
In `@src/aiconfigurator/webapp/components/readme_tab.py`:
- Around line 57-60: Update the Sizing Recommender description to call request
rate and concurrency “load targets,” while identifying request rate specifically
as throughput; preserve the surrounding explanation of sizing inputs and
deployment configuration.
---
Nitpick comments:
In `@src/aiconfigurator/cli/main.py`:
- Around line 388-548: Run Ruff formatting and apply its exact formatting
changes to the argument definitions in _add_recommend_mode_arguments in
src/aiconfigurator/cli/main.py (lines 388-548) and the related code in
src/aiconfigurator/webapp/components/readme_tab.py (lines 55-60); also format
tests/integration/test_picking.py and tests/unit/cli/test_argument_parsing.py so
ruff format --check . passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2ac251ff-3dff-4283-a32e-a8e38c11280c
📒 Files selected for processing (14)
README.mddocs/cli_user_guide.mdsrc/aiconfigurator/cli/__init__.pysrc/aiconfigurator/cli/api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/cli/report_and_save.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/events/event_fn.pysrc/aiconfigurator/webapp/events/event_handler.pysrc/aiconfigurator/webapp/main.pytests/integration/test_picking.pytests/unit/cli/test_argument_parsing.pytests/unit/cli/test_cli_api.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/aiconfigurator/cli/init.py
- src/aiconfigurator/webapp/events/event_handler.py
- src/aiconfigurator/webapp/events/event_fn.py
- docs/cli_user_guide.md
- README.md
- src/aiconfigurator/cli/api.py
- src/aiconfigurator/cli/report_and_save.py
- tests/integration/test_picking.py
- tests/unit/cli/test_argument_parsing.py
- tests/unit/cli/test_cli_api.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
**/*: Treat this file as the only always-injected rule file; do not add new always-on rules here without human approval. New rule files must includepaths:frontmatter.
When reviewing changes in a governed area, read that area's rule files first, even if path-based auto-loading does not occur during a read-only review. Rule violations are review findings even when the code works.
A task must remain within its module: collector tasks may change onlycollector/and its tests, generator tasks onlysrc/aiconfigurator/generator/and its tests, and SDK tasks must not modify either. Cross-module contract changes require explicit human approval.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/cli/main.py
⚙️ CodeRabbit configuration file
**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.
- Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
- If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/cli/main.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Run
ruff check .andruff format --check .to validate Python linting and formatting.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/main.pysrc/aiconfigurator/cli/main.py
src/aiconfigurator/webapp/**
⚙️ CodeRabbit configuration file
src/aiconfigurator/webapp/**: - Verify UI changes preserve support-matrix navigation, filtering, and rendering behavior across local static previews and the webapp.
- Check that webapp flows handle missing Git LFS performance data gracefully when the change touches default-mode or support data loading.
Files:
src/aiconfigurator/webapp/components/recommender_tab.pysrc/aiconfigurator/webapp/components/readme_tab.pysrc/aiconfigurator/webapp/main.py
src/aiconfigurator/cli/main.py
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
When adding a generator parameter, expose it through the CLI, using
--generator-setwhen applicable.
Files:
src/aiconfigurator/cli/main.py
src/aiconfigurator/cli/**
⚙️ CodeRabbit configuration file
src/aiconfigurator/cli/**: - Check that CLI argument changes preserve backward compatibility, validation behavior, defaults, and plain-output expectations.
- For new or changed user-facing options, verify docs updates and generator/SDK wiring.
- Watch for non-TTY assumptions in tests or output formatting.
Files:
src/aiconfigurator/cli/main.py
🔇 Additional comments (5)
src/aiconfigurator/webapp/components/recommender_tab.py (2)
37-47: Default values still violate the "exactly one" contract for rate vs. concurrency.
target_request_ratedefaults to10whiletarget_concurrencydefaults toNone. If a user only fills concurrency,cli_recommendwill raise, and the failure only surfaces as a raw traceback in the debug box.🛠️ Proposed fix
target_request_rate = gr.Number( - value=10, + value=None, label="Target request rate (req/s)", info="Target system throughput. Fill this OR target concurrency, not both.", )
1-36: LGTM!Also applies to: 49-75
src/aiconfigurator/webapp/main.py (1)
77-77: LGTM!Also applies to: 86-86, 127-130, 169-170, 198-199
src/aiconfigurator/cli/main.py (2)
1175-1187: LGTM!
2522-2566: 📐 Maintainability & Code QualityRecord approval for the cross-module contract change.
Please link the required explicit human approval for the new CLI-to-SDK recommendation interface and associated web workflow. A code suggestion is not safe because the next step is review metadata, not a source edit. As per coding guidelines, “Cross-module contract changes require explicit human approval.”
Source: Coding guidelines
| load_group = parser.add_mutually_exclusive_group(required=True) | ||
| load_group.add_argument( | ||
| "--target-request-rate", | ||
| type=float, | ||
| default=None, | ||
| help="Target system request rate in req/s. Find minimum GPUs to serve this rate.", | ||
| ) | ||
| load_group.add_argument( | ||
| "--target-concurrency", | ||
| type=float, | ||
| default=None, | ||
| help="Target number of concurrent users. Find minimum GPUs to serve this concurrency.", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-positive and non-finite load targets.
float accepts 0, negatives, nan, and infinity; all pass the mutually-exclusive check and can produce nonsensical procurement sizing. Validate both targets as positive finite values at parse time. A one-click patch is not safe without checking for an existing shared CLI numeric validator and adding boundary tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/cli/main.py` around lines 398 - 410, Update the CLI
definitions in the load_group for --target-request-rate and --target-concurrency
to use the existing shared numeric validator, or add one if none exists, that
accepts only finite values greater than zero. Apply validation during argument
parsing and add boundary tests covering zero, negative, NaN, and infinity for
both options.
Source: Path instructions
| ### Sizing Recommender tab | ||
| Given a model, system, backend, workload, and a throughput target (request rate or concurrency), | ||
| find the minimum number of GPUs and optimal deployment configuration (agg vs disagg, TP/PP/DP). | ||
| Designed for procurement sizing — the output is unconstrained. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Call concurrency a load target, not throughput.
Concurrency and request rate are alternative load targets; only request rate is throughput.
Proposed patch
- Given a model, system, backend, workload, and a throughput target (request rate or concurrency),
+ Given a model, system, backend, workload, and a load target (request rate or concurrency),As per path instructions, “When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Sizing Recommender tab | |
| Given a model, system, backend, workload, and a throughput target (request rate or concurrency), | |
| find the minimum number of GPUs and optimal deployment configuration (agg vs disagg, TP/PP/DP). | |
| Designed for procurement sizing — the output is unconstrained. | |
| ### Sizing Recommender tab | |
| Given a model, system, backend, workload, and a load target (request rate or concurrency), | |
| find the minimum number of GPUs and optimal deployment configuration (agg vs disagg, TP/PP/DP). | |
| Designed for procurement sizing — the output is unconstrained. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/webapp/components/readme_tab.py` around lines 57 - 60,
Update the Sizing Recommender description to call request rate and concurrency
“load targets,” while identifying request rate specifically as throughput;
preserve the surrounding explanation of sizing inputs and deployment
configuration.
Source: Path instructions
Inverts the default AIC flow: instead of "given GPUs, estimate performance", the recommender answers "given performance targets, find minimum GPUs." Sweeps per-worker parallelism configs and uses pick_load_match to compute unconstrained replica counts for procurement sizing decisions. Adds cli_recommend() Python API, `aiconfigurator cli recommend` CLI subcommand, Gradio webapp tab (--enable-recommender), load-match display fixes in report_and_save.py, and unit + integration tests. Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8b2fab7 to
4abf8de
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/aiconfigurator/cli/main.py (1)
398-410: 🎯 Functional Correctness | 🟡 MinorReject non-positive and non-finite load targets at parse time.
type=floataccepts0, negative values,nan, and infinity; all pass the mutually exclusive check and can reach sizing with invalid procurement targets. This is the same still-valid finding from the previous review—use a shared positive-finite validator and add boundary tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiconfigurator/cli/main.py` around lines 398 - 410, Replace the raw float parsing for --target-request-rate and --target-concurrency in load_group with a shared validator that accepts only finite values greater than zero and raises the parser’s argument error for invalid inputs, including zero, negatives, NaN, and infinities. Add boundary tests covering invalid and valid target values.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/aiconfigurator/cli/main.py`:
- Around line 398-410: Replace the raw float parsing for --target-request-rate
and --target-concurrency in load_group with a shared validator that accepts only
finite values greater than zero and raises the parser’s argument error for
invalid inputs, including zero, negatives, NaN, and infinities. Add boundary
tests covering invalid and valid target values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20821401-d8f1-4dbf-8197-44f42238109e
📒 Files selected for processing (3)
src/aiconfigurator/cli/api.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/webapp/components/readme_tab.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/aiconfigurator/webapp/components/readme_tab.py
- src/aiconfigurator/cli/api.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Build and Test (unit)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Cargo Deny
- GitHub Check: Build and Test (e2e)
🧰 Additional context used
📓 Path-based instructions (4)
src/aiconfigurator/cli/main.py
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
When adding a generator parameter, expose it through the CLI, using
--generator-setwhen applicable.
Files:
src/aiconfigurator/cli/main.py
**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
**/*: Treat this file as the only always-injected rule file; do not add new always-on rules here without human approval. New rule files must includepaths:frontmatter.
When reviewing changes in a governed area, read that area's rule files first, even if path-based auto-loading does not occur during a read-only review. Rule violations are review findings even when the code works.
A task must remain within its module: collector tasks may change onlycollector/and its tests, generator tasks onlysrc/aiconfigurator/generator/and its tests, and SDK tasks must not modify either. Cross-module contract changes require explicit human approval.
Files:
src/aiconfigurator/cli/main.py
⚙️ CodeRabbit configuration file
**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.
- Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
- If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.
Files:
src/aiconfigurator/cli/main.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Run
ruff check .andruff format --check .to validate Python linting and formatting.
Files:
src/aiconfigurator/cli/main.py
src/aiconfigurator/cli/**
⚙️ CodeRabbit configuration file
src/aiconfigurator/cli/**: - Check that CLI argument changes preserve backward compatibility, validation behavior, defaults, and plain-output expectations.
- For new or changed user-facing options, verify docs updates and generator/SDK wiring.
- Watch for non-TTY assumptions in tests or output formatting.
Files:
src/aiconfigurator/cli/main.py
🔇 Additional comments (4)
src/aiconfigurator/cli/main.py (4)
398-410: 🎯 Functional CorrectnessConfirm the intended default target behavior.
The required group and
default=Nonefor both options meanrecommendalways requires an explicit load target, which conflicts with the stated retained default request rate of 10. Either restore that default or update the PR’s documented behavior and tests.
511-514: 🎯 Functional CorrectnessValidate
--nextn-accept-ratesbefore dispatch.The help text promises five rates, but the CLI only splits and casts values. Verify that
cli_recommendrejects incorrect lengths and non-finite/out-of-range rates; otherwise malformed input can escape argparse or surface as a rawValueError.Also applies to: 2561-2561
1772-1780: LGTM!
388-548: 📐 Maintainability & Code QualityRuff checks pass.
- Validate load targets as positive finite values (reject 0, nan, inf) - Do not suggest --total-gpus in recommend mode error messages - Fix readme tab wording: load target, not throughput target Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dc73340 to
22160ef
Compare
1 participant
Overview:
Adds a new
recommendCLI mode and webapp tab ("Sizing Recommender") that inverts AIC's default flow: instead of "given N GPUs, estimate performance," it answers "given a performance target, find the minimum GPUs and optimal deployment configuration." Designed as a procurement sizing tool where the output is unconstrained — the answer might be 8 GPUs or 512.Details:
SDK API (
cli/api.py):cli_recommend()function with the same inputs ascli_defaultminustotal_gpus, plustarget_request_rate/target_concurrency(mutually exclusive).build_default_tasks, then appliespick_load_matchto computereplicas_needed = ceil(target_rate / per_replica_rate)andtotal_gpus_needed = replicas * gpus_per_replica.defaultmode), since PP>1 can yield up to 36% better throughput/GPU under relaxed TTFT constraints. The SLA filter handles the tradeoff — tight TTFT still picks TP-heavy configs.chosen_expby fewest total GPUs (not highest throughput) since that's the metric procurement users care about.CLI (
cli/main.py):recommendsubparser with--target-request-rate/--target-concurrencymutually exclusive group (one required).main()callscli_recommend()and reusessave_results()for output.Display fixes (
cli/report_and_save.py):replicas_needed/total_gpus_neededfrom the DataFrame instead of the sweep ceiling for table display and summary output.replicas_neededis present.Webapp (
webapp/):update_model_capabilitiesevent handler auto-detects MoE/nextn/wideep on model change.Tests:
Where should the reviewer start?
src/aiconfigurator/cli/api.py—_build_recommend_tasks()helper andcli_recommend()are the core logic (~250 lines)src/aiconfigurator/cli/report_and_save.py— display fixes for load-match mode in_plot_worker_setup_tableandlog_final_summarysrc/aiconfigurator/webapp/components/recommender_tab.py— new Gradio tabSummary by CodeRabbit
recommendmode to the CLI and Python API for procurement-style GPU sizing from request-rate or concurrency + SLA targets, with ranked output and optional export.recommendusage and API calls.