fix: embedder batch_size not applied + better embedding error diagnostics - #748
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Helm chart adds the ChangesEndpoint synchronization configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/orchestrators/model_endpoint_service.py (1)
96-107: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSkip redundant database updates when settings haven't changed.
Currently, when
sync_on_bootis enabled, the service writes to the database and emits a log message on every boot, even if the environment variables perfectly match the existing endpoint row. Consider adding an idempotency check to avoid unnecessary database I/O and startup noise.💡 Proposed idempotency check
if existing_row is not None: if sync_on_boot: - await self._repo.update( - name, - model_type, - endpoint=endpoint, - model_name=model_name or None, - batch_size=data.get("batch_size", 32), - timeout=data.get("timeout", 30.0), - ) - logger.info(f"Synced {model_type} endpoint '{name}' from env (MODEL_ENDPOINT_SYNC_ON_BOOT=true).") + new_model_name = model_name or None + new_batch_size = data.get("batch_size", 32) + new_timeout = data.get("timeout", 30.0) + + if ( + existing_row.endpoint != endpoint or + existing_row.model_name != new_model_name or + existing_row.batch_size != new_batch_size or + existing_row.timeout != new_timeout + ): + await self._repo.update( + name, + model_type, + endpoint=endpoint, + model_name=new_model_name, + batch_size=new_batch_size, + timeout=new_timeout, + ) + logger.info(f"Synced {model_type} endpoint '{name}' from env (MODEL_ENDPOINT_SYNC_ON_BOOT=true).") continue🤖 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 `@openrag/services/orchestrators/model_endpoint_service.py` around lines 96 - 107, Update the existing-row branch in the model endpoint sync flow to compare the environment-derived values against the persisted row before calling self._repo.update or logging synchronization. Only perform the update and “Synced” log when at least one relevant setting differs; otherwise continue without database I/O or startup noise.
🤖 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 `@openrag/services/orchestrators/model_endpoint_service.py`:
- Around line 96-107: Update the existing-row branch in the model endpoint sync
flow to compare the environment-derived values against the persisted row before
calling self._repo.update or logging synchronization. Only perform the update
and “Synced” log when at least one relevant setting differs; otherwise continue
without database I/O or startup noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0778f8c5-80d8-45ca-a2bb-6d62946a6f0e
📒 Files selected for processing (5)
openrag/core/config/loader.pyopenrag/core/config/model_endpoints.pyopenrag/services/inference/vllm_client.pyopenrag/services/orchestrators/model_endpoint_service.pytests/unit/services/orchestrators/test_model_endpoint_service.py
Replace the release/*-branch-gated, Docker-Hub-publishing upstream design with one that matches this fork's actual setup: any v*-rc.* tag push builds and publishes api/ray/admin-ui to ghcr only (no Docker Hub creds configured here), from whatever branch the tag points to.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build_rc.yml (1)
21-28: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueApply the principle of least privilege by specifying explicit permissions.
The
varsjob currently relies on default workflow permissions. Since this job only executes anechocommand and does not require access to the repository contents or the GitHub API, configure an emptypermissionsblock to restrict its token scope.As per static analysis hints, default permissions are used due to a missing permissions block.
🛡️ Proposed fix
vars: runs-on: ubuntu-latest + permissions: {} outputs: image_name: ${{ steps.vars.outputs.image_name }}🤖 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 @.github/workflows/build_rc.yml around lines 21 - 28, Add an explicit empty permissions block to the vars job before its steps, restricting the job token to no repository or API permissions while preserving the existing image_name computation and output.Source: Linters/SAST tools
🤖 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 @.github/workflows/build_rc.yml:
- Around line 21-28: Add an explicit empty permissions block to the vars job
before its steps, restricting the job token to no repository or API permissions
while preserving the existing image_name computation and output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9118f39-e73e-4cda-87d9-86b535c67c56
📒 Files selected for processing (1)
.github/workflows/build_rc.yml
docker/metadata-action adds a latest tag by default even with a plain type=ref,event=tag rule. Confirmed live: tagging v2.0.0-rc.5 just overwrote ghcr.io/thibautchoppy/openrag(-ray|-admin-ui):latest, despite this file's own header claiming it never tags latest. flavor: latest=false makes that true.
There was a problem hiding this comment.
I found a few issues that should be addressed before merging:
-
Model-name changes are not synchronized. The lookup uses the new model slug, so an endpoint stored under the previous model name is missed; because another endpoint exists, startup then skips the update. With sync enabled, changing the model through Helm still leaves the old database configuration active.
-
This PR now conflicts with develop in the endpoint startup logic. Develop added sampling-parameter backfilling for existing LLM/VLM endpoints. Please rebase and preserve that behavior when combining it with sync-on-boot, with a test covering both paths.
-
The RC workflow and Graphify ignore changes are unrelated and fork-specific. If merged upstream, RC images could be published from a tag on any branch and Docker Hub RC publishing would be removed, which conflicts with the documented release flow. Please revert these changes here or move them to a dedicated release-process PR.
-
The new warning persists the complete upstream response body and raw snippets from indexed documents. That can leak document content into stderr and centralized logs, and the response body is unbounded. Please keep non-sensitive identifiers such as batch index and character position, or gate bounded raw details behind an explicit debug option.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openrag/services/inference/vllm_client.py (2)
49-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAttach suspect escape findings to the embedding failure.
_find_suspect_escapes()is never used by the shown failure path: Lines 359-368 only attach the provider response, so Lines 300-308 cannot log malformed-escape indexes/snippets. Add a sanitized result toEmbeddingAPIError.extrawhen the request fails.🤖 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 `@openrag/services/inference/vllm_client.py` around lines 49 - 68, The embedding failure path must invoke _find_suspect_escapes() and attach its sanitized findings to EmbeddingAPIError.extra alongside the provider response. Update the request-failure handling near the existing response attachment, using the submitted input texts, so malformed-escape indexes and snippets are available to the logging path.
359-368: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHTTP 500 is still not retried.
This preserves
status_code=500, but the retry contract only retries 429, 502, 503, and 504. That contradicts the Line 360-361 claim that 5xx responses retry; transient provider 500s fail immediately. Extend the centralized retry policy if all intended server errors should retry, and cover 500, 429, and 4xx behavior in tests.🤖 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 `@openrag/services/inference/vllm_client.py` around lines 359 - 368, Update the centralized retry policy used by the embedding path to include HTTP 500 alongside 429, 502, 503, and 504, while keeping other 4xx responses non-retryable. Add or update tests covering retry behavior for 500 and 429 and fail-fast behavior for a representative 4xx response, using the existing retry-policy symbols and embedding client tests.
🤖 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.
Outside diff comments:
In `@openrag/services/inference/vllm_client.py`:
- Around line 49-68: The embedding failure path must invoke
_find_suspect_escapes() and attach its sanitized findings to
EmbeddingAPIError.extra alongside the provider response. Update the
request-failure handling near the existing response attachment, using the
submitted input texts, so malformed-escape indexes and snippets are available to
the logging path.
- Around line 359-368: Update the centralized retry policy used by the embedding
path to include HTTP 500 alongside 429, 502, 503, and 504, while keeping other
4xx responses non-retryable. Add or update tests covering retry behavior for 500
and 429 and fail-fast behavior for a representative 4xx response, using the
existing retry-policy symbols and embedding client tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d93edc58-51d2-4912-b4d4-83f559042f76
📒 Files selected for processing (5)
.gitignoreopenrag/core/config/loader.pyopenrag/services/inference/vllm_client.pyopenrag/services/orchestrators/model_endpoint_service.pytests/unit/services/orchestrators/test_model_endpoint_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- openrag/core/config/loader.py
- openrag/services/orchestrators/model_endpoint_service.py
|
Thanks for this — the embedding diagnostics and the "endpoint env vars silently ignored after first boot" find are both genuinely useful. +1 on splitting out the CI/build changes. The One heads-up on develop overlap for the app-code fix — this area moved recently, which is likely the source of the conflict:
Rebasing onto develop and reconciling |
…dpoint-sync-on-boot
These are fork-local (Docker Hub creds, lowercase-owner ghcr workaround, graphify tooling ignore) and were flagged by @hedhoud and @andyne13 on PR linagora#748 as out of scope for an upstream PR. The remaining app-code work was already cherry-picked, completed, and merged via linagora#758, so this leaves the PR with no diff against develop.
|
Unecessary changes on ci and gitignore have been removed. PR ready to close ;) |
Every other optional model-registry toggle is explicitly listed in the chart's env.config for discoverability (RERANKER_ENABLED, WITH_CHAINLIT_UI, ...). This one was missing entirely, so Helm-based deployments had no discoverable way to opt in even though the generic config map passthrough already supported it.
Requested changes dismissed after follow-up review.
4 participants
Context
Indexing a large PDF (1785 chunks) was failing at the embedding stage with an opaque 400 from the inference gateway, with no indication of the actual cause. Investigation happened in stages:
Embedder API error (400)— never the response body — so there was no way to know why the request was rejected.EMBEDDER_BATCH_SIZE(and the other model-endpoint env vars) were never actually applied to the indexing task: the model endpoint registry is seeded into the DB once on first boot, then becomes the source of truth — an env var change on a later rollout silently did nothing without an explicit admin API call.What changed
1. Richer embedding error logs (
openrag/services/inference/vllm_client.py)"Embedding failed after N/M batches"warning now includeserror_detail— the embedder's actual HTTP response body, which was already captured on the exception but never logged.\uescape sequence (the shape that trips some downstream JSON parsers even though our own JSON serialization is always valid) — surfaced assuspect_texts: [{index, snippet}]to pinpoint the offending input directly from the log line.2. Model endpoint config can now be hot-reloaded via a pod rollout (
core/config/model_endpoints.py,core/config/loader.py,services/orchestrators/model_endpoint_service.py)MODEL_ENDPOINT_SYNC_ON_BOOT(defaultfalse, existing behavior unchanged).true, on every boot the endpoint whose name matches the current env-derived slug (e.g.bge-m3) is resynced from Settings/env (endpoint,model_name,batch_size,timeout) — so a Helm/env value change plus a rollout is enough, no admin API call required.extra(API keys, implementation flags) is deliberately never touched by the sync, so a manually-set secret survives.Why opt-in: the model endpoint registry is meant to be admin-editable after first boot (supports multiple named endpoints per type, hand-tuned independently of env vars). Always resyncing from env on every boot would silently clobber that for anyone managing endpoints through the admin UI.
MODEL_ENDPOINT_SYNC_ON_BOOT=trueis for deployments (like ours) that manage this purely through Helm values + GitOps and want a plain rollout to be enough.Test plan
uv run ruff check openrag/ tests/uv run ruff format --check openrag/ tests/uv run python scripts/check_layer_imports.pyuv run pytest tests/unit/— 1894 passed🤖 Generated with Claude Code
Summary by CodeRabbit