fix(core): improve retrieval quality and pipeline observability#52
Merged
Conversation
The PersistentConversationStore was initialized but never registered
in the ProviderRegistry, causing GET /api/v1/conversations/{id} to
return 503 "not configured" even when VEKTRA_CONVERSATION_KEY was set.
Part of BUG-014.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tracks the missing create_conversation() call that causes PersistentConversationStore to silently discard all turns. Annotates BUG-010 as partially incomplete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Require consulting docs/reference/api.md or /openapi.json before constructing any API call to prevent auth, parameter, and endpoint mistakes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PersistentConversationStore.add_turn() requires an existing ConversationOrm row, but no one was creating it. All turns were silently discarded and multi-turn context was broken. Changes: - Add ensure_conversation() to PersistentConversationStore (upsert that creates the row only if it doesn't exist) - Extend create_conversation() to accept an optional conversation_id - Core API: create conversation before calling pipeline, using namespace_id and key_id from the auth layer - Learn API: ensure conversation exists using sentinel key_id (JWT auth has no API key) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The system prompt told the LLM not to reference sources, but the instruction was in English only and too generic. The model would use translated equivalents like "brani forniti" in Italian, which confuses users who don't know a retrieval system exists. Changes to system.j2: - Explicit blacklist of forbidden expressions in both English and Italian (extensible to other languages) - Reframe assistant persona as "knowledgeable" rather than "with access to a knowledge base" - Provide concrete alternative phrasings for partial-knowledge scenarios - Structure instructions with clear sections for context handling and user communication Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previous prompt still allowed the LLM to say "the text I consulted" and offer to "search for more" - both expose the retrieval mechanism or make promises the system cannot keep. Changes: - Blacklist "consulted/reviewed" phrasing in addition to "provided" - Explicitly forbid offering to search, look up, or provide more information later (the system has no such capability) - Instruct to state incomplete knowledge plainly without hinting that more information exists elsewhere Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous structured prompt was too verbose for gpt-5-mini, which ignored the blacklist and used "brani forniti" anyway. The word "passage" in the prompt itself was being translated and echoed back. Rewritten as a compact prompt with numbered critical rules. Removed all words the model could recycle (passage, document, source). Added "brani" to the forbidden list since gpt-5-mini translates "passage" to this Italian word. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewritten based on research findings (see vektra-internal
20260324-rag-prompt-best-practices-research.md):
- 4 rules only (within compliance threshold for smaller models)
- Positive framing ("Sound like you simply know") instead of blacklists
- Reintroduce <source> clarification to prevent BUG-012 regression
- Add language-matching instruction (rule 4)
- Remove forbidden phrase lists (brittle, secondary defense)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
VEKTRA_LLM_API_KEY existed in config but was never passed to litellm, forcing users to set provider-specific env vars (OPENAI_API_KEY) even for OpenAI-compatible endpoints like vLLM. Changes: - LitellmProvider now passes config.api_key to litellm as api_key kwarg - Add VEKTRA_LLM_EXTRA_BODY config field (JSON dict) for provider- specific params (e.g., chat_template_kwargs to disable thinking on Qwen3.5/DeepSeek-R1 via vLLM) - Update configuration.md with vLLM setup instructions and correct VEKTRA_LLM_API_KEY documentation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move structlog import to top level (was inline in except block) - Extract _LEARN_SENTINEL_KEY_ID as module-level constant - Fix import ordering (ruff I001) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…l quality BUG-015: reranker scores discarded (threshold applied to wrong scores) BUG-016: English-only reranker on Italian content TECH-002: RAG evaluation harness (50-question dataset, RAGAS) DEBT-010: recalibrate relevance threshold with empirical data All traced to analysis in vektra-internal/stack/20260324-reranker-threshold-gap-analysis.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ckfile Pin litellm <1.82.7 (versions 1.82.7-1.82.8 contain credential stealer). Replace direct uv pip install of qdrant-client/fastembed with lockfile-based --extra sparse --extra qdrant. Drop unused torchvision. All dependencies now verified against lockfile hashes via uv sync --frozen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
RerankerService.rerank() was discarding reranker scores and returning original cosine similarities, making the relevance threshold filter operate on the wrong values. Now reranker scores replace SearchResult.score with sigmoid normalization for cross-encoder logits. Original score preserved in SearchResult.original_score for debugging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…016, DEBT-010) Default reranker changed from flashrank/ms-marco-MiniLM-L-12-v2 (English-only) to cross-encoder/BAAI/bge-reranker-v2-m3 (multilingual). Threshold lowered from 0.3 to 0.15 as interim safety net - top-k is the primary chunk count control. Existing flashrank config remains supported for English-only lightweight deployments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add eval scripts that call the Vektra API and compute retrieval quality metrics (hit rate, MRR, precision@k) and e2e metrics (answer rate, latency percentiles). Includes seed dataset with 5 questions, Makefile targets (eval-retrieval, eval-e2e), and JSONL results output. Full 50-question dataset to be curated on Kalypso with live content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion The /api/v1/search endpoint returns raw vector scores without reranker. Add --use-query to call /api/v1/query instead, evaluating retrieval quality after reranking. Handles field name differences between the two endpoints (text_snippet vs snippet). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion Dataset covers Italian Constitution and UDHR excerpts: - 20 factual (15 IT + 5 EN) - 15 reasoning (10 IT + 5 EN) - 10 multi-chunk (cross-document) - 10 adversarial (unanswerable from content) Baseline with bge-reranker-v2-m3 at threshold 0.15: factual 90%, reasoning 80%, multi-chunk 10%, adversarial 0% Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eted BUG-017: context window fallback silently truncates prompts for vLLM models not in litellm registry. Discovered via conversation analysis - budget allocator fits only 2/5 chunks when using 4096 default instead of actual 32768. Fix: VEKTRA_LLM_CONTEXT_WINDOW env var + warn on fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ty gaps Four gaps found during post-hoc conversation analysis: no API to read turns (encrypted, DB-only), query traces not persisted for streaming, response_id not populated in conversation_turns, no admin trace lookup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document key pitfalls: encrypted conversation turns, namespace_id column naming, Qdrant as vector store (not pgvector), and the fundamental difference between /search (raw vector) and /query (full pipeline with reranker/threshold/LLM). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
litellm.get_max_tokens() silently falls back to 4096 for models not in its registry (all local vLLM models), causing the token budget allocator to discard most retrieved chunks. Add VEKTRA_LLM_CONTEXT_WINDOW env var as explicit override, and emit structlog warnings (once per model) when context window or token count fall back to defaults. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fvadicamo
added
bug
github-actions
Bot
added
component:shared
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughComprehensive updates introducing evaluation harnesses for retrieval and end-to-end RAG, enhancing LLM and reranker configuration, propagating reranker scores with original score preservation, improving conversation persistence with ensure semantics, updating provider defaults, refactoring Docker dependencies, and expanding documentation with debugging guidance. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as HTTP Client
participant API as /api/v1/query
participant Store as PersistentConversationStore
participant DB as PostgreSQL
User->>Client: POST query with conversation_id (optional)
Client->>API: Request with body.conversation_id
API->>Store: Get registry & check if store exists
alt conversation_id is None
API->>Store: create_conversation(namespace_id, key_id)
Store->>DB: INSERT INTO conversations
DB-->>Store: Return generated UUID
Store-->>API: Return new conversation_id
else conversation_id provided
API->>Store: ensure_conversation(conversation_id, namespace_id, key_id)
Store->>DB: INSERT ... ON CONFLICT DO NOTHING
DB-->>Store: Confirm (new or existing row)
Store-->>API: Return (void)
end
API->>API: Build QueryRequest with resolved conversation_id
API-->>Client: Continue RAG pipeline processing
sequenceDiagram
participant Script as eval_retrieval.py
participant Client as httpx.Client
participant Search as /api/v1/search or /api/v1/query
participant Reranker as RerankerService
Script->>Client: Load dataset & initialize client
loop For each question in dataset
Client->>Search: POST with question + expected_keywords
Search->>Reranker: Retrieve & rerank results
Reranker-->>Search: Return ranked results with scores
Search-->>Client: HTTP response with results/sources
Client->>Client: Extract retrieved items & compute metrics
Client->>Client: Check keyword match, compute MRR & precision@k
end
Script->>Script: Aggregate: hit_rate, mean_MRR, mean_precision, score percentiles
Script->>Script: Print summary with optional category/language breakdowns
Script->>Script: Write JSONL output with per-question metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the core retrieval-augmented generation (RAG) pipeline by improving the accuracy of content retrieval and the overall observability of the system. It addresses several critical issues related to reranker score handling, multilingual support, and LLM context window management, leading to more reliable and performant query responses. Additionally, it introduces robust evaluation tools and refines system configurations and documentation to streamline development and debugging. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the RAG pipeline by improving conversation persistence, reranking, and evaluation capabilities. Key changes include implementing conversation row creation and ensuring existence in the API layer for multi-turn queries (BUG-014), propagating and normalizing reranker scores (BUG-015), and updating the default reranker model to a multilingual one (BAAI/bge-reranker-v2-m3) for better performance on diverse content (BUG-016). It also addresses the silent truncation of prompts by adding VEKTRA_LLM_CONTEXT_WINDOW and logging warnings for context window and token counting fallbacks (BUG-017). Furthermore, a RAG evaluation harness (TECH-002) has been added with new Makefile targets and scripts for both retrieval-only and end-to-end evaluations, alongside recalibrating the VEKTRA_MIN_RELEVANCE_SCORE to 0.15 (DEBT-010). Documentation for configuration and internal development has been updated, and Dockerfile dependencies simplified. Review feedback highlights the need to ensure thread-safety for the new warning mechanisms in _count_tokens_impl and _context_window_impl to prevent race conditions when logging fallback warnings, and to clarify the bash code block syntax in the configuration documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vektra-shared/src/vektra_shared/config.py (1)
36-55:⚠️ Potential issue | 🟠 Major
VEKTRA_LLM_CONTEXT_WINDOWis still dropped byas_llm_config().
LLMConfigcan parse the new env var, butVektraSettingsnever stores it andas_llm_config()never forwards it. In the normal settings path, Line 539 passesVEKTRA_LLM_EXTRA_BODYwhileVEKTRA_LLM_CONTEXT_WINDOWdisappears, so the new vLLM override still falls back to 4096. Please also validate Line 51 withge=1so bad values fail fast instead of corrupting token budgeting.🛠️ Suggested fix
class LLMConfig(BaseSettings): @@ context_window: int | None = Field( None, alias="VEKTRA_LLM_CONTEXT_WINDOW", + ge=1, description="Context window size in tokens. Required for models not in litellm's registry (e.g. local vLLM). If unset, litellm lookup is attempted with a 4096-token fallback.", ) @@ llm_api_key: str | None = Field(None, alias="VEKTRA_LLM_API_KEY") llm_api_base: str | None = Field(None, alias="VEKTRA_LLM_API_BASE") llm_extra_body: dict[str, Any] | None = Field(None, alias="VEKTRA_LLM_EXTRA_BODY") + llm_context_window: int | None = Field(None, alias="VEKTRA_LLM_CONTEXT_WINDOW") llm_fallback_model: str | None = Field(None, alias="VEKTRA_LLM_FALLBACK_MODEL") @@ "VEKTRA_LLM_API_KEY": self.llm_api_key, "VEKTRA_LLM_API_BASE": self.llm_api_base, "VEKTRA_LLM_EXTRA_BODY": self.llm_extra_body, + "VEKTRA_LLM_CONTEXT_WINDOW": self.llm_context_window, "VEKTRA_LLM_FALLBACK_MODEL": self.llm_fallback_model, "VEKTRA_LLM_FALLBACK_TIMEOUT_MS": self.llm_fallback_timeout_ms, "VEKTRA_LLM_CONTEXT_ONLY_ENABLED": self.llm_context_only_enabled,Also applies to: 430-430, 532-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-shared/src/vektra_shared/config.py` around lines 36 - 55, VektraSettings currently defines context_window but never stores/forwards it to LLMConfig in as_llm_config(), so VEKTRA_LLM_CONTEXT_WINDOW is dropped; update VektraSettings to include context_window (matching the Field name used in the diff) and modify as_llm_config() to pass context_window through alongside extra_body and fallback_model into the returned LLMConfig, and add validation to the Field declaration (context_window: int | None = Field(..., ge=1)) so invalid/non-positive values fail fast.docs/reference/configuration.md (1)
25-32:⚠️ Potential issue | 🟡 MinorAdd
VEKTRA_LLM_EXTRA_BODYto the main LLM reference table.The local vLLM section introduces this knob, but the primary config matrix still skips it, so operators scanning the reference table will miss the new setting.
As per coding guidelines, "use VEKTRA_LLM_EXTRA_BODY to disable “thinking mode” when needed".📘 Suggested doc update
| `VEKTRA_LLM_PROVIDER` | str | (required) | LLM model in litellm format | | `VEKTRA_LLM_API_KEY` | str | - | API key for the LLM provider. Not needed for Ollama. | | `VEKTRA_LLM_API_BASE` | str | - | Custom API base URL for OpenAI-compatible providers (e.g., vLLM, LMStudio) | +| `VEKTRA_LLM_EXTRA_BODY` | json | - | Extra JSON body passed to litellm (for example `{"chat_template_kwargs": {"enable_thinking": false}}` for vLLM) | | `VEKTRA_LLM_FALLBACK_MODEL` | str | - | Fallback model when the primary times out | | `VEKTRA_LLM_FALLBACK_TIMEOUT_MS` | int | `60000` | Milliseconds before switching to fallback model | | `VEKTRA_LLM_CONTEXT_WINDOW` | int | - | Context window size in tokens. Required for models not in litellm's registry (e.g. local vLLM). If unset, litellm lookup with 4096 fallback (warns on fallback) |Also applies to: 42-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/configuration.md` around lines 25 - 32, Add a new row for VEKTRA_LLM_EXTRA_BODY to the main LLM configuration table so operators see the knob in the primary matrix; update the table that lists environment variables (the one containing VEKTRA_LLM_PROVIDER, VEKTRA_LLM_API_KEY, etc.) to include a row for `VEKTRA_LLM_EXTRA_BODY` (type: str, default: - or provide recommended default), with a short description like "Extra JSON body appended to LLM requests; used to disable 'thinking mode' per coding guidelines"; also mirror this addition in the related LLM config section referenced later (the local vLLM area) so both tables are consistent.
🧹 Nitpick comments (2)
.claude/CLAUDE.md (1)
20-20: Avoid hardcoded Qdrant connection details in docs.Hardcoding
port 10633and collectionvektracan go stale across environments. Prefer referencing the configured env vars/settings (or saying “check runtime config”) to keep this accurate.Suggested doc tweak
-- **Postgres holds metadata/text, Qdrant holds vectors**: chunk text is in `document_chunks` (Postgres), but vector search runs against Qdrant (port 10633, collection `vektra`). To inspect vectors or search results, query Qdrant REST API directly. The Qdrant payload uses `namespace_id` as the namespace filter field. +- **Postgres holds metadata/text, Qdrant holds vectors**: chunk text is in `document_chunks` (Postgres), but vector search runs against Qdrant (use the configured host/port and collection from environment/runtime config). To inspect vectors or search results, query Qdrant REST API directly. The Qdrant payload uses `namespace_id` as the namespace filter field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/CLAUDE.md at line 20, The docs currently hardcode Qdrant connection details ("port 10633" and collection `vektra`) which may vary by environment; update the text that references Qdrant (the sentence mentioning Qdrant port 10633, collection `vektra`, and the use of `namespace_id` and `document_chunks`) to instead point readers to the runtime/config settings or environment variables used by the app (e.g., reference the configured Qdrant host/port and collection name in config/env) and suggest checking runtime config to determine the actual collection/port for their environment.Dockerfile (1)
73-76: Minor: Redundantuv syncwhenINSTALL_UNSTRUCTURED=true.When
INSTALL_UNSTRUCTURED=true, this runsuv synctwice—once without--extra ocrand once with it. You could restructure to avoid the duplicate work:♻️ Suggested optimization
-RUN uv sync --frozen --no-editable --no-dev --extra sparse --extra qdrant \ - && if [ "$INSTALL_UNSTRUCTURED" = "true" ]; then \ - uv sync --frozen --no-editable --no-dev --extra sparse --extra qdrant --extra ocr; \ - fi +RUN if [ "$INSTALL_UNSTRUCTURED" = "true" ]; then \ + uv sync --frozen --no-editable --no-dev --extra sparse --extra qdrant --extra ocr; \ + else \ + uv sync --frozen --no-editable --no-dev --extra sparse --extra qdrant; \ + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 73 - 76, The Dockerfile currently runs `uv sync` twice (first unconditionally, then again with `--extra ocr` when `INSTALL_UNSTRUCTURED=true`), causing redundant work; change the logic to compute the extras/options before calling `uv sync` and invoke `uv sync` only once: build an extras/options variable that always includes `--extra sparse --extra qdrant` and conditionally appends `--extra ocr` when `INSTALL_UNSTRUCTURED` is "true", then call `uv sync --frozen --no-editable --no-dev` with that single assembled extras string so `uv sync` runs only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Line 35: The sentence is ambiguous about “retrieval quality” vs “end-to-end
answer quality”; update the text to explicitly state that `/api/v1/search`
should be used to evaluate raw retrieval quality (vector similarity only) and
`/api/v1/query` should be used to evaluate full pipeline or end-to-end answer
quality (retrieval plus reranking, prompt/LLM behavior, and post-processing), so
replace or augment the current line with a clarifying phrase that contrasts
`/api/v1/search` (raw retrieval) and `/api/v1/query` (full pipeline behavior).
In `@scripts/eval_e2e.py`:
- Around line 67-89: The except block currently marks transport/API failures as
no_relevant_context=True and omits duration_ms; change the except handler for
the client.post block to compute duration_ms = int((time.monotonic() - t0) *
1000) and pass that into the returned E2EResult, and do NOT set
no_relevant_context=True (set it to False or leave as failure-specific value) so
failed requests are not treated as "no relevant context" answers; update the
E2EResult construction (referencing t0 and E2EResult) to include duration_ms and
the error string while keeping num_sources as appropriate.
In `@scripts/eval_retrieval.py`:
- Around line 29-41: The current aggregation/print_summary logic averages in
QuestionResult rows that have no ground-truth (keyword-less), which biases
metrics; update the aggregation in print_summary() (and any summary/total
computation paths that consume QuestionResult) to first filter out results where
QuestionResult.num_relevant == 0 or QuestionResult.error indicates "no ground
truth" (i.e., the branch that marked entries as having no ground truth), compute
hit/mrr/precision_at_k and breakdowns only over the filtered list, adjust totals
and per-category counts to reflect the number of scored items, and include a
separate printed count of excluded/unscored rows so users know how many were
omitted.
In `@vektra-core/src/vektra_core/api.py`:
- Around line 211-239: The generated server-side conversation_id created in the
conversation store block (conversation_id, conv_store.create_conversation /
conv_store.ensure_conversation) must be threaded through the SSE path so clients
receive it; update the call to _sse_generator(...) to accept an additional
conversation_id argument (or context object) and modify _sse_generator to
include that conversation_id in the final "done" SSE event payload, ensuring the
same conversation_id is passed into the QueryRequest and into the SSE generator
so streamed first turns return the newly created ID.
In `@vektra-core/src/vektra_core/reranker.py`:
- Around line 70-80: The normalization decision should be based on the score
provider semantics, not the observed numeric range: replace the heuristic that
sets needs_sigmoid by value range with a provider-based check (e.g., inspect the
ranking metadata or a provider/type field on ranked or each SearchResult) and
consistently apply _sigmoid when the provider is a cross-encoder (or other
unnormalized scorer); update the logic around needs_sigmoid,
top_items/raw_scores and the normalization step (where normalized =
_sigmoid(raw_score) if needs_sigmoid else raw_score) so
_apply_retrieval_filter() always compares thresholds against the correctly
normalized scores for that provider. Ensure you reference the provider/type
field available on ranked or SearchResult to decide normalization rather than
using any s < 0.0 or s > 1.0 checks.
In `@vektra-core/tests/test_pipeline.py`:
- Around line 494-528: Update the two "warns_once" tests to assert that the
first invocation actually emits the warning and the second does not: for
test_context_window_fallback_warns_once, call
_context_window_impl("test/warn-once-model"), capture output with
capsys.readouterr() and assert that captured.out contains
"context_window_fallback", then call _context_window_impl again and assert the
subsequent captured.out does not contain it; similarly for
test_count_tokens_fallback_warns_once, call _count_tokens_impl(mock_llm,
"test/token-once-model", "first"), assert the first captured output contains
"token_count_fallback", then call it a second time and assert the second
captured output does not contain the token_count_fallback string; keep the
existing set discard/reset logic (_context_window_fallback_warned and
_token_count_fallback_warned) and use capsys.readouterr() to separate first and
second captures.
---
Outside diff comments:
In `@docs/reference/configuration.md`:
- Around line 25-32: Add a new row for VEKTRA_LLM_EXTRA_BODY to the main LLM
configuration table so operators see the knob in the primary matrix; update the
table that lists environment variables (the one containing VEKTRA_LLM_PROVIDER,
VEKTRA_LLM_API_KEY, etc.) to include a row for `VEKTRA_LLM_EXTRA_BODY` (type:
str, default: - or provide recommended default), with a short description like
"Extra JSON body appended to LLM requests; used to disable 'thinking mode' per
coding guidelines"; also mirror this addition in the related LLM config section
referenced later (the local vLLM area) so both tables are consistent.
In `@vektra-shared/src/vektra_shared/config.py`:
- Around line 36-55: VektraSettings currently defines context_window but never
stores/forwards it to LLMConfig in as_llm_config(), so VEKTRA_LLM_CONTEXT_WINDOW
is dropped; update VektraSettings to include context_window (matching the Field
name used in the diff) and modify as_llm_config() to pass context_window through
alongside extra_body and fallback_model into the returned LLMConfig, and add
validation to the Field declaration (context_window: int | None = Field(...,
ge=1)) so invalid/non-positive values fail fast.
---
Nitpick comments:
In @.claude/CLAUDE.md:
- Line 20: The docs currently hardcode Qdrant connection details ("port 10633"
and collection `vektra`) which may vary by environment; update the text that
references Qdrant (the sentence mentioning Qdrant port 10633, collection
`vektra`, and the use of `namespace_id` and `document_chunks`) to instead point
readers to the runtime/config settings or environment variables used by the app
(e.g., reference the configured Qdrant host/port and collection name in
config/env) and suggest checking runtime config to determine the actual
collection/port for their environment.
In `@Dockerfile`:
- Around line 73-76: The Dockerfile currently runs `uv sync` twice (first
unconditionally, then again with `--extra ocr` when
`INSTALL_UNSTRUCTURED=true`), causing redundant work; change the logic to
compute the extras/options before calling `uv sync` and invoke `uv sync` only
once: build an extras/options variable that always includes `--extra sparse
--extra qdrant` and conditionally appends `--extra ocr` when
`INSTALL_UNSTRUCTURED` is "true", then call `uv sync --frozen --no-editable
--no-dev` with that single assembled extras string so `uv sync` runs only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac00851e-5ec3-498d-8746-3431c6f02bfb
⛔ Files ignored due to path filters (2)
.s2s/BACKLOG.mdis excluded by!.s2s/**uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (24)
.claude/CLAUDE.md.env.example.gitignoreDockerfileMakefiledocs/reference/configuration.mdscripts/eval_e2e.pyscripts/eval_retrieval.pytests/eval/dataset.jsonlvektra-app/src/vektra_app/main.pyvektra-core/pyproject.tomlvektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/src/vektra_core/api.pyvektra-core/src/vektra_core/conversation.pyvektra-core/src/vektra_core/pipeline.pyvektra-core/src/vektra_core/providers/litellm_provider.pyvektra-core/src/vektra_core/reranker.pyvektra-core/src/vektra_core/templates/system.j2vektra-core/tests/test_pipeline.pyvektra-core/tests/test_reranker.pyvektra-learn/src/vektra_learn/api.pyvektra-shared/src/vektra_shared/config.pyvektra-shared/src/vektra_shared/types.pyvektra-shared/tests/test_config.py
context_window was defined in LLMConfig but not stored in VektraSettings or forwarded by as_llm_config(). Worked by accident via pydantic-settings env fallback, but broke when passing values programmatically. Add llm_context_window field and forward it explicitly. Also add ge=1 validation to reject invalid values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adversarial questions (no expected_keywords) were averaged into hit rate, MRR, and precision, biasing all metrics downward. Add has_ground_truth flag and compute metrics only over scored entries. Unscored count shown separately in summary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests only verified the second call was silent but not that the first call actually emitted the warning. If logging broke entirely, both tests would still pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
API/network errors were incorrectly classified as no_relevant_context, inflating the no-context count and omitting duration_ms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Missing from the primary config matrix, only documented in the local vLLM section. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reference config env vars instead of hardcoding port 10633 and collection name. Clarify wording for when to use each endpoint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Was running uv sync twice: once without --extra ocr, then again with it. Restructured to run once with the correct extras. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Fixed in f31b588. Added llm_context_window to VektraSettings and forwarded via as_llm_config(). Added ge=1 validation. Addresses outside-diff comment on vektra-shared/src/vektra_shared/config.py:36-55. |
Contributor
Author
|
Fixed in fe910a4. Added VEKTRA_LLM_EXTRA_BODY row to the main LLM config table. Addresses outside-diff comment on docs/reference/configuration.md:25-32. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/reference/configuration.md (1)
45-51: Consider adding shell quoting guidance for JSON values.The
VEKTRA_LLM_EXTRA_BODYexample shows raw JSON. In shell environments, this requires quoting (e.g.,VEKTRA_LLM_EXTRA_BODY='{"chat_template_kwargs": {"enable_thinking": false}}'), while Docker Compose.envfiles may handle it differently. A brief note on quoting could prevent user confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/configuration.md` around lines 45 - 51, Update the VEKTRA_LLM_EXTRA_BODY example to warn about shell quoting: explain that JSON must be quoted in shell environments (e.g., use single quotes around the JSON string) and note that Docker Compose .env files may handle quoting differently; reference the VEKTRA_LLM_EXTRA_BODY variable name and the example block so readers know to use something like VEKTRA_LLM_EXTRA_BODY='{"chat_template_kwargs": {"enable_thinking": false}}' in shells while calling out Docker Compose .env behavior.scripts/eval_retrieval.py (2)
107-119: Consider settinghas_ground_truth=Falsefor error results.Error results default to
has_ground_truth=True, but they're filtered out byr.error is Nonebefore the scored/unscored split. While this works correctly, explicitly settinghas_ground_truth=Falsewould make the intent clearer and prevent accidental inclusion if filtering logic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eval_retrieval.py` around lines 107 - 119, The except block returning a QuestionResult currently omits has_ground_truth; update the error return in the except to include has_ground_truth=False so that the error result explicitly marks no ground truth (modify the QuestionResult construction inside the except in scripts/eval_retrieval.py where QuestionResult(...) is returned for exceptions, adding has_ground_truth=False alongside the existing fields like id=question_id, question=question, category=category, language=language, error=str(e)).
271-272: Inconsistent JSON encoding between eval scripts.
eval_e2e.pyusesjson.dumps(row, ensure_ascii=False)(line 171) for proper Unicode output, but this script usesjson.dumps(row)which escapes non-ASCII characters. For a bilingual evaluation dataset, consider usingensure_ascii=Falsehere as well for consistent and readable JSONL output.♻️ Suggested fix
- f.write(json.dumps(row) + "\n") + f.write(json.dumps(row, ensure_ascii=False) + "\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eval_retrieval.py` around lines 271 - 272, The JSONL output here uses json.dumps(row) which escapes non-ASCII characters; update the call to json.dumps(row, ensure_ascii=False) where the row is written (the json.dumps invocation used with f.write) so Unicode is preserved and matches the behavior in eval_e2e.py (keeping the rest of the row population like row["error"] unchanged).scripts/eval_e2e.py (1)
122-124: Minor: percentile index calculation could be more precise.The p95 index calculation
int(len(durations) * 0.95)truncates rather than rounding, which may select a slightly lower percentile for small datasets. For an evaluation harness this is acceptable, butmin(int(len(durations) * 0.95), len(durations) - 1)would be safer to prevent potential index-out-of-bounds on edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eval_e2e.py` around lines 122 - 124, The p95 index calculation can be out-of-bounds for small datasets; update the p95 computation that uses durations and p95 to clamp the index: compute idx = min(int(len(durations) * 0.95), len(durations) - 1) (or use round if preferred) and then set p95 = durations[idx]; ensure you apply this guard where durations is built from valid (the sorted(r.duration_ms for r in valid)) so an empty/small durations list cannot cause an IndexError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/configuration.md`:
- Around line 45-51: Update the VEKTRA_LLM_EXTRA_BODY example to warn about
shell quoting: explain that JSON must be quoted in shell environments (e.g., use
single quotes around the JSON string) and note that Docker Compose .env files
may handle quoting differently; reference the VEKTRA_LLM_EXTRA_BODY variable
name and the example block so readers know to use something like
VEKTRA_LLM_EXTRA_BODY='{"chat_template_kwargs": {"enable_thinking": false}}' in
shells while calling out Docker Compose .env behavior.
In `@scripts/eval_e2e.py`:
- Around line 122-124: The p95 index calculation can be out-of-bounds for small
datasets; update the p95 computation that uses durations and p95 to clamp the
index: compute idx = min(int(len(durations) * 0.95), len(durations) - 1) (or use
round if preferred) and then set p95 = durations[idx]; ensure you apply this
guard where durations is built from valid (the sorted(r.duration_ms for r in
valid)) so an empty/small durations list cannot cause an IndexError.
In `@scripts/eval_retrieval.py`:
- Around line 107-119: The except block returning a QuestionResult currently
omits has_ground_truth; update the error return in the except to include
has_ground_truth=False so that the error result explicitly marks no ground truth
(modify the QuestionResult construction inside the except in
scripts/eval_retrieval.py where QuestionResult(...) is returned for exceptions,
adding has_ground_truth=False alongside the existing fields like id=question_id,
question=question, category=category, language=language, error=str(e)).
- Around line 271-272: The JSONL output here uses json.dumps(row) which escapes
non-ASCII characters; update the call to json.dumps(row, ensure_ascii=False)
where the row is written (the json.dumps invocation used with f.write) so
Unicode is preserved and matches the behavior in eval_e2e.py (keeping the rest
of the row population like row["error"] unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c9950f8-b106-4293-a829-400d5043472d
📒 Files selected for processing (7)
.claude/CLAUDE.mdDockerfiledocs/reference/configuration.mdscripts/eval_e2e.pyscripts/eval_retrieval.pyvektra-core/tests/test_pipeline.pyvektra-shared/src/vektra_shared/config.py
✅ Files skipped from review due to trivial changes (1)
- .claude/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Dockerfile
- vektra-core/tests/test_pipeline.py
- vektra-shared/src/vektra_shared/config.py
Streaming path does not emit server-generated conversation_id in any SSE event. Direct API consumers using SSE without pre-generating an ID cannot discover which conversation to continue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cross-encoder reranker (bge-m3) takes ~4s per query vs ~50ms for flashrank. 100 queries caused the integration job to exceed its 20-minute timeout. 20 queries is sufficient for p50/p95 on a non-blocking warn-only measurement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Query latency measurement (100 queries with cross-encoder reranker) takes ~7 minutes and is warn-only. No reason to run on every PR push. Now runs only on push to develop/main (post-merge). Restored 100 queries and raised job timeout to 30 minutes to accommodate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add shell quoting note for VEKTRA_LLM_EXTRA_BODY JSON values - Set has_ground_truth=False on error results (defensive) - Use ensure_ascii=False in eval_retrieval JSONL output (matches eval_e2e) - Add VEKTRA_LLM_CONTEXT_WINDOW to vLLM example block Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed 3 of 4 nitpicks from review round 2 in 4f2fa6a:
Skipped #4 (p95 index clamp): |
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.
Summary
making the relevance threshold operate on wrong values (BUG-015)
models not in litellm registry (BUG-017)
dataset (TECH-002)
Test plan
make lintpassesmake testpasses (587 passed, 2 pre-existing env-leak failures)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration Changes
Documentation
Bug Fixes