fix(chat): resolve the default chat LLM from the catalog default endpoint - #755
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Ahmath-Gadji
commented
Jul 22, 2026
hedhoud
approved these changes
Jul 22, 2026
Ahmath-Gadji
force-pushed
the
fix/754-chat-llm-default-endpoint
branch
from
July 22, 2026 13:34
c3799d9 to
3832c36
Compare
5 tasks
…oint
_resolve_llm's default branches returned the static self._llm — a client
built once at startup from settings.llm (the .env LLM_ENDPOINT/LLM_MODEL)
— so promoting a new default endpoint in the model-endpoint catalog had no
effect on the default chat path. Route the default through
llm_factory("default") (the live is_default endpoint, cache-invalidated on
every default change), falling back to the static client only when no
factory is wired (tests) or the catalog has no default yet.
Also log the resolved preset name on every path, including the default, so
the answering chat LLM is identifiable from the logs.
Closes #754
The default-path change makes chat answering use the catalog default while
_infer_relevancy stays on the static self._llm, so the two diverge. The
docstring claimed map-reduce "stays on the catalog default" (wrong) and the
_infer_relevancy comment ("pinned to the default LLM") was now ambiguous.
State the divergence explicitly in both.
Ahmath-Gadji
force-pushed
the
fix/754-chat-llm-default-endpoint
branch
from
July 22, 2026 13:42
3832c36 to
26352e1
Compare
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 22, 2026
check_llm_model_availability ran as a Depends() on /v1/chat/completions and /v1/completions, probing config.llm's /v1/models before every request. It only ever validated the default (.env) endpoint, so once a partition's chat_llm preset — or a non-.env catalog default (#755) — answers a request, it was checking the wrong endpoint: a partition on a healthy custom LLM got falsely rejected (503/404) whenever the default endpoint happened to be down, and its "Validating model" log misattributed the answering model, contradicting _resolve_llm's own "which chat_llm answered" log. The liveness it emulated already exists against the actually-resolved endpoint: the generation call wraps upstream failures into InferenceConnectionError / InferenceTimeoutError / InferenceError (503/504/502), surfaced by the global exception handler (non-streaming) and the streaming handler's SSE error path. Removing the preflight loses no error contract (no test pinned the 404/503 behaviour) and drops a per-request /v1/models round-trip on the healthy path. get_openai_models stays — the per-endpoint token preflight (_fetch_max_model_tokens) still uses it.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 23, 2026
check_llm_model_availability ran as a Depends() on /v1/chat/completions and /v1/completions, probing config.llm's /v1/models before every request. It only ever validated the default (.env) endpoint, so once a partition's chat_llm preset — or a non-.env catalog default (#755) — answers a request, it was checking the wrong endpoint: a partition on a healthy custom LLM got falsely rejected (503/404) whenever the default endpoint happened to be down, and its "Validating model" log misattributed the answering model, contradicting _resolve_llm's own "which chat_llm answered" log. The liveness it emulated already exists against the actually-resolved endpoint: the generation call wraps upstream failures into InferenceConnectionError / InferenceTimeoutError / InferenceError (503/504/502), surfaced by the global exception handler (non-streaming) and the streaming handler's SSE error path. Removing the preflight loses no error contract (no test pinned the 404/503 behaviour) and drops a per-request /v1/models round-trip on the healthy path. get_openai_models stays — the per-endpoint token preflight (_fetch_max_model_tokens) still uses it.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 24, 2026
Mirrors QueryService._resolve_llm/_default_llm (#755): log (debug) which reranker resolved for a partition — its own preset, the catalog default, or the static startup reranker — so "which reranker ran?" is answerable from logs the same way chat_llm already is. Also closes the same gap #755 fixed for chat_llm, on the reranker side: a partition's `reranker` preset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching `_reranker_factory` raised an unhandled KeyError instead of falling back to the catalog default (and then the static reranker if that's missing too).
4 tasks
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
Mirrors QueryService._resolve_llm/_default_llm (#755): log (debug) which reranker resolved for a partition — its own preset, the catalog default, or the static startup reranker — so "which reranker ran?" is answerable from logs the same way chat_llm already is. Also closes the same gap #755 fixed for chat_llm, on the reranker side: a partition's `reranker` preset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching `_reranker_factory` raised an unhandled KeyError instead of falling back to the catalog default (and then the static reranker if that's missing too).
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
RetrievalService now resolves a partition's reranker the same way QueryService._resolve_llm/_default_llm (#755) resolves chat_llm: the partition's configured preset first, falling back to the catalog default, then the static startup reranker, with the resolved endpoint logged at debug — so "which reranker ran?" is answerable from logs the way chat_llm already is. This also closes the gap #755 fixed for chat_llm, on the reranker side: a partition's `reranker` preset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching `_reranker_factory` raised an unhandled KeyError instead of falling back to the catalog default (and then the static reranker if that's missing too). The fallback warning binds reranker/partition as structured Loguru context via `.bind()` rather than passing them as message-format kwargs — passed directly to `logger.warning(msg, key=val)`, they were silently dropped since the message has no `{}` placeholders to substitute into, so the log never actually recorded which reranker/partition triggered the fallback.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
RetrievalService now resolves a partition's reranker the same way QueryService._resolve_llm/_default_llm (#755) resolves chat_llm: the partition's configured preset first, falling back to the catalog default, then the static startup reranker, with the resolved endpoint logged at debug — so "which reranker ran?" is answerable from logs the way chat_llm already is. This also closes the gap #755 fixed for chat_llm, on the reranker side: a partition's `reranker` preset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching `_reranker_factory` raised an unhandled KeyError instead of falling back to the catalog default (and then the static reranker if that's missing too). The fallback warning binds reranker/partition as structured Loguru context via `.bind()` rather than passing them as message-format kwargs — passed directly to `logger.warning(msg, key=val)`, they were silently dropped since the message has no `{}` placeholders to substitute into, so the log never actually recorded which reranker/partition triggered the fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #754.
Problem
When a partition has no
chat_llmpreset ("Default (from retrieval config)"),QueryService._resolve_llm()returned the staticself._llm— a client built once at startup fromsettings.llm(the.envLLM_ENDPOINT/LLM_MODEL) indi/container.py. It never consulted the model-endpoint catalog, so promoting a new default endpoint in the catalog had no effect on the default chat path — chat kept answering with the.envmodel. Only setting a partition'schat_llmexplicitly worked (that path goes throughllm_factory(name)).Fix
services/orchestrators/query_service.py:llm_factory("default")(theis_default=Trueendpoint, kept in sync byModelEndpointService.load_all()and cache-invalidated on every default change). Falls back to the staticself._llmonly when no factory is wired (unit tests) or the catalog has no default yet.chat_llmnow also falls through to the catalog default (was the static.envclient)._agreed_partition_chat_llm(),_default_llm(),_default_llm_name().Answering with the default chat_llm preset · chat_llm=<endpoint>) — so the answering chat LLM is identifiable from the logs.Map-reduce relevancy (
_infer_relevancy) stays deliberately pinned to the static default — pre-existing documented behavior, unchanged here.Live verification
Two endpoints in the catalog (same host, distinct
model_name). Partitiontestleft onchat_llm=nullthe whole time; only the catalog default was flipped:_resolve_llmlogis_default)Answering with the default chat_llm preset · chat_llm=Mistral-Small-3.2-24B-Instruct-2506-FP8 · partitions=['test']set-default)Answering with the default chat_llm preset · chat_llm=Luciole · partitions=['test']The resolved LLM followed the catalog default (Mistral → Luciole) without touching the partition; the two different answers confirm a different endpoint actually served each request. Before this fix, the second row would still have resolved to the
.envMistral client.Tests
tests/unit/services/orchestrators/test_query_service.py— updated the default-path assertions to the corrected contract and added coverage for_default_llm/_default_llm_name. Suite green (45 in the file, 394 across orchestrators).ruff checkclean.