LCORE-418: Allow configurating a default model/provider #284
Conversation
WalkthroughThe changes introduce configuration-based defaults for model and provider selection in the application, updating the selection logic, configuration schema, metrics setup, and corresponding tests. Additional validation ensures both default model and provider are set together. Logging is enhanced for traceability, and tests are expanded for more granular coverage of the new behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint
participant Config
participant ModelsList
Client->>Endpoint: Send QueryRequest (may include model/provider)
Endpoint->>Endpoint: select_model_and_provider_id()
alt Model/Provider in Request
Endpoint->>Endpoint: Use provided model/provider
else Defaults in Config
Endpoint->>Config: Get default_model/default_provider
Endpoint->>Endpoint: Use config defaults
else Fallback
Endpoint->>ModelsList: Get first available LLM model
Endpoint->>Endpoint: Use first LLM model/provider
end
Endpoint->>Endpoint: Validate selection
Endpoint-->>Client: Return selected model/provider or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
I think this change makes sense, but would like to discuss where to put the provider + model configuration. Basically llama_stack section was meant to be just for core<->lstack API, i.e. how to run Llama Stack and how to call it (as library or via REST API).
We have customization section, but probably some new section might be created? WDYT?
|
@tisnik makes sense, I can create a new section for this. Thanks for the review |
tisnik
changed the title
umago
force-pushed
the
LCORE-418-default-model-provider-config
branch
from
1c5dbf1 to
33b11eb
Compare
This patch allows for users to configure a default model/provider pair in the configuration file. Now models are selected as: * If no model/provider is specified in the configuration nor in the request, lightspeed-stack will use the FIRST MODEL AVAILABLE from llama-stack. * If the default model/provider is specified in the configuration file and a model/provider ARE NOT PROVIDED IN THE REQUEST, lightspeed-stack will use the model/provider FROM THE CONFIGURATION FILE. * If the default model/provider is specified in the configuration file and a model/provider ARE PROVIDED IN THE REQUEST, lightspeed-stack will use the model/provider FROM THE REQUEST. tl;dr the precedent order to use a model is: request, configuration, first available in llama-stack. Signed-off-by: Lucas Alvares Gomes <[email protected]>
umago
force-pushed
the
LCORE-418-default-model-provider-config
branch
from
33b11eb to
5943d1b
Compare
|
@tisnik I created a new section called "inference", for now it only have the default_model/provider but in the future we could add things like temperature, max_tokens, top_k, etc etc etc |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/unit/metrics/test_utis.py (1)
1-1: Rename the test file to correct the typoThe test filename in
tests/unit/metrics/test_utis.pyshould be updated to match the module name (utils.py):
- tests/unit/metrics/test_utis.py → tests/unit/metrics/test_utils.py
- tests/unit/metrics/test_utis.py + tests/unit/metrics/test_utils.py
🧹 Nitpick comments (1)
tests/unit/metrics/test_utis.py (1)
56-69: Assertion logic is correct but could be more robust.The assertions properly verify that only LLM models are processed and that the default model receives metric value 1 while others get 0. However, the chained call structure with
.labels().set()could be fragile.Consider using individual call assertions for better clarity:
- mock_metric.assert_has_calls( - [ - mocker.call.labels("test_provider-0", "test_model-0"), - mocker.call.labels().set(0), - mocker.call.labels("default_provider", "default_model"), - mocker.call.labels().set(1), - mocker.call.labels("test_provider-1", "test_model-1"), - mocker.call.labels().set(0), - ], - any_order=False, # Order matters here - ) + # Verify each model's metric setting individually + mock_metric.labels.assert_any_call("test_provider-0", "test_model-0") + mock_metric.labels.assert_any_call("default_provider", "default_model") + mock_metric.labels.assert_any_call("test_provider-1", "test_model-1") + + # Verify the labels().set() calls + assert mock_metric.labels().set.call_count == 3 + mock_metric.labels().set.assert_any_call(0) # for non-default models + mock_metric.labels().set.assert_any_call(1) # for default model
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/endpoints/query.py(2 hunks)src/configuration.py(2 hunks)src/metrics/utils.py(2 hunks)src/models/config.py(2 hunks)tests/unit/app/endpoints/test_query.py(2 hunks)tests/unit/metrics/test_utis.py(1 hunks)tests/unit/models/test_config.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/configuration.py
- src/models/config.py
- src/app/endpoints/query.py
- tests/unit/models/test_config.py
- src/metrics/utils.py
- tests/unit/app/endpoints/test_query.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/metrics/test_utis.py (1)
src/metrics/utils.py (1)
setup_model_metrics(11-45)
🔇 Additional comments (2)
tests/unit/metrics/test_utis.py (2)
11-18: LGTM! Configuration patching is correctly implemented.The patches properly mock the new configuration paths for default model and provider, ensuring the test isolates the function's behavior from actual configuration dependencies.
21-52: Excellent test coverage with comprehensive mock setup.The mock models effectively test all scenarios:
- Default model identification (metric value 1)
- Non-default LLM models (metric value 0)
- Non-LLM model filtering (ignored)
The setup aligns perfectly with the function's logic.
2 participants
Description
This patch allows for users to configure a default model/provider pair in the configuration file.
Now models are selected as:
tl;dr the precedent order to use a model is: request, configuration, first available in llama-stack.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests