feat(evaluation): admin System tab for indexing speed and RAG quality - #811
Draft
EnjoyBacon7 wants to merge 14 commits into
Draft
feat(evaluation): admin System tab for indexing speed and RAG quality#811EnjoyBacon7 wants to merge 14 commits into
EnjoyBacon7 wants to merge 14 commits into
Conversation
Adds an on-demand evaluation harness driven from the admin System page. A run indexes an admin-uploaded corpus into a throwaway partition, replays a CSV test set through promptfoo, and reports three metric families: indexing throughput, retrieval ranking quality, and answer quality. Backend - EvalRunner Ray actor (detached, one run at a time) drives OpenRAG over its own HTTP API, so the timings measure the path real uploads take. - Two promptfoo configs rather than one: retrieval targets /search (whose documents carry chunk text and metadata.file_id), answers target /v1/chat/completions. Each config has a single provider, so no assertion runs against an output shape it cannot read. - Hit rate / MRR / recall follow the definitions already documented in tests/load/automatic-evaluation-pipeline. Rows lacking expected_file_ids are reported as skipped, never scored as misses. - Model-graded assertions use OpenRAG's own configured LLM, so an eval needs no third-party credentials. - Runs authenticate as the non-admin service user __openrag_eval__ whose token is regenerated per run, so no usable plaintext token is at rest. - __eval_* partitions are filtered out of the partition listing. UI - Evaluation tab on the System page: dataset upload, run history, and a run detail view with the three metric panels and per-question results. Infra - Node + pinned promptfoo in ray.Dockerfile; OPENRAG_INTERNAL_URL and PROMPTFOO_BIN documented in env_vars.md. Tests: 37 backend unit tests (CSV parsing, config generation, metric math) and 9 UI tests.
📝 WalkthroughWalkthroughAdds an admin evaluation system with dataset uploads, persisted run history, Ray-based indexing and Promptfoo benchmarking, metric aggregation, API endpoints, and a System-page UI for launching, monitoring, canceling, and inspecting runs. ChangesEvaluation workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
added
admin-ui
The compose stack runs Ray inside the openrag container, which is built from api.Dockerfile — ray.Dockerfile is only used by deployments with a separate Ray cluster. Without this, EvalRunner would fail every run with 'promptfoo executable not found' on the standard docker compose install. Found while deploying the branch to a test host.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
ui/src/pages/admin/evaluation/index.test.tsx (2)
148-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFirst
renderTab()in this test is never unmounted.The initial
renderTab()call (line 149) isn't captured/unmounted before the secondrenderTab()(line 157) mounts a second tree in the same test — twoEvaluationTabinstances coexist until the end of the test.♻️ Proposed fix
- renderTab(); + const { unmount: unmountFirst } = renderTab(); await screen.findByText("Support docs"); expect(screen.queryByRole("button", { name: /cancel run/i })).toBeNull(); + unmountFirst();🤖 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 `@ui/src/pages/admin/evaluation/index.test.tsx` around lines 148 - 162, Update the test case around renderTab so the initial rendered EvaluationTab instance is captured and unmounted before the second renderTab call. Preserve the existing assertions and active-run cancellation flow while ensuring only one mounted tree exists at a time.
1-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for dataset creation/deletion flows.
Tests cover listing, starting, cancelling runs and metric rendering well, but the "New dataset" upload dialog and delete flow (
createEvalDataset/deleteEvalDataset, mocked but never exercised) have no tests. A test reopening the upload dialog after Cancel would have caught the stale-state bug flagged indataset-card.tsx.Want me to draft tests for the upload dialog (including a reopen-after-cancel case) and the delete confirmation flow?
🤖 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 `@ui/src/pages/admin/evaluation/index.test.tsx` around lines 1 - 213, The EvaluationTab tests omit dataset creation, upload-dialog cancellation/reopen behavior, and deletion coverage. Extend the EvaluationTab test suite to exercise createEvalDataset with the New dataset dialog, verify Cancel clears the selected upload state so reopening starts clean, and cover the delete confirmation flow including deleteEvalDataset invocation; add the necessary mocked symbols and fixtures while preserving existing tests.ui/src/pages/admin/evaluation/run-detail.tsx (2)
81-125: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFailed-file samples aren't surfaced anywhere.
IndexingMetrics.samples(withfilename,duration_seconds,failed) is fetched but never rendered — only the aggregatefiles_failedcount shows. An admin debugging a bad corpus can't tell which file(s) failed.Consider rendering the failed entries from
metrics.samples(filename + reason if available) whenfiles_failed > 0, so failures are actionable rather than just a number.🤖 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 `@ui/src/pages/admin/evaluation/run-detail.tsx` around lines 81 - 125, Update IndexingPanel to render failed entries from metrics.samples when metrics.files_failed is greater than zero, showing each filename and its failure reason when available. Keep the existing aggregate statistics and only display the failure details when failed samples exist.
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated, inconsistent metric-formatting helpers across the two views.
run-detail.tsxandindex.tsxeach define their ownpercent/inline formatting for the same metrics (hit_rate, mrr), with different precision, so the identical run can display "75%" in the runs table and "75.0%" in the detail panel, and MRR astoFixed(2)vstoFixed(3).
ui/src/pages/admin/evaluation/run-detail.tsx#L11-L17: movepercent/scoreinto a shared module (e.g.ui/src/lib/format.ts) and import it here.ui/src/pages/admin/evaluation/index.tsx#L16-L18: drop the localpercentand inlinetoFixed(2)for mrr; import the same sharedpercent/scorehelpers so both views render identical precision for the same metric.🤖 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 `@ui/src/pages/admin/evaluation/run-detail.tsx` around lines 11 - 17, Move the percent and score helpers from run-detail.tsx into a shared formatting module, then import and use them in both evaluation views. In ui/src/pages/admin/evaluation/run-detail.tsx lines 11-17, remove the local definitions and import the shared helpers; in ui/src/pages/admin/evaluation/index.tsx lines 16-18, remove the local percent helper and inline MRR formatting, replacing both with the shared percent/score functions so hit_rate and mrr use identical precision.ui/src/lib/api/evaluation.ts (1)
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
by_extensionbucket shape explicitly.
Record<string, number>for each extension bucket type-checksbucket.files/bucket.mean_secondsaccess inrun-detail.tsxonly because of the loose index signature — it doesn't document the actual shape or catch typos in field names.♻️ Proposed type refinement
- by_extension: Record<string, Record<string, number>>; + by_extension: Record<string, { files: number; mean_seconds: number }>;🤖 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 `@ui/src/lib/api/evaluation.ts` around lines 42 - 53, Update the IndexingMetrics interface’s by_extension property to use an explicit bucket type defining the actual fields consumed by run-detail.tsx, including files and mean_seconds, instead of nested Record<string, number>. Preserve the existing extension-keyed structure while enabling type checking for bucket field names.
🤖 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.
Inline comments:
In `@infra/docker/ray.Dockerfile`:
- Around line 24-32: Update the Dockerfile’s Node installation in the promptfoo
setup to install a supported Node.js version, such as Node 20.20.0 or newer,
from an official or NodeSource source instead of Debian bookworm’s default
nodejs package. Preserve the pinned promptfoo version and ensure npm remains
available for the existing global installation.
In `@openrag/core/evaluation/metrics.py`:
- Around line 41-52: Update _percentile to calculate the nearest-rank index with
a ceiling-based rank rather than round, ensuring exact fractional ties select
the intended rank while retaining the existing bounds clamping and
observed-value behavior.
In `@openrag/core/evaluation/testset.py`:
- Around line 95-136: Update the CSV iteration around the cases and errors
collections to enforce limits while reading: stop processing once valid cases
exceed MAX_ROWS, and cap stored validation errors at _MAX_REPORTED_ERRORS while
still tracking the total error count needed for the summary suffix. Preserve the
existing validation messages and final ValidationError codes, but avoid
materializing unbounded cases or errors before rejection.
In `@openrag/services/orchestrators/evaluation_service.py`:
- Around line 147-204: Enforce the single-active-run invariant at both layers:
in evaluation_service.py lines 147-204, protect the active_run() check through
create_run() and dispatch with an atomic lock or database-level guard; in
eval_runner.py lines 59-60 and 87-108, reject run() when _active_run_id is
already set before overwriting state; and in eval_runner.py lines 168-169,
ensure finally cleanup only clears state belonging to the completing run so
overlapping calls cannot corrupt cancellation or process tracking.
- Around line 147-204: Serialize the active-run check and run creation in
EvaluationService.start_run so concurrent requests cannot both pass active_run()
before create_run(). Use the service’s existing locking or synchronization
mechanism to guard the check-and-create sequence, while preserving the current
conflict behavior and single-run dispatch flow.
- Around line 131-134: Update delete_dataset to detect and reject deletion when
an evaluation run is actively using the dataset, before deleting the repository
record or dataset directory. Reuse the existing run-tracking or active-run
mechanism associated with _index_corpus, and raise a clear dataset-in-use error;
retain the current NotFoundError behavior for missing datasets and only call
shutil.rmtree after the guard passes.
In `@openrag/services/orchestrators/partition_service.py`:
- Around line 245-247: Update list_partition_summaries() to filter out eval
partitions with the same is_eval_partition() predicate used by
list_partitions(), including when expanding the admin all sentinel. Ensure
orphaned __eval_<run_id> partitions are excluded from the GET /partition/
response while preserving non-eval summaries.
In `@openrag/services/persistence/evaluation_repo.py`:
- Around line 110-120: Enforce the one-active-run invariant at the database
layer rather than relying on the check-then-act flow around active_run(). Add a
partial unique index or equivalent constraint on eval_runs covering rows whose
status is QUEUED, INDEXING, or EVALUATING, and update create_run() to surface
the atomic constraint failure as the existing active-run conflict. Keep
active_run() for lookup, but do not rely on it for exclusivity.
In `@ui/src/pages/admin/evaluation/dataset-card.tsx`:
- Around line 137-158: Introduce a shared close handler near reset that resets
name, testset, and corpus before invoking onOpenChange(false). Use this handler
both in the Dialog onOpenChange wrapper for closing and in the Cancel button
instead of calling the raw onOpenChange prop, while preserving the existing open
behavior.
In `@ui/src/pages/admin/evaluation/index.tsx`:
- Around line 93-116: Update the run rows rendered in the runs.map callback to
be keyboard accessible: add an appropriate row role, make each row focusable
with tabIndex, and invoke setSelectedRunId(run.id) from an onKeyDown handler for
Enter and Space while preserving the existing onClick behavior.
---
Nitpick comments:
In `@ui/src/lib/api/evaluation.ts`:
- Around line 42-53: Update the IndexingMetrics interface’s by_extension
property to use an explicit bucket type defining the actual fields consumed by
run-detail.tsx, including files and mean_seconds, instead of nested
Record<string, number>. Preserve the existing extension-keyed structure while
enabling type checking for bucket field names.
In `@ui/src/pages/admin/evaluation/index.test.tsx`:
- Around line 148-162: Update the test case around renderTab so the initial
rendered EvaluationTab instance is captured and unmounted before the second
renderTab call. Preserve the existing assertions and active-run cancellation
flow while ensuring only one mounted tree exists at a time.
- Around line 1-213: The EvaluationTab tests omit dataset creation,
upload-dialog cancellation/reopen behavior, and deletion coverage. Extend the
EvaluationTab test suite to exercise createEvalDataset with the New dataset
dialog, verify Cancel clears the selected upload state so reopening starts
clean, and cover the delete confirmation flow including deleteEvalDataset
invocation; add the necessary mocked symbols and fixtures while preserving
existing tests.
In `@ui/src/pages/admin/evaluation/run-detail.tsx`:
- Around line 81-125: Update IndexingPanel to render failed entries from
metrics.samples when metrics.files_failed is greater than zero, showing each
filename and its failure reason when available. Keep the existing aggregate
statistics and only display the failure details when failed samples exist.
- Around line 11-17: Move the percent and score helpers from run-detail.tsx into
a shared formatting module, then import and use them in both evaluation views.
In ui/src/pages/admin/evaluation/run-detail.tsx lines 11-17, remove the local
definitions and import the shared helpers; in
ui/src/pages/admin/evaluation/index.tsx lines 16-18, remove the local percent
helper and inline MRR formatting, replacing both with the shared percent/score
functions so hit_rate and mrr use identical precision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cbd4ffa-bc56-4da0-a201-a7a86269aa65
📒 Files selected for processing (32)
CLAUDE.mddocs/content/docs/documentation/env_vars.mdinfra/docker/ray.Dockerfileopenrag/api/main.pyopenrag/api/routers/admin/evaluation.pyopenrag/api/schemas/admin/evaluation_schemas.pyopenrag/core/evaluation/__init__.pyopenrag/core/evaluation/metrics.pyopenrag/core/evaluation/promptfoo_config.pyopenrag/core/evaluation/testset.pyopenrag/core/models/evaluation.pyopenrag/core/ports/catalog_store.pyopenrag/core/ports/evaluation_repo.pyopenrag/di/container.pyopenrag/di/providers.pyopenrag/services/orchestrators/evaluation_service.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/persistence/evaluation_repo.pyopenrag/services/persistence/migrations/alembic/versions/a7c9e1f2b3d4_add_evaluation_tables.pyopenrag/services/persistence/schema.pyopenrag/services/storage/postgres_store.pyopenrag/services/workers/eval_runner.pytests/unit/core/evaluation/test_metrics.pytests/unit/core/evaluation/test_promptfoo_config.pytests/unit/core/evaluation/test_testset.pyui/src/components/shared/status-badge.tsxui/src/lib/api/evaluation.tsui/src/pages/admin/evaluation/dataset-card.tsxui/src/pages/admin/evaluation/index.test.tsxui/src/pages/admin/evaluation/index.tsxui/src/pages/admin/evaluation/run-detail.tsxui/src/pages/admin/system.tsx
coderabbitai
Bot
removed
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@infra/docker/api.Dockerfile`:
- Around line 29-31: Update the Node.js installation in both Dockerfiles so it
uses a controlled source providing Node.js 20.20+ or 22.22+, rather than relying
on the distribution apt version. In the setup that installs promptfoo, add
build-time assertions for node --version, npm --version, and promptfoo
--version, ensuring promptfoo@${PROMPTFOO_VERSION} is available before the image
build completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e20f881-14c0-4973-9d7a-028a6be608df
📒 Files selected for processing (2)
CLAUDE.mdinfra/docker/api.Dockerfile
expected_file_ids in the test-set CSV is matched against metadata.file_id,
but the runner uploaded each file as 'eval-{index}-{name}'. No CSV author
can predict that prefix, so every ranking metric scored zero. The file_id
is now the filename, percent-encoded for the request path since corpus
filenames routinely contain spaces and accents.
Found while building a default dataset from a real corpus.
… dead actor Two defects found by running the feature on a real deployment: 1. EvalRunner built ConnectionManager from settings.rdb, whose database is None unless derived from the Milvus collection name. The actor died in __init__ on every run. The derivation moves to Settings.resolved_rdb() so the API's catalog store and any Ray worker resolve it identically — services cannot import di, where the logic previously lived. 2. Dispatch is fire-and-forget, so the dead actor left the run in QUEUED forever with no error, plus an orphaned partition and token. start_run now pings the runner before provisioning anything and returns 503 when it cannot be reached.
… Node 22 Three defects from the first end-to-end run on a real corpus: - The indexing API rejects a file_id containing spaces, so 8 of 24 corpus files failed to upload. The file_id is now sanitized to the allowed character set. - That sanitization would have broken expected_file_ids matching, so the ranking metrics now match a test set's ground truth against the chunk's metadata.source filename as well as its file_id. Authors keep writing real filenames. - Debian's nodejs package is 20.19.x, below promptfoo's floor of ^20.20.0 || >=22.22.0, so 'promptfoo eval' refused to start. Both images now install Node 22 from NodeSource.
11 questions over 6 documents, answers verified against the source PDFs, plus 18 topically-adjacent distractors so retrieval has to discriminate — a corpus with one document per topic flatters any retriever. The PDFs are third-party and stay out of the repo; corpus.txt lists them so the dataset can be reassembled. Lives under tests/evaluation/ rather than tests/data/ because .gitignore's 'data/' rule silently swallows the latter.
.gitignore's blanket '*.csv' silently dropped the sample test set from the previous commit, leaving its README pointing at a missing file.
promptfoo keeps a SQLite eval history under $HOME/.promptfoo. In the API image that path is not writable by the non-root app user, so the database migration failed and the CLI exited 1 with *no output on either stream* — even 'promptfoo --version' failed. Each run now points PROMPTFOO_CONFIG_DIR at its own temp directory. The failure was invisible because the error path only captured stderr; it now reports stdout too, where promptfoo prints config errors. Also dumps YAML with allow_unicode so accented questions stay readable.
coderabbitai
Bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/unit/services/orchestrators/test_evaluation_service.py`:
- Around line 116-119: Run Ruff formatting on
tests/unit/services/orchestrators/test_evaluation_service.py and apply all
resulting formatting changes, including the settings model_copy block, so ruff
format --check passes without altering test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32c17c78-7012-4a47-9ea2-9295c859faac
⛔ Files ignored due to path filters (1)
tests/evaluation/rag_dataset_sample.csvis excluded by!**/*.csv
📒 Files selected for processing (13)
.gitignoreinfra/docker/api.Dockerfileinfra/docker/ray.Dockerfileopenrag/core/config/root.pyopenrag/core/evaluation/metrics.pyopenrag/di/repositories.pyopenrag/services/orchestrators/evaluation_service.pyopenrag/services/workers/eval_runner.pytests/evaluation/README.mdtests/evaluation/corpus.txttests/unit/core/config/test_resolved_rdb.pytests/unit/core/evaluation/test_metrics.pytests/unit/services/orchestrators/test_evaluation_service.py
🚧 Files skipped from review as they are similar to previous changes (6)
- infra/docker/api.Dockerfile
- infra/docker/ray.Dockerfile
- openrag/core/evaluation/metrics.py
- tests/unit/core/evaluation/test_metrics.py
- openrag/services/workers/eval_runner.py
- openrag/services/orchestrators/evaluation_service.py
…ize ids The first successful end-to-end run scored 0 on every answer and missed every document whose filename contains a space. Two causes: - promptfoo evaluates transformResponse as a single JavaScript expression. The IIFE that parsed 'extra' for sources failed with 'is not a function', so every answer row errored before grading and factuality/llm-rubric produced no scores at all. The answer pass only needs the message text, so the transform is now json.choices[0].message.content. Verified in the container: both graders return real scores and reasoning. - Ground-truth matching relied on metadata.source holding the original filename; it holds the sanitized one. Both sides of the comparison now run through the same sanitizer (core.evaluation.identity), so a test set naming 'A B.pdf' matches the stored 'A_B.pdf' either way.
… path
metadata.source is the server's temp path
('/app/data/1785151308957_c270_report.pdf'), so the per-question table was
showing that instead of the document's name. Verified against a live
search response.
Corrects a test that encoded the earlier wrong assumption that source
holds the original filename.
Rebased onto the latest branch. The upstream commits independently fixed the file_id rejection, the promptfoo transform and orphaned-run reaping — their versions are kept, since they were verified against a live run. This carries the findings they did not cover. Correctness: - p50/p95 used round(), which breaks ties to even and returned the wrong observation whenever fraction * n was an odd integer (p50 of two files reported the slower one). Now ceil, per the nearest-rank definition. - start_run checked for an active run, then regenerated the shared eval user's token. Two concurrent starts could both pass that check, and the loser revoked the credentials the winner was still indexing with. The run row is now created before anything is provisioned, and the partial unique index ux_eval_runs_single_active makes that insert the lock. This composes with the pre-dispatch runner ping, which still runs first. - A failed provision left the row QUEUED, which now holds the lock, so it would block every later run. Failure releases the row and drops the partition. Configuration — limits and timeouts were hard-coded: - New evaluation config block (EVAL_*, OPENRAG_INTERNAL_URL, PROMPTFOO_BIN) wired through the existing Pydantic + env-override mechanism. - Contracts stay constants on purpose: partition prefix, CSV columns, the file_id alphabet. Making those configurable would invalidate stored datasets. Simplification and cost: - Corpus uploads stream to disk instead of buffering up to 512 MB in RAM. - Rejected duplicate corpus basenames, which previously overwrote silently and left corpus_file_count overstated. - Removed dead code: active_run(), now superseded by the index, and the expected_file_ids var no assertion reads. is_busy() is deliberately kept — it is the liveness probe start_run pings. - Deduped the httpx client construction and the UI's duplicated POLL_MS. - Fresh assertion copies per test so the promptfoo YAML has no aliases. Also registers get_evaluation_service in the DI wiring test, which the feature commits missed — it was failing on the branch.
Comments described one deployment's incidents rather than the code: a container's absolute storage path, a distro's Node version, the symptoms of a specific failed run. Rewritten to state what the code does and why, in terms true of any install. Also genericised a test fixture path.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
openrag/services/workers/eval_runner.py (1)
191-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind
partition/file_idcontext on these warning logs instead of rawself._logger.
run()uses a context-boundlog = self._logger.bind(run_id=..., partition=...), but the per-file indexing warning and the partition-drop warnings use unboundself._loggerwith values only interpolated into the message string. As per coding guidelines, "Use structured Loguru logging throughget_logger(), binding relevant context such asfile_idandpartition."♻️ Bind context instead of string-interpolating it
- self._logger.warning(f"Eval corpus file '{path.name}' failed to index: {exc}") + self._logger.bind(partition=partition, file_id=sanitize_file_id(path.name)).warning( + f"Eval corpus file '{path.name}' failed to index: {exc}" + )- if response.status_code >= 400: - self._logger.warning(f"Could not drop eval partition '{partition}': {response.status_code}") - except Exception as exc: # noqa: BLE001 — teardown must not mask the run's outcome - self._logger.warning(f"Could not drop eval partition '{partition}': {exc}") + if response.status_code >= 400: + self._logger.bind(partition=partition).warning( + f"Could not drop eval partition: {response.status_code}" + ) + except Exception as exc: # noqa: BLE001 — teardown must not mask the run's outcome + self._logger.bind(partition=partition).warning(f"Could not drop eval partition: {exc}")Also applies to: 349-353
🤖 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/workers/eval_runner.py` around lines 191 - 193, Update the per-file indexing warning in run() and the partition-drop warnings to use context-bound Loguru loggers instead of raw self._logger; bind the relevant partition and file_id/path context as structured fields, while retaining the existing warning messages and run-level context.Source: Coding guidelines
openrag/core/evaluation/identity.py (1)
15-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSanitized
file_idcollisions aren't detected at upload time.Two distinct corpus filenames can normalize to the same
file_id(e.g."invoice#1.pdf"and"invoice_1.pdf"both becomeinvoice_1.pdf). The upload-time duplicate check inevaluation_service.pyonly compares raw basenames (Path(filename).name), so this collision passes validation. During indexing (eval_runner.py::_index_corpus), the second upload then silently collides with the first file'sfile_id, corrupting corpus coverage and skewing indexing/retrieval metrics without any visible error.Consider rejecting corpus uploads when two files sanitize to the same
file_id, using this same function, rather than only comparing raw basenames.🤖 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/core/evaluation/identity.py` around lines 15 - 17, Update the upload-time duplicate validation in evaluation_service.py to derive each filename’s file_id through sanitize_file_id and reject uploads when two sanitized IDs match, including collisions from distinct raw basenames. Reuse sanitize_file_id from openrag/core/evaluation/identity.py and preserve the existing duplicate-error behavior for conflicting IDs.
🤖 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.
Inline comments:
In `@conf/config.yaml`:
- Around line 365-368: Remove the duplicate max_download_bytes entry from the
mcp configuration mapping, leaving a single 104857600-byte setting before the
Evaluation section.
In `@openrag/api/routers/admin/evaluation.py`:
- Around line 80-85: Update the admin evaluation upload flow before
EvaluationService.create_dataset to enforce max_testset_bytes while reading
testset: avoid unbounded await testset.read(), read at most the configured limit
plus one byte, and reject oversized uploads before passing data to dataset
creation or CSV parsing. Preserve the existing bounded corpus stream handling
and normal behavior for uploads within the limit.
- Around line 82-85: Move the synchronous corpus-copying work in
EvaluationService.create_dataset, including _copy_within_budget and its
stream.read/target.open/write operations, off the event-loop thread by running
it through a worker-thread mechanism such as asyncio.to_thread. Preserve the
existing corpus filenames, upload contents, and budget behavior while keeping
the request coroutine non-blocking.
---
Nitpick comments:
In `@openrag/core/evaluation/identity.py`:
- Around line 15-17: Update the upload-time duplicate validation in
evaluation_service.py to derive each filename’s file_id through sanitize_file_id
and reject uploads when two sanitized IDs match, including collisions from
distinct raw basenames. Reuse sanitize_file_id from
openrag/core/evaluation/identity.py and preserve the existing duplicate-error
behavior for conflicting IDs.
In `@openrag/services/workers/eval_runner.py`:
- Around line 191-193: Update the per-file indexing warning in run() and the
partition-drop warnings to use context-bound Loguru loggers instead of raw
self._logger; bind the relevant partition and file_id/path context as structured
fields, while retaining the existing warning messages and run-level context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ebd50231-aaac-48b8-9cbc-1ecf5072335f
📒 Files selected for processing (28)
CLAUDE.mdconf/config.yamldocs/content/docs/documentation/env_vars.mdinfra/docker/api.Dockerfileinfra/docker/ray.Dockerfileopenrag/api/routers/admin/evaluation.pyopenrag/core/config/evaluation.pyopenrag/core/config/loader.pyopenrag/core/config/root.pyopenrag/core/evaluation/__init__.pyopenrag/core/evaluation/identity.pyopenrag/core/evaluation/metrics.pyopenrag/core/evaluation/promptfoo_config.pyopenrag/core/evaluation/testset.pyopenrag/core/ports/evaluation_repo.pyopenrag/services/orchestrators/evaluation_service.pyopenrag/services/persistence/evaluation_repo.pyopenrag/services/persistence/migrations/alembic/versions/a7c9e1f2b3d4_add_evaluation_tables.pyopenrag/services/persistence/schema.pyopenrag/services/workers/eval_runner.pytests/unit/core/evaluation/test_metrics.pytests/unit/core/evaluation/test_promptfoo_config.pytests/unit/core/evaluation/test_testset.pytests/unit/di/test_container.pytests/unit/services/orchestrators/test_evaluation_service.pyui/src/lib/api/evaluation.tsui/src/pages/admin/evaluation/index.tsxui/src/pages/admin/evaluation/run-detail.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- openrag/core/evaluation/init.py
- ui/src/lib/api/evaluation.ts
- openrag/services/persistence/migrations/alembic/versions/a7c9e1f2b3d4_add_evaluation_tables.py
- infra/docker/api.Dockerfile
- openrag/services/persistence/schema.py
- openrag/core/config/root.py
- openrag/core/ports/evaluation_repo.py
- infra/docker/ray.Dockerfile
- CLAUDE.md
- ui/src/pages/admin/evaluation/index.tsx
- openrag/core/evaluation/metrics.py
- tests/unit/core/evaluation/test_testset.py
- ui/src/pages/admin/evaluation/run-detail.tsx
- openrag/services/orchestrators/evaluation_service.py
- conf/config.yaml: drop a duplicated `max_download_bytes` key in the `mcp:` block, introduced when the `evaluation:` section was appended. Invalid YAML that stricter loaders reject. - Partition listings: `list_partition_summaries` is what `GET /partition/` responds from, so a throwaway `__eval_<run_id>` could still surface there even though `list_partitions` hid it. Both filter now. - Dataset upload: the test set was read unbounded before its size was checked. It is now read to one byte past the cap and rejected there, and the corpus copy runs on a worker thread instead of blocking the event loop. - Test-set parsing: the row cap is enforced on the row that would exceed it, and only the reported errors are retained rather than every one. - `delete_dataset` is refused while a run is using the dataset — the runner reads the corpus off disk for the whole indexing phase, so removing it mid-run surfaced as a FileNotFoundError instead of a conflict. - UI: the dialog's Cancel button bypassed the form reset that backdrop and Escape go through, so a reopened dialog silently kept the previous selection. Run rows are now keyboard-selectable. Verified locally: ruff, layer guard, 2267 unit tests, tsc, eslint on the files this PR changes, and the 9 evaluation UI tests.
EvaluationService reached for `build_eval_runner()` and drove the actor handle itself, so an orchestrator held Ray imports and `.remote()` calls — the one place Phase 9 reserves for `services/workers/`. It also meant the unit tests had to stub `services.workers.ray_utils` into `sys.modules` and fake an actor handle whose attributes return objects with `.remote()`. Introduce the `EvaluationRunner` port (is_busy / dispatch / cancel) and the `RayEvaluationRunner` adapter, injected by the container — the same shape as IndexingDispatcher. The adapter resolves the detached actor lazily, so building the service (e.g. to list datasets) still does not spawn a worker. The service now has no Ray import at all and its tests bind a plain in-memory fake. Also move `internal_url` from `evaluation` to `server` config (env var `OPENRAG_INTERNAL_URL` is unchanged): now that the base URL crosses the port as a dispatch argument, it is plainly a property of the server every out-of-process worker calls back on, not of the evaluation feature. While here, resolve the eval service user via the `UserRepository` port's `get_user_by_external_id` instead of the `_dict` variant the port does not declare, and mark `get_evaluation_service` a required container provider now that it wires cleanly.
Ahmath-Gadji
force-pushed
the
feat/evaluation-page
branch
from
July 27, 2026 12:35
a7af1af to
c80b40b
Compare
This was referenced Jul 27, 2026
This was referenced Jul 27, 2026
Ahmath-Gadji
marked this pull request as draft
July 27, 2026 13:17
Collaborator
|
Split into a stacked chain of 15 PRs — #812 … #826 — each under 500 LOC. See the table at the top of the description for the order. Verification, so reviewers do not have to take the split on trust:
Two files had to be introduced across two PRs each, because either would have blown the 500-line budget in one: The |
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
3 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.
Important
Superseded — this PR has been split into a stacked chain of 15 reviewable PRs.
It stays open as a draft tracker because the design discussion and review history live here. Do not merge it.
The chain
Merge in order. Each PR targets the previous one's branch, so its diff shows only its own change; GitHub retargets automatically as each lands. The tree at the tip of the chain is byte-identical to this branch (verified with
git diff), and every part leavesruff, the layer-import guard, the unit suite,tsc -b,eslintand the UI tests in the same state asdevelop.EvaluationConfig,server.internal_url,resolved_rdb()) and domain modelsfile_idnormalisationeval_datasets/eval_runspersistence + migrationEvaluationRunnerportEvalRunnerRay actor and its dispatcherLayering. Parts 1–5 are pure logic with no I/O. 6–9 are storage and orchestration. 10–11 are the worker and the HTTP surface — the backend is complete and usable at 11. 12–14 are the UI, ordered data layer → components → page. 15 is documentation.
Functional between parts. Nothing is reachable until part 14 registers the tab, so every intermediate state is inert-but-valid rather than half-wired. The one place this cost something is
metrics.pyandEvaluationService, each of which is introduced across two parts (3/4 and 7/9) because a single part would have exceeded 500 LOC.Original description
What
Adds an admin-only Evaluation tab to the System page. A run indexes an uploaded corpus into a throwaway partition, replays a CSV test set through promptfoo, and reports three metric families:
GET /search/partition/{p}responsesPOST /v1/chat/completionsresponsesHow it works
The runner drives OpenRAG through its own HTTP API rather than in-process calls: that is the path real uploads take, so the timings mean something, and it is the same surface promptfoo talks to.
Two promptfoo configs, not one. Retrieval targets
/search, whose documents carry the chunk undercontent(context-relevancegrades it) andmetadata.file_id(feeds the ranking metrics). Answers target/v1/chat/completions, reduced to the assistant message. Splitting them means every assertion applies to a single provider, so none can run against an output shape it cannot read.Notable decisions
tests/load/automatic-evaluation-pipeline/README.md, so these numbers mean the same thing as the offline pipeline's.expected_file_idsare skipped, not failed. Scoring them as misses would make a sparsely-annotated test set look like a broken retriever; the run reports the skipped count.llm.base_url/llm.model), so model-graded assertions need no third-party credentials.__openrag_eval__whose token is regenerated at the start of every run. That keeps a single service account in the DB while ensuring no usable plaintext token is ever stored at rest.__eval_*partitions are filtered out of the partition listing so a throwaway never shows up as a user-facing collection.file_idsanitisation. The indexing API accepts only[A-Za-z0-9._:-], so a corpus file cannot be uploaded under a raw filename. Both the uploader and the ground-truth comparison go throughcore/evaluation/identity.py, so an author keeps writing real filenames.POST /evaluation/runsreturns 409, so indexing timings stay comparable. Enforced by the partial unique indexux_eval_runs_single_active: the run row is created before the token is regenerated, so racing starts cannot revoke each other's credentials.api.Dockerfileandray.Dockerfile(compose runs Ray inside the API container), not run vianpx promptfoo@latest: a run should not depend on npm reachability or on CLI behaviour changing under a deployment that was not rebuilt. Node 22 comes from NodeSource — distro packages predate promptfoo's floor.Testing
file_idsanitisation on both sides of the match, and the run-lifecycle service paths.ruff, the layer-import guard,tsc -bandeslintall pass.Reviewer notes
file_idrejection, the invalid answer transform, the unwritable promptfoo config dir and the unresolved DB name — all fixed in the commits above. promptfoo still does not parse the generated configs in CI; they are asserted structurally.uihas 31 pre-existing test failures ondevelop(localStorageundefined under jsdom); this branch neither adds to nor fixes them.env_vars.mdwith working defaults:OPENRAG_INTERNAL_URL,PROMPTFOO_BIN, and theEVAL_*tunables (EVAL_MAX_CORPUS_MB,EVAL_MAX_TESTSET_MB,EVAL_MAX_TESTSET_ROWS,EVAL_TOP_K,EVAL_TASK_TIMEOUT,EVAL_TASK_POLL_SECONDS,EVAL_HTTP_TIMEOUT,EVAL_PROMPTFOO_TIMEOUT). They resolve through the existing Pydantic + env-override mechanism (evaluation:inconf/config.yaml).a7c9e1f2b3d4is idempotent per the repo's alembic rules.Update — audit pass
A review pass over the whole feature. Four defects fixed, plus configuration and simplification work.
Correctness
p50/p95reported the wrong file.round()breaks ties to even, so the nearest-rank index was off by one wheneverfraction * nlanded on an odd integer —p50of two files returned the slower one. Nowceil, per the definition.start_runreadactive_run()and then regenerated the shared eval user's token; two requests could pass that check, and the loser invalidated the token the winner was still indexing with. The run row is now created before anything is provisioned, and a partial unique index makes that insert the lock. Composes with the pre-dispatch runner ping, which still runs first.QUEUED, which now holds the lock, blocking every later run. Failure releases the row and drops the partition.get_evaluation_servicewas never registered in the provider registry.Configuration — limits and timeouts were hard-coded. They now resolve through the project's Pydantic + env-override mechanism. Domain contracts stay constants on purpose: the reserved partition prefix, the CSV column names and the
file_idalphabet would invalidate stored datasets if retuned.Cost and simplification
corpus_file_countoverstated.active_run(), an unread promptfoo var) and deduplicated the httpx client and the UI's repeated poll constant.Not changed, flagged for a decision
files_per_minutemeasures serialised throughput, not the system's real concurrency./searchdefaultssimilarity_thresholdto0.75and the eval does not override it, so retrieval can return fewer thantop_kand depress recall for reasons unrelated to ranking.<data_dir>on shared storage — the runner reads dataset files by path.__eval_-prefixed partitions, which are now hidden from listings.Verification:
ruff, the layer-import guard and 2262 unit tests pass. The one failure,test_content_deduplication_can_be_disabled_by_env, reproduces ondevelopand is unrelated. Frontend checks (tsc -b,eslint, UI tests) were not re-run — dependencies are not installed in this environment; three UI files changed, all import-only edits for the shared poll constant.Summary by CodeRabbit