feat: autodetect model name#905
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@eaa9028f4b350d3882625a5aad19fc0c22144462Recommended 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@eaa9028f4b350d3882625a5aad19fc0c22144462Last updated for commit: |
lianakoleva
force-pushed
the
feat/autodetect-model
branch
from
6602d8a to
a2dcf43
Compare
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughOptional model auto-detection for ChangesModel Auto-Detection Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. Comment |
lianakoleva
force-pushed
the
feat/autodetect-model
branch
from
59a7e4d to
274336b
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
lianakoleva
force-pushed
the
feat/autodetect-model
branch
from
e9b3ce9 to
cb4bfe3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/aiperf/cli_commands/profile.py (2)
131-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun readiness wait before model autodetect when
--modelis omitted.At Line 131, autodetect runs before Line 132 readiness probing. If the endpoint is still booting, omitted-model runs can fail before
--wait-for-model-timeouthas any effect.Suggested fix
- _maybe_autodiscover_models(user_config) - _maybe_wait_for_model(user_config) + _maybe_wait_for_model(user_config) + _maybe_autodiscover_models(user_config)🤖 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/aiperf/cli_commands/profile.py` around lines 131 - 132, The call order is reversed: _maybe_autodiscover_models runs before readiness probing, causing autodetect to fail if the endpoint is still booting; swap the calls so _maybe_wait_for_model(user_config) is invoked before _maybe_autodiscover_models(user_config). Update the sequence in the function that currently calls these (references: _maybe_autodiscover_models and _maybe_wait_for_model) to ensure readiness probing happens first when --model is omitted so the wait-for-model-timeout is honored.
56-60:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
artifact_directoryrecompute guard is likely using the wrong signal.Line 56 checks
model_fields_setto infer whether the user setartifact_directory, but at this stage that field can already be marked set by config computation. That can skip recomputation and leave autodetected runs writing to anauto-derived artifact path.Suggested direction
- if "artifact_directory" not in user_config.output.model_fields_set: + # Use an explicit "user provided artifact_directory" flag captured during CLI/config parse, + # not model_fields_set after defaults/computation have run. + if not user_config.output.user_provided_artifact_directory: user_config.output.artifact_directory = OutputDefaults.ARTIFACT_DIRECTORY user_config.output.artifact_directory = ( user_config._compute_artifact_directory() )#!/bin/bash # Verify where artifact_directory is assigned before this guard and how model_fields_set is used. rg -n -C4 'artifact_directory|model_fields_set|_compute_artifact_directory' src/aiperf/common/config src/aiperf/cli_commands/profile.pyIn Pydantic v2, after model initialization, does assigning to a field add that field to `model_fields_set`? Please cite official docs.🤖 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/aiperf/cli_commands/profile.py` around lines 56 - 60, The guard should not rely on user_config.output.model_fields_set (which can be populated by prior computation); instead detect whether artifact_directory is actually unset and only then recompute: replace the conditional with a check on the actual value (e.g. if not user_config.output.artifact_directory) and then call user_config.output.artifact_directory = user_config._compute_artifact_directory(); keep the fallback assignment to OutputDefaults.ARTIFACT_DIRECTORY only if the value is still falsy after computation; reference the symbols artifact_directory, user_config.output.model_fields_set, user_config.output.artifact_directory, and user_config._compute_artifact_directory when applying the change.
🤖 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/aiperf/common/models/model_autodetect.py`:
- Around line 76-77: The models_url construction currently appends "/v1/models"
blindly and can duplicate "/v1" when users pass a URL that already ends with
"/v1"; update the logic around base_url/models_url (variables: urls, base_url,
models_url in model_autodetect.py) to mirror readiness_probe.py by making
models_url = base_url + ("/models" if base_url.endswith("/v1") else
"/v1/models") or by stripping a trailing "/v1" from base_url before appending
"/v1/models" so that passing --url .../v1 produces the correct models URL.
---
Duplicate comments:
In `@src/aiperf/cli_commands/profile.py`:
- Around line 131-132: The call order is reversed: _maybe_autodiscover_models
runs before readiness probing, causing autodetect to fail if the endpoint is
still booting; swap the calls so _maybe_wait_for_model(user_config) is invoked
before _maybe_autodiscover_models(user_config). Update the sequence in the
function that currently calls these (references: _maybe_autodiscover_models and
_maybe_wait_for_model) to ensure readiness probing happens first when --model is
omitted so the wait-for-model-timeout is honored.
- Around line 56-60: The guard should not rely on
user_config.output.model_fields_set (which can be populated by prior
computation); instead detect whether artifact_directory is actually unset and
only then recompute: replace the conditional with a check on the actual value
(e.g. if not user_config.output.artifact_directory) and then call
user_config.output.artifact_directory =
user_config._compute_artifact_directory(); keep the fallback assignment to
OutputDefaults.ARTIFACT_DIRECTORY only if the value is still falsy after
computation; reference the symbols artifact_directory,
user_config.output.model_fields_set, user_config.output.artifact_directory, and
user_config._compute_artifact_directory when applying the change.
🪄 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: Pro
Run ID: b922e183-ca45-4028-bc56-28a355bc9640
📒 Files selected for processing (9)
docs/cli-options.mdsrc/aiperf/cli_commands/profile.pysrc/aiperf/common/config/endpoint_config.pysrc/aiperf/common/config/user_config.pysrc/aiperf/common/models/model_autodetect.pysrc/aiperf/common/readiness_probe.pytests/unit/common/config/test_endpoint_config.pytests/unit/common/test_model_autodetect.pytests/unit/common/test_readiness_probe.py
| Field( | ||
| ..., # This must be set by the user | ||
| description="Model name(s) to be benchmarked. Can be a comma-separated list or a single model name.", | ||
| default_factory=list, |
There was a problem hiding this comment.
Making model_names globally default to [] lets non-profile configs validate without a model even though runtime code still indexes endpoint.model_names[0], so aiperf service or programmatic runs now fail later with IndexError instead of a config error. Fix: keep EndpointConfig.model_names required outside the profile autodetect path, or add validation that only permits an empty list when autodetection will run before services start.
lianakoleva
force-pushed
the
feat/autodetect-model
branch
from
8d1b313 to
b70e58a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/aiperf/cli_commands/profile.py (1)
66-70: 💤 Low valueConsider removing the redundant assignment on line 67.
The assignment
user_config.output.artifact_directory = OutputDefaults.ARTIFACT_DIRECTORYis immediately overwritten by_compute_artifact_directory(). Unless there's a side effect from the first assignment that affects the computation, line 67 can be removed.♻️ Proposed simplification
if "artifact_directory" not in user_config.output.model_fields_set: - user_config.output.artifact_directory = OutputDefaults.ARTIFACT_DIRECTORY user_config.output.artifact_directory = ( user_config._compute_artifact_directory() )🤖 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/aiperf/cli_commands/profile.py` around lines 66 - 70, The assignment to user_config.output.artifact_directory using OutputDefaults.ARTIFACT_DIRECTORY is redundant because it is immediately overwritten by user_config._compute_artifact_directory(); remove the first assignment so only user_config.output.artifact_directory = user_config._compute_artifact_directory() runs inside the if block, keeping the check that "artifact_directory" not in user_config.output.model_fields_set unchanged and ensuring no unintended side effects from the redundant write.tests/unit/common/config/test_user_config.py (1)
280-300: ⚡ Quick winAdd return type hints to the new test functions.
Please annotate both test methods with
-> Noneto match the project-wide typing rule.Proposed patch
- def test_compute_config_does_not_mark_artifact_directory_as_user_set(self): + def test_compute_config_does_not_mark_artifact_directory_as_user_set(self) -> None: @@ - def test_explicit_artifact_directory_is_marked_as_user_set(self): + def test_explicit_artifact_directory_is_marked_as_user_set(self) -> None:As per coding guidelines, "Add type hints on ALL functions (params and return)".
🤖 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/unit/common/config/test_user_config.py` around lines 280 - 300, Add explicit return type annotations (-> None) to both test functions test_compute_config_does_not_mark_artifact_directory_as_user_set and test_explicit_artifact_directory_is_marked_as_user_set so they comply with the project-wide typing rule; update the function signatures to include the return type hint without changing any logic inside the functions.tests/unit/common/test_model_autodetect.py (1)
66-183: ⚡ Quick winUse
@pytest.mark.asyncio+awaitinstead ofasyncio.run()for these async tests.Per the repository testing conventions, all async tests should use the
@pytest.mark.asynciodecorator withawaitrather than wrapping the coroutine inasyncio.run(). This applies to all six test functions in this segment.Example conversion
-def test_autodetect_picks_first_id_from_data( +@pytest.mark.asyncio +async def test_autodetect_picks_first_id_from_data( monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: @@ - result = asyncio.run( - autodetect_names( - urls=["http://localhost:8000"], - headers={"Authorization": "Bearer token"}, - timeout_s=10.0, - ) - ) + result = await autodetect_names( + urls=["http://localhost:8000"], + headers={"Authorization": "Bearer token"}, + timeout_s=10.0, + )🤖 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/unit/common/test_model_autodetect.py` around lines 66 - 183, Convert the six tests that call asyncio.run(...) to proper async pytest tests: add the `@pytest.mark.asyncio` decorator, change the test functions to async def, and replace asyncio.run(autodetect_names(...)) with direct awaits (e.g., result = await autodetect_names(...)); do this for test_autodetect_picks_first_id_from_data, test_autodetect_single_model_logs_info_not_warning, test_autodetect_timeout_exhausted_on_non_200, test_autodetect_retries_on_non_200_then_succeeds, test_autodetect_retries_on_empty_data_then_succeeds, and test_autodetect_raises_valueerror_on_empty_urls (use with pytest.raises(TimeoutError, match="Timed out"): await autodetect_names(...) and for the ValueError test await inside pytest.raises).
🤖 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.
Nitpick comments:
In `@src/aiperf/cli_commands/profile.py`:
- Around line 66-70: The assignment to user_config.output.artifact_directory
using OutputDefaults.ARTIFACT_DIRECTORY is redundant because it is immediately
overwritten by user_config._compute_artifact_directory(); remove the first
assignment so only user_config.output.artifact_directory =
user_config._compute_artifact_directory() runs inside the if block, keeping the
check that "artifact_directory" not in user_config.output.model_fields_set
unchanged and ensuring no unintended side effects from the redundant write.
In `@tests/unit/common/config/test_user_config.py`:
- Around line 280-300: Add explicit return type annotations (-> None) to both
test functions test_compute_config_does_not_mark_artifact_directory_as_user_set
and test_explicit_artifact_directory_is_marked_as_user_set so they comply with
the project-wide typing rule; update the function signatures to include the
return type hint without changing any logic inside the functions.
In `@tests/unit/common/test_model_autodetect.py`:
- Around line 66-183: Convert the six tests that call asyncio.run(...) to proper
async pytest tests: add the `@pytest.mark.asyncio` decorator, change the test
functions to async def, and replace asyncio.run(autodetect_names(...)) with
direct awaits (e.g., result = await autodetect_names(...)); do this for
test_autodetect_picks_first_id_from_data,
test_autodetect_single_model_logs_info_not_warning,
test_autodetect_timeout_exhausted_on_non_200,
test_autodetect_retries_on_non_200_then_succeeds,
test_autodetect_retries_on_empty_data_then_succeeds, and
test_autodetect_raises_valueerror_on_empty_urls (use with
pytest.raises(TimeoutError, match="Timed out"): await autodetect_names(...) and
for the ValueError test await inside pytest.raises).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21361f4d-9445-4ad7-8109-0b11cf7f32f6
📒 Files selected for processing (10)
docs/cli-options.mdsrc/aiperf/cli_commands/profile.pysrc/aiperf/common/config/endpoint_config.pysrc/aiperf/common/config/user_config.pysrc/aiperf/common/models/model_autodetect.pysrc/aiperf/common/readiness_probe.pytests/unit/common/config/test_endpoint_config.pytests/unit/common/config/test_user_config.pytests/unit/common/test_model_autodetect.pytests/unit/common/test_readiness_probe.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/common/config/test_endpoint_config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/aiperf/common/config/endpoint_config.py
- src/aiperf/common/readiness_probe.py
- src/aiperf/common/config/user_config.py
lianakoleva
force-pushed
the
feat/autodetect-model
branch
2 times, most recently
from
7d7091a to
534c18f
Compare
Signed-off-by: Liana Koleva <43767763+lianakoleva@users.noreply.github.com>
Signed-off-by: Liana Koleva <43767763+lianakoleva@users.noreply.github.com>
lianakoleva
force-pushed
the
feat/autodetect-model
branch
2 times, most recently
from
8062941 to
2b60a71
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/aiperf/common/config/endpoint_config.py`:
- Around line 68-72: The validator for mode="after" currently checks
self.model_fields_set (symbols: model_fields_set, wait_for_model_timeout,
wait_for_model_interval, wait_for_model_mode), which false-positives when
defaults are present after YAML/JSON round-trips; change the check to compare
actual attribute values against EndpointDefaults (use
EndpointDefaults.wait_for_model_interval and
EndpointDefaults.wait_for_model_mode) instead of membership in model_fields_set:
if wait_for_model_timeout > 0 return early, otherwise compute which dependent
settings differ from their EndpointDefaults values and raise the same error only
when one or more of those values are non-default.
In `@src/aiperf/common/config/user_config.py`:
- Around line 544-545: The private attribute _gpu_telemetry_urls is defined with
a mutable default list (and other similar private lists around the file, e.g.,
the one noted at the later occurrence) which causes the list to be shared across
instances; change these to use Pydantic's PrivateAttr with a default_factory
(e.g., PrivateAttr(default_factory=list)) so each instance gets its own list,
and keep _gpu_telemetry_metrics_file as PrivateAttr(default=None) or appropriate
Optional/None default; update uses of _gpu_telemetry_urls and the other private
list attributes to reference the new PrivateAttr-backed fields (look for symbols
_gpu_telemetry_urls and any other private list attributes around the noted
position).
🪄 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: Pro
Run ID: 1c4fe99d-bf4f-421b-bdeb-30f13e1f2868
📒 Files selected for processing (7)
docs/cli-options.mdsrc/aiperf/cli_commands/profile.pysrc/aiperf/common/config/endpoint_config.pysrc/aiperf/common/config/user_config.pysrc/aiperf/common/models/model_autodetect.pysrc/aiperf/common/readiness_probe.pysrc/aiperf/config/flags/cli_config.py
✅ Files skipped from review due to trivial changes (2)
- src/aiperf/config/flags/cli_config.py
- docs/cli-options.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/aiperf/common/readiness_probe.py
- src/aiperf/common/models/model_autodetect.py
|
|
||
| # If the user didn't provide --model/--model-names, try to discover | ||
| # one from the server's OpenAI-compatible model list. | ||
| if not cli_config.model_names: |
There was a problem hiding this comment.
Autodetection runs before resolve_config, so aiperf profile --config benchmark.yaml with the model or URL supplied in YAML still probes CLI defaults and can replace the YAML model with an autodetected CLI override. Fix: resolve YAML+CLI config first and autodetect only when the merged config has no model, using the merged endpoint URLs.
| cli_config.model_names = asyncio.run( | ||
| autodetect_names( | ||
| urls=cli_config.urls, | ||
| headers={}, |
There was a problem hiding this comment.
Passing headers={} ignores --api-key and --header, so omitted-model runs fail against endpoints that require the same auth headers used by benchmark requests. Fix: construct autodetect headers from the resolved endpoint config, including custom headers and Authorization from api_key.
| def _models_url_from_base(base_url: str) -> str: | ||
| """Build the /v1/models URL, handling bases that already end in /v1.""" | ||
| _base = base_url.rstrip("/") | ||
| return _base + ("/models" if _base.endswith("/v1") else "/v1/models") |
There was a problem hiding this comment.
_models_url_from_base() handles bare /v1 bases but still turns accepted full endpoint URLs like http://host/v1/chat/completions into http://host/v1/chat/completions/v1/models, so omitted-model runs fail for a URL form the normal transport supports. Fix: strip recognized OpenAI endpoint paths back to their /v1 base or share the transport URL/path normalization before appending /models.
Signed-off-by: Liana Koleva <43767763+lianakoleva@users.noreply.github.com>
lianakoleva
force-pushed
the
feat/autodetect-model
branch
from
5828ac0 to
eaa9028
Compare
2 participants
Summary
aiperf profilewithGET /v1/modelsfrom specified--urlSummary by CodeRabbit
New Features
-m/--model-namesis now optional; the profile command will auto-detect models from the server (with retries and timeout) when omitted. Auto-detection selects the first discovered model and logs guidance.Bug Fixes
/v1.Documentation
Tests