DO NOT MERGE: discussion needed #368
Conversation
WalkthroughThe function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryEndpoint
participant ModelList
Client->>QueryEndpoint: Send QueryRequest (with model_id, provider_id)
QueryEndpoint->>ModelList: Get available models
QueryEndpoint->>QueryEndpoint: For each model, log details and check if model_id & provider_id match
QueryEndpoint-->>Client: Return selected model_id and provider_id
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
lightspeed-core/lightspeed-reference-ui#6 is an example of changing the client logic to use the correct llamastack model identifier. |
![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
🔭 Outside diff range comments (1)
src/app/endpoints/query.py (1)
174-193: Fix validation logic, typing, and lint; add backward-compat and safe fallback for invalid defaults
- Removes the untyped local function (mypy error) and long log line (pylint).
- Validates by Llama Stack model identifier (model_id) as primary, with optional provider check.
- Adds compatibility for:
- pre-populated identifiers in the form "provider_id/model_id".
- explicit provider_resource_id + provider_id pairs (logs still show provider_resource_id for debugging).
- Falls back to the first available LLM only when request provided no model/provider and configuration defaults are invalid (aligns with prior behavior and fixes unit test failures).
- Normalizes the return to the resolved model’s identifier and provider_id.
@@ - # Validate that the model_id and provider_id are in the available models - logger.debug("Searching for model: %s, provider: %s", model_id, provider_id) - - def check_model(m): - logger.debug("Available model - model_identifier: %s, provider_model_id: %s, provider_id: %s", m.identifier, m.provider_resource_id, m.provider_id) - return m.identifier == model_id and m.provider_id == provider_id - - if not any(check_model(m) for m in models): - message = f"Model {model_id} from provider {provider_id} not found in available models" - logger.error(message) - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail={ - "response": constants.UNABLE_TO_PROCESS_RESPONSE, - "cause": message, - }, - ) - - return model_id, provider_id + # Validate that the model_id and provider_id are in the available models. + logger.debug("Searching for model: %s, provider: %s", model_id, provider_id) + + # Prefer validation by Llama Stack model identifier (m.identifier). + selected_model = None + for m in models: + logger.debug( + "Available model - identifier=%s, provider_model_id=%s, provider_id=%s", + m.identifier, # pyright: ignore[reportAttributeAccessIssue] + getattr(m, "provider_resource_id", None), + m.provider_id, # pyright: ignore[reportAttributeAccessIssue] + ) + if ( + m.identifier == model_id # pyright: ignore[reportAttributeAccessIssue] + and (provider_id is None or m.provider_id == provider_id) # pyright: ignore[reportAttributeAccessIssue] + ): + selected_model = m + break + + # Back-compat: accept "provider_id/model_id" for pre-populated identifiers. + if selected_model is None and provider_id: + combined_identifier = f"{provider_id}/{model_id}" + selected_model = next( + ( + m + for m in models + if m.identifier == combined_identifier # pyright: ignore[reportAttributeAccessIssue] + ), + None, + ) + + # Back-compat: accept provider_resource_id when explicitly paired with provider_id. + if selected_model is None and provider_id: + selected_model = next( + ( + m + for m in models + if ( + getattr(m, "provider_resource_id", None) == model_id + and m.provider_id == provider_id # pyright: ignore[reportAttributeAccessIssue] + ) + ), + None, + ) + + if selected_model is None: + # If request didn't specify model/provider and configuration-provided defaults are invalid, + # fall back to the first available LLM to preserve previous behavior. + if query_request.model is None and query_request.provider is None: + try: + model = next( + m + for m in models + if m.model_type == "llm" # pyright: ignore[reportAttributeAccessIssue] + ) + logger.warning( + "Default model '%s' (provider '%s') not found. Falling back to " + "'%s' (provider '%s').", + model_id, + provider_id, + model.identifier, # pyright: ignore[reportAttributeAccessIssue] + model.provider_id, # pyright: ignore[reportAttributeAccessIssue] + ) + return model.identifier, model.provider_id # pyright: ignore[reportAttributeAccessIssue] + except (StopIteration, AttributeError) as e: + message = "No LLM model found in available models" + logger.error(message) + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "response": constants.UNABLE_TO_PROCESS_RESPONSE, + "cause": message, + }, + ) from e + + message = ( + f"Model {model_id} from provider {provider_id} not found in available models" + ) + logger.error(message) + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "response": constants.UNABLE_TO_PROCESS_RESPONSE, + "cause": message, + }, + ) + + # Normalize outputs to LLS identifiers. + return ( + selected_model.identifier, # pyright: ignore[reportAttributeAccessIssue] + selected_model.provider_id, # pyright: ignore[reportAttributeAccessIssue] + )Notes:
- This keeps model_id (LLS identifier) as the client-facing ID per PR objective.
- It also resolves the pipeline errors reported (mypy, pylint, unit tests).
- Please add/adjust tests and docs to reflect the clarified semantics (model_id primary, provider optional; limited compatibility kept).
🧹 Nitpick comments (2)
src/app/endpoints/query.py (2)
178-178: Pylint long line; wrap the debug log fieldsThe debug log line exceeds 100 chars (155). The refactor above wraps it; if you keep the current approach, split the format string and args across lines.
1-1: Black is failing on this fileRun Black to normalize formatting: black --line-length 100 .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/endpoints/query.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Black
src/app/endpoints/query.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black --write' to fix code style issues.
🪛 GitHub Actions: Type checks
src/app/endpoints/query.py
[error] 177-177: mypy: Function is missing a type annotation. (no-untyped-def)
[error] 181-181: mypy: Call to untyped function "check_model" in typed context. (no-untyped-call)
🪛 GitHub Actions: Python linter
src/app/endpoints/query.py
[error] 176-176: Pylint: Trailing whitespace (C0303)
[error] 178-178: Pylint: Line too long (155/100) (C0301)
[error] 180-180: Pylint: Trailing whitespace (C0303)
🪛 GitHub Actions: Unit tests
src/app/endpoints/query.py
[error] 184-184: Runtime error: HTTPException 400 raised due to model 'model2' from provider 'provider2' not found in available models.
[error] 184-184: Runtime error: HTTPException 400 raised due to model 'default_model' from provider 'default_provider' not found in available models.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
|
|
||
| def check_model(m): | ||
| logger.debug("Available model - model_identifier: %s, provider_model_id: %s, provider_id: %s", m.identifier, m.provider_resource_id, m.provider_id) | ||
| return m.identifier == model_id and m.provider_id == provider_id | ||
|
|
||
| if not any(check_model(m) for m in models): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trailing whitespace and mypy untyped local function
- Trailing whitespace on Line 176 and 180 (pylint C0303).
- Local function
check_modellacks annotations and is called in a typed context (mypy no-untyped-def, no-untyped-call).
Either annotate it (param type and return type) or remove it. The proposed refactor removes it entirely; alternatively:
- def check_model(m):
+ def check_model(m: object) -> bool:
logger.debug(
"Available model - model_identifier: %s, provider_model_id: %s, provider_id: %s",
m.identifier, m.provider_resource_id, m.provider_id
)
return m.identifier == model_id and m.provider_id == provider_id📝 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 check_model(m): | |
| logger.debug("Available model - model_identifier: %s, provider_model_id: %s, provider_id: %s", m.identifier, m.provider_resource_id, m.provider_id) | |
| return m.identifier == model_id and m.provider_id == provider_id | |
| if not any(check_model(m) for m in models): | |
| def check_model(m: object) -> bool: | |
| logger.debug("Available model - model_identifier: %s, provider_model_id: %s, provider_id: %s", | |
| m.identifier, m.provider_resource_id, m.provider_id) | |
| return m.identifier == model_id and m.provider_id == provider_id |
🧰 Tools
🪛 GitHub Actions: Type checks
[error] 177-177: mypy: Function is missing a type annotation. (no-untyped-def)
[error] 181-181: mypy: Call to untyped function "check_model" in typed context. (no-untyped-call)
🪛 GitHub Actions: Python linter
[error] 176-176: Pylint: Trailing whitespace (C0303)
[error] 178-178: Pylint: Line too long (155/100) (C0301)
[error] 180-180: Pylint: Trailing whitespace (C0303)
🤖 Prompt for AI Agents
In src/app/endpoints/query.py around lines 176 to 181, remove trailing
whitespace on lines 176 and 180 to fix pylint C0303. Add type annotations to the
local function check_model by specifying the parameter type and return type to
satisfy mypy no-untyped-def and no-untyped-call checks. Alternatively, consider
removing the local function if it is not necessary.
|
github issue for the upstream llamastack behavior aspects of this: |
|
i've opened https://issues.redhat.com/browse/LCORE-600 to track resolving this behavior. |
Use the llamastack identifier for the model id, not the provider's model id
Description
see:https://llama-stack.readthedocs.io/en/latest/distributions/configuration.html#resources
If we look at the LLS config api we have, for example:
which results in this in the model list:
Note that other prepopulated models look like:
As far as LLS is concerned, the model it is aware of is identified by the "identifier" aka "model_id". The "provider_model_id" should not be exposed to clients.
When i want to tell LCORE and LLS that i want to use the "benbot" model, i should be referring to it as "benbot", not "gpt-4o-mini".
if i want to use the prepopulated "gpt-4-turbo" model from openai, i should use the model_id "openai/gpt-4-turbo" with a provider_id of "openai"
This code change attemps to implement that by modifying the model lookup logic.
Note that this may impact clients that were previously using the "provider_resource_id" reported by LLS + LCORE models apis, when calling back into LCORE. That is why this PR needs discussion.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Bug Fixes
Other