LCORE-689: Fixed wrong model name from conversation history #570
Conversation
WalkthroughThe tuple returned when selecting a model now includes a derived model_label as the second element, replacing the previous duplicate model_id. The label is parsed from model_id by taking the segment after the first slash if present; otherwise it equals model_id. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Q as Query Endpoint
participant R as Model Resolver
C->>Q: Query request
Q->>R: Resolve available model
R-->>Q: model_id, provider_id
Note over Q: Derive model_label = segment after first "/" in model_id<br/>or model_id if no "/"
Q-->>C: (model_id, model_label, provider_id)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![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
🧹 Nitpick comments (4)
src/app/endpoints/query.py (4)
291-297: Docstring “Returns” is outdated and misleading (now a 3‑tuple).Update to reflect
(llama_stack_model_id, model_label, provider_id)to avoid consumer confusion.- Returns: - A tuple containing the combined model ID (in the format - "provider/model") and the provider ID. + Returns: + tuple[str, str, str]: A tuple of: + - llama_stack_model_id: str (e.g., "provider/model") + - model_label: str (segment after the first "/", or the whole identifier) + - provider_id: str
217-233: Rename local tomodel_labelfor clarity; adjust downstream uses.The second element from
select_model_and_provider_id()is now a label, not a rawmodel_id. Renaming reduces cognitive load and prevents future misuse.- llama_stack_model_id, model_id, provider_id = select_model_and_provider_id( + llama_stack_model_id, model_label, provider_id = select_model_and_provider_id( await client.models.list(), *evaluate_model_hints( user_conversation=user_conversation, query_request=query_request ), ) @@ - metrics.llm_calls_total.labels(provider_id, model_id).inc() + metrics.llm_calls_total.labels(provider_id, model_label).inc() @@ - model_id=model_id, + model_id=model_label, provider_id=provider_id, @@ - model=model_id, + model=model_label, provider_id=provider_id,Also applies to: 237-256
325-327: Deduplicate model-label parsing via a small helper.
model_id.split("/", 1)logic appears twice. Extract a helper to keep the rule in one place.+def derive_model_label(identifier: str) -> str: + """Return the segment after the first '/', or the identifier if no slash is present.""" + return identifier.split("/", 1)[1] if "/" in identifier else identifier + @@ - model_label = model_id.split("/", 1)[1] if "/" in model_id else model_id + model_label = derive_model_label(model_id) return model_id, model_label, provider_id @@ - model_label = model_id.split("/", 1)[1] if "/" in model_id else model_id + model_label = derive_model_label(model_id)Also applies to: 526-529
45-45: Align logger name with guidelines.Prefer
logging.getLogger(__name__)for consistency and hierarchical control.-logger = logging.getLogger("app.endpoints.handlers") +logger = logging.getLogger(__name__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/endpoints/query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/app/endpoints/query.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/query.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/app/endpoints/query.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/query.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/query.py
⏰ 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). (1)
- GitHub Check: build-pr
🔇 Additional comments (5)
src/app/endpoints/query.py (5)
325-327: Good fix: stop propagating provider-prefixed model into history/metrics.Deriving
model_labelfrom the combined identifier and returning it separately addresses LCORE-689 as intended.
181-181: Verify that logging full Llama stack config cannot leak secrets.If
configuration.llama_stack_configurationcontains tokens/keys, log a redacted view or selected fields only.Redaction option:
- Keep provider id, default model, feature toggles
- Remove or mask tokens/host-level secrets
338-356: Consistency check: return shape is correct here.This branch returns
(llama_stack_model_id, model_label, provider_id)wheremodel_idis already a label. Good alignment with the new semantics.Consider adding a brief in-code comment to state the tuple contract explicitly.
400-434: Token metrics path matches new semantics.
retrieve_response()receives the combined ID and derivesmodel_labelinternally for metrics. This matches the refactor and avoids mislabeling.Also applies to: 526-529
110-149: Don't add provider/model parsing to evaluate_model_hints — last_used_model already contains the provider prefix; normalize in select_model_and_provider_id or add tests.The repository stores conversation last_used_model as "provider/model" (example: src/models/responses.py:497–498 contains "gemini/gemini-2.0-flash") and select_model_and_provider_id is the canonical selector (defined at src/app/endpoints/query.py:277–279 and called from query/streaming entrypoints). Do not split model/provider in evaluate_model_hints; instead ensure select_model_and_provider_id accepts combined identifiers (and add a unit test for request.model="provider/model" when provider is omitted).
Likely an incorrect or invalid review comment.
asimurka
force-pushed
the
conversation_history_wrong_model_fix
branch
from
3d40744 to
b439611
Compare
asimurka
force-pushed
the
conversation_history_wrong_model_fix
branch
from
b439611 to
80bf590
Compare
2 participants
Description
After starting a new conversation without specifying a model, the next query after the initial one filled in the attribute specifying the model in a bad format.
Fixing this bug required saving only a separate model label without the prefix defining the provider.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit