fix(embedding): observable in-process fallback + mode-tagged errors; dedup get_embedding - #312
Merged
Merged
Conversation
…bedding
Observability (design 1c):
- Emit a rate-limited (once/60s per process) WARNING when local/* mode
resolution falls back to inprocess because the daemon /health probe failed
(or the daemon serves a different model). The message names the probe
target, the failure reason, and that a second in-process model copy is
loaded. An explicit REFLEXIO_EMBEDDING_PROVIDER=inprocess is intentional and
does not warn.
- Tag the embedding error path with the resolved mode: the service-provider
unavailability warning and the _litellm_embedding raise sites now log
extra={"mode": ...} so failures are attributable to daemon vs in-process
vs cloud. Exception types and control flow are unchanged.
Dedup (Phase-3 item 1):
- get_embedding and get_embeddings now share one private _embed_texts dispatch
body; get_embedding is a thin [text] -> [0] wrapper. The only per-caller
divergence (batch vs single error-message wording) is preserved via a
`batch` flag, since existing tests assert both wordings verbatim — a plain
one-liner collapse would have changed those messages, so the shared body was
extracted instead. Embedding output semantics (routing, truncation, index
ordering) are byte-for-byte unchanged.
📝 WalkthroughWalkthroughThis PR refactors embedding dispatch in ChangesEmbedding dispatch and fallback
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant embedding_provider_mode
participant LocalServiceStatus
participant WarnFallback
participant GetServiceEmbeddings
Caller->>embedding_provider_mode: request local/* embedding
embedding_provider_mode->>LocalServiceStatus: check /health, active_model
alt reachable and model matches
LocalServiceStatus-->>embedding_provider_mode: local_service
embedding_provider_mode->>GetServiceEmbeddings: route via HTTP service
else mismatch or unreachable
LocalServiceStatus-->>embedding_provider_mode: failure reason
embedding_provider_mode->>WarnFallback: warn(model, reason)
WarnFallback-->>embedding_provider_mode: rate-limited warning emitted/suppressed
embedding_provider_mode-->>Caller: inprocess mode
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
reflexio/server/llm/_litellm_embedding.py (2)
364-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
!sconversion flag instead ofstr()in f-strings.Ruff RUF010 flags
str(e)inside f-strings on lines 366, 389, and 422. The idiomatic form is{e!s}.♻️ Proposed fix
raise _embedding_error( f"Nomic{' batch' if batch else ''} embedding generation " - f"failed: {str(e)}", + f"failed: {e!s}", mode, ) from eraise _embedding_error( f"Local{' batch' if batch else ''} embedding generation " - f"failed: {str(e)}", + f"failed: {e!s}", mode, ) from eraise _embedding_error( f"{'Batch embedding' if batch else 'Embedding'} generation " - f"failed: {str(e)}", + f"failed: {e!s}", mode, ) from eAlso applies to: 387-391, 420-424
🤖 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 `@reflexio/server/llm/_litellm_embedding.py` around lines 364 - 368, Replace the `str(e)` calls inside the f-strings in `_litellm_embedding.py` with the `!s` conversion flag, using the existing exception variable in `_embedding_error`-related raise paths (including the Nomic batch branch and the other matching embedding error branches). This keeps the message formatting idiomatic and satisfies Ruff RUF010 without changing the surrounding error handling or `raise ... from e` behavior.Source: Linters/SAST tools
348-354: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid redundant
embedding_provider_moderesolution.Line 348 resolves
mode = embedding_provider_mode(embedding_model), but line 351 callsshould_use_embedding_service(embedding_model)which internally callsembedding_provider_modeagain. Forlocal/*models this triggers a second daemon health probe (_local_service_status()) and a second_warn_inprocess_fallbackinvocation (suppressed by rate-limiting, but still wasteful). Sincemodeis already in hand, check it directly.♻️ Proposed fix
embedding_model = model or self._resolve_default_embedding_model() mode = embedding_provider_mode(embedding_model) if mode == "off": raise EmbeddingUnavailableError("Embedding provider is disabled") - if should_use_embedding_service(embedding_model): + if mode in _SERVICE_MODES: return get_service_embeddings( texts, model=embedding_model, dimensions=dimensions )If
_SERVICE_MODESis not currently imported, add it to the existing import fromembedding_service_provider. Alternatively, a local set check or a thinis_service_mode(mode)helper in the provider module would avoid exposing a private constant.🤖 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 `@reflexio/server/llm/_litellm_embedding.py` around lines 348 - 354, The embedding selection logic in the function that resolves `embedding_model` is doing duplicate provider resolution: `embedding_provider_mode(embedding_model)` is already computed, but `should_use_embedding_service(embedding_model)` rechecks it and can trigger extra local service probes. Update this branch to use the existing `mode` value directly to decide whether to call `get_service_embeddings`, and import or otherwise reference the service-mode set/helper from `embedding_service_provider` if needed so the check stays centralized without another `embedding_provider_mode` call.Source: Linters/SAST tools
reflexio/server/llm/providers/embedding_service_provider.py (1)
204-235: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider adding
extra={"mode": "inprocess"}to the fallback warning for log attribution consistency.The
_post_embedding_batchunavailability warning (lines 433-438) and the_embedding_errorhelper both attachextra={"mode": mode}to their log records, enabling downstream consumers to filter/group by resolved mode. The fallback warning here doesn't include this attribute, making it the only embedding-related WARNING without mode attribution. Adding it would let operators uniformly query all embedding warnings by mode.♻️ Optional: add mode extra to fallback warning
_LOGGER.warning( "Embedding daemon probe at %s failed for model %r; falling back to an " "in-process embedding model copy (a second model is loaded into this " "worker process). Probe failure: %s. Further fallback warnings are " "suppressed for %.0fs.", _local_service_url(), model, reason or "unknown", _INPROCESS_FALLBACK_WARN_INTERVAL_SECONDS, + extra={"mode": "inprocess"}, )🤖 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 `@reflexio/server/llm/providers/embedding_service_provider.py` around lines 204 - 235, The fallback warning in _warn_inprocess_fallback is missing the same log attribution used elsewhere for embedding warnings. Update the _LOGGER.warning call to include extra metadata with mode set to inprocess so downstream consumers can consistently filter and group logs by resolved mode, matching the behavior of _post_embedding_batch and _embedding_error.
🤖 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 `@reflexio/server/llm/_litellm_embedding.py`:
- Around line 364-368: Replace the `str(e)` calls inside the f-strings in
`_litellm_embedding.py` with the `!s` conversion flag, using the existing
exception variable in `_embedding_error`-related raise paths (including the
Nomic batch branch and the other matching embedding error branches). This keeps
the message formatting idiomatic and satisfies Ruff RUF010 without changing the
surrounding error handling or `raise ... from e` behavior.
- Around line 348-354: The embedding selection logic in the function that
resolves `embedding_model` is doing duplicate provider resolution:
`embedding_provider_mode(embedding_model)` is already computed, but
`should_use_embedding_service(embedding_model)` rechecks it and can trigger
extra local service probes. Update this branch to use the existing `mode` value
directly to decide whether to call `get_service_embeddings`, and import or
otherwise reference the service-mode set/helper from
`embedding_service_provider` if needed so the check stays centralized without
another `embedding_provider_mode` call.
In `@reflexio/server/llm/providers/embedding_service_provider.py`:
- Around line 204-235: The fallback warning in _warn_inprocess_fallback is
missing the same log attribution used elsewhere for embedding warnings. Update
the _LOGGER.warning call to include extra metadata with mode set to inprocess so
downstream consumers can consistently filter and group logs by resolved mode,
matching the behavior of _post_embedding_batch and _embedding_error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 623d8698-39b1-4e90-b329-afbc9abd018d
📒 Files selected for processing (4)
reflexio/server/llm/_litellm_embedding.pyreflexio/server/llm/providers/embedding_service_provider.pytests/server/llm/test_embedding_service_provider.pytests/server/llm/test_litellm_client_unit.py
This was referenced Aug 6, 2026
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
1 participant
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.
What
Two OSS-only quality items from the embedding-stability redesign (Phase 1, non-urgent follow-ups to the #311 lock fix):
Fallback observability (1c). When a
local/*model's daemon/healthprobe fails, mode resolution silently returnsinprocessand loads a second in-process model copy — invisible to operators. Now emits a rate-limited (≤1/60s) WARNING naming the probe target, the failure reason, and the second-copy implication. Fires only on the model-driven fallback, never on an explicitREFLEXIO_EMBEDDING_PROVIDER=inprocess(intentional, returns earlier). Adds amode=<resolved_mode>tag on embedding-error log/exception context so failures are attributable to the daemon vs in-process path. Exception types/messages/control-flow unchanged.Dedup
get_embedding/get_embeddings. Extracted a shared private_embed_texts(..., batch=); the two public methods now delegate (single passes[text], returns[0]). Signatures preserved. Note: extracted-to-helper rather than a plainget_embeddings([t])[0]collapse because the single/batch error-message wording differs and is asserted verbatim by existing tests — thebatch=flag selects the wording, so no message regression and no output change.Out of scope (different semantics — not a safe dedup): unifying
_truncate_and_renormalise/_pad/_resize_embedding.Tests
New: fallback-warns-once-then-rate-limits, explicit-inprocess-does-not-warn, mismatched-daemon-warns-with-model-reason; get_embedding==get_embeddings[0] parity; in-process failure carries
mode=.tests/server/llm/= 515 passed (baseline 507); ruff + pyright clean. No enterprise files, no embedding output semantics changed.Summary by CodeRabbit