feat(evaluation) 1/15: config surface and domain models - #812
Open
Ahmath-Gadji wants to merge 6 commits into
Open
feat(evaluation) 1/15: config surface and domain models#812Ahmath-Gadji wants to merge 6 commits into
Ahmath-Gadji wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds centralized evaluation configuration and domain models, supports internal worker API access, centralizes RDB database resolution, and isolates temporary evaluation partitions from normal listings and wildcard retrieval. ChangesEvaluation configuration and environment wiring
Evaluation domain models
Resolved RDB configuration
Evaluation partition isolation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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
the
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: 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 `@openrag/core/config/infrastructure.py`:
- Around line 101-105: Update infrastructure configuration so internal_url does
not permanently default to port 8080 and instead derives from the
container-internal APP_iPORT, while preserving OPENRAG_INTERNAL_URL as an
override. In conf/config.yaml lines 128-131, set the deployed internal URL using
the configured container port. In docs/content/docs/documentation/env_vars.md
lines 575-575, document that OPENRAG_INTERNAL_URL must be overridden when the
internal port differs.
🪄 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: 50e2a422-3a6d-40e4-a42f-403e07a81a25
📒 Files selected for processing (11)
conf/config.yamldocs/content/docs/documentation/env_vars.mdopenrag/core/config/evaluation.pyopenrag/core/config/infrastructure.pyopenrag/core/config/loader.pyopenrag/core/config/root.pyopenrag/core/models/evaluation.pyopenrag/di/repositories.pyopenrag/services/orchestrators/partition_service.pytests/unit/core/config/test_resolved_rdb.pytests/unit/services/orchestrators/test_partition_preset_resolution.py
Ahmath-Gadji
changed the title
First slice of the admin evaluation feature. Adds the pieces every later slice depends on and nothing that runs yet: - `EvaluationConfig` (`evaluation:` in conf/config.yaml) holding the run limits and timeouts, all env-overridable via `EVAL_*` / `PROMPTFOO_BIN`. Domain contracts stay out of it on purpose — the reserved partition prefix, the CSV column names and the `file_id` alphabet would invalidate stored datasets if they were retunable. - `server.internal_url` (`OPENRAG_INTERNAL_URL`): how an out-of-process worker reaches the API from inside the deployment. - `Settings.resolved_rdb()`: the Postgres database name is optionally derived from the Milvus collection. Any process opening its own connection has to derive it identically, so the rule moves out of `di/repositories.py` and onto `Settings`. - `core/models/evaluation.py`: the dataclasses and the run status enum the repository, service, worker and API all speak in. - Partition listings filter out `__eval_*`, so a run's throwaway partition never surfaces as a user-facing collection.
Ahmath-Gadji
force-pushed
the
eval/01-config-and-models
branch
from
July 27, 2026 13:31
7a9a880 to
9407f34
Compare
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 `@docs/content/docs/documentation/env_vars.md`:
- Line 681: Update the evaluation configuration validation description near the
operational limits statement to say that numeric limits must be strictly
positive while each field follows its own validation rules. Explicitly account
for string-valued settings such as PROMPTFOO_BIN and field-specific bounds such
as EVAL_TOP_K, while preserving the existing statements about non-configurable
contracts and OPENRAG_INTERNAL_URL.
🪄 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: 1d3a0ce2-0324-4b39-abef-b256a2f9bf71
📒 Files selected for processing (16)
conf/config.yamldocs/content/docs/documentation/env_vars.mdopenrag/core/config/evaluation.pyopenrag/core/config/infrastructure.pyopenrag/core/config/loader.pyopenrag/core/config/root.pyopenrag/core/models/__init__.pyopenrag/core/models/evaluation.pyopenrag/di/repositories.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/orchestrators/retrieval_service.pytests/unit/core/config/test_evaluation_config.pytests/unit/core/config/test_resolved_rdb.pytests/unit/core/config/test_server_internal_url.pytests/unit/services/orchestrators/test_partition_preset_resolution.pytests/unit/services/orchestrators/test_retrieval_service.py
🚧 Files skipped from review as they are similar to previous changes (7)
- openrag/di/repositories.py
- tests/unit/core/config/test_resolved_rdb.py
- openrag/core/config/root.py
- openrag/core/config/loader.py
- conf/config.yaml
- openrag/core/config/evaluation.py
- openrag/core/models/evaluation.py
Review findings on part 1/15.
Reserve `__eval_` at partition creation. Hiding the prefix from the
listings without reserving it hands any user an invisible partition:
`__eval_x` is a real, quota-consuming partition that no admin listing and
no `partitions=all` search can see. The check reuses the existing
`_RESERVED_PARTITION_NAMES` site and lowercases first, so `__EVAL_x`
cannot sit just outside the filter. `allow_reserved` is the internal
escape hatch a run needs to create its own partition; it is not reachable
over HTTP.
Bound every `EvaluationConfig` field. All nine are env-overridable and
none was constrained, against a codebase that spells `gt=0` everywhere.
`EVAL_TOP_K=-5` was accepted and `EVAL_MAX_CORPUS_MB=-1` yielded a
negative byte cap that rejects every upload; both now fail at load, where
the error names the field.
Fall back on an empty `APP_iPORT`, not just an absent one. entrypoint.sh
and docker-compose.yaml both spell it `${APP_iPORT:-8080}`, which covers
either; `os.environ.get(..., '8080')` covered only absent, so a bare
`APP_iPORT=` line in an env file produced `http://openrag:`.
Rename `EVAL_TASK_POLL_SECONDS` to `EVAL_TASK_POLL_INTERVAL`, the only
one of the four durations that carried its unit. Cheap now, a deprecation
cycle after release.
Tests: the empty-`APP_iPORT` case; two loader-level assertions against
the shipped conf/config.yaml, so a literal `internal_url:` re-added there
cannot silently defeat the default factory; the nine `EVAL_*` mappings,
with a guard that the table covers every field; the bounds; and the
reserved prefix with its escape hatch.
Also move the evaluation variables out of the FastAPI table in
env_vars.md into their own section, and correct the core/models docstring
that claimed the package is pure Pydantic.
Further review finding on part 1/15. Hiding `__eval_*` from `list_partitions` and `list_partition_summaries` does not hide it from `partitions=all`. `create_partition` calls `load_partitions()`, which hydrates `Settings.partitions` from the unfiltered repo, and `RetrievalService._pipeline_groups_for_partitions` expands the wildcard to `list(configs.keys())`. So while a run is in flight, a SUPER_ADMIN_MODE admin on `openrag-all` draws chat context from the throwaway partition and gets sources attributed to a partition no listing shows and no detail view can reach. Filter the expansion, not the hydration. The cache has to keep the eval partition: the run measures retrieval by searching it *by name*, and dropping it from `Settings.partitions` would make `_require_partition_config` raise `PartitionNotFoundError` on the very query the run exists to time. Only the meaning of "all" is narrowed. When nothing user-facing is left, the expansion yields no partition groups and the query returns empty rather than falling back to the unscoped legacy pipeline — the fallback is the bug, not the remedy. The raw `GET /search?partitions=all` is deliberately left alone. There the wildcard is not expanded to a list at all: it reaches `_build_filter_expr` as the documented "no partition clause" sentinel, and excluding a prefix would mean inventing a negative clause inside the fail-closed logic that #706 hardened. An admin asking for an explicitly unscoped search getting an unscoped search is defensible; the comment in `create_partition` now says so instead of over-claiming, and reserving the prefix is what bounds that surface to a live run's own rows. Tests: the wildcard skipping an eval partition, the named path still reaching it (the regression that would break a run), and the eval-only-deployment case returning nothing. The first and third fail without the change.
Ahmath-Gadji
force-pushed
the
eval/01-config-and-models
branch
from
July 27, 2026 15:30
a9f7fd5 to
7b7f3fe
Compare
…d_rdb() Settings.resolved_rdb() was introduced here to give the partitions_for_collection_<collection> derivation one owner, reachable from a Ray worker. Two copies of it survived that move: services/workers/indexer_pool.py _catalog_rdb_config services/persistence/migrations/run.py _rdb_config_for_migrations Both take a Settings and are the same four lines, so a change to how the catalog database is named would have had to land in three places to be true everywhere — the drift this method exists to prevent. alembic/env.py keeps its own copy deliberately. It builds a URL rather than an RDBConfig, and unlike the other two it ignores an explicit rdb.database. Routing it through resolved_rdb() would start honouring POSTGRES_DATABASE on a bare `alembic upgrade head`, which is a behaviour change that wants its own PR, not a quiet ride-along in a refactor. The migration-entrypoint tests moved off the deleted private helper and onto main() itself, asserting the RDBConfig the ConnectionManager is handed. Same guarantee, stated where it matters: standalone migrations must open the database the API opens. The derivation's own cases are already covered by tests/unit/core/config/test_resolved_rdb.py. indexer_pool.py imported Settings as `openrag.core.config.root` while importing its siblings as `core.config.*` — the same class under two module paths. Restored under the form the rest of the file, and the project convention, already use. The indexer_pool tests hand the actor a SimpleNamespace config, two of which carried a hand-rolled RDBConfig with its own model_copy — a stub of the derivation. They now stub resolved_rdb() instead, and the claim-store test uses a real Settings so it exercises the method rather than a look-alike.
…n k8s
server.internal_url defaults to http://openrag:$APP_iPORT. That is right
for compose, where both openrag and openrag-cpu carry the `openrag`
network alias from the x-openrag template, under either profile.
It cannot be right for this chart. The API Service is named
{{ .Release.Name }}-openrag, so the host does not resolve — and the
workers that use this URL run in the RayCluster, a separate pod, so
there is no loopback to fall back on. Every evaluation run on Helm would
have failed at the first corpus upload.
env.config values are rendered through `tpl`, so the value follows the
release name the same way BASE_URL and VDB_HOST already do, and takes
its port from openrag.service.port — the same value that opens the
container port and the Service port, so the three cannot drift.
Verified by rendering configmap-env.yaml standalone (the chart's
subchart dependencies aren't vendored):
release "rel" -> http://rel-openrag:8080
--set openrag.service.port=9090 -> http://rel-openrag:9090
Reserving a partition name was split across two layers and two modules.
`_RESERVED_PARTITION_NAMES` (the `all` sentinel) lived in
services/orchestrators/partition_service.py; the `__eval_` prefix rule
lived in core/models/evaluation.py. create_partition composed the two by
hand, and two generic orchestrators — PartitionService and
RetrievalService — imported the *evaluation* domain to answer a question
about partition naming.
core/models/partition.py now owns both:
RESERVED_PARTITION_NAMES closed to every caller
INTERNAL_PARTITION_PREFIXES a namespace, open to the subsystem that owns it
is_internal_partition() what the listings and the "all" fan-out filter on
is_reserved_partition_name() what create_partition checks
The two rules read alike but are not the same, which is why keeping them
apart matters: `all` collides with the cross-partition sentinel and is
refused even to a caller passing allow_reserved, while `__eval_` is a
namespace whose owner is admitted. Both are matched on the stripped,
lowercased name, so neither " all " nor `__EVAL_x` can be spelled to
sit just outside the check.
core/models/evaluation.py keeps EVAL_PARTITION_PREFIX and
is_eval_partition — the evaluation code still builds `__eval_<run_id>`
from them. The registry imports the prefix rather than restating it, so
the literal has one definition and registering the next internal
namespace is a line in INTERNAL_PARTITION_PREFIXES, not an edit to four
call sites.
Behaviour is unchanged; the existing partition and retrieval tests cover
that and pass untouched. The reasoning that was inlined at the call site
moved to the module that now states the rule, and the new unit tests pin
the asymmetry between the two kinds of reservation.
hedhoud
requested changes
Jul 28, 2026
hedhoud
left a comment
hedhoud
left a comment
Collaborator
There was a problem hiding this comment.
I found one blocking deployment issue. The configuration and partition-focused tests otherwise pass locally.
| # not resolve here — this chart's Service is <release>-openrag, and the | ||
| # RayCluster that runs those workers is a separate pod. Follows | ||
| # openrag.service.port, the same value that opens the container port. | ||
| OPENRAG_INTERNAL_URL: "http://{{ .Release.Name }}-openrag:{{ .Values.openrag.service.port }}" |
Collaborator
There was a problem hiding this comment.
Could you please update this URL to use the Ray Serve service? In the Helm setup, the API listens on raycluster-head-svc:RAY_SERVE_PORT, not on <release>-openrag:openrag.service.port. With the current value, evaluation runs on Kubernetes will fail as soon as they try to call the API.
Please also make the default use RAY_SERVE_PORT when Ray Serve is enabled, and add a test for the rendered Helm value. Thanks.
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.
Part 1 of 15 of the split of #811 (admin Evaluation tab), which was too large to review as one change. Each part is < 500 LOC and leaves the tree building and green; the feature becomes reachable in part 13.
Merge in order. Each PR targets the previous one's branch, so its diff shows only its own change.
What
The pieces every later slice depends on, and nothing that runs yet.
core/config/evaluation.pyEvaluationConfig— run limits and timeouts, env-overridable (EVAL_*,PROMPTFOO_BIN)core/config/infrastructure.pyserver.internal_url(OPENRAG_INTERNAL_URL)core/config/root.pyevaluation:intoSettings; addsSettings.resolved_rdb()core/models/evaluation.pyEvalRunStatus, the vocabulary the repo/service/worker/API shareservices/orchestrators/partition_service.py__eval_*partitions from listingsNotable
file_idalphabet stay constants: changing them would invalidate datasets already on disk.resolved_rdb()moved ontoSettings.rdb.databaseis optional and historically derived from the Milvus collection name. That derivation lived insidedi/repositories.py, which is unreachable from a Ray worker opening its own connection — part 9 needs exactly the same name. Same behaviour, one owner.is_eval_partitionand is harmless before any eval partition can exist.Review follow-up
internal_urlnow followsAPP_iPORT(CodeRabbit). The first draft defaulted it to a hard-codedhttp://openrag:8080, which is wrong for any deployment that moves the container-internal port:infra/scripts/entrypoint.shbinds uvicorn to${APP_iPORT:-8080}and compose maps${APP_PORT}:${APP_iPORT}, so workers would have kept calling 8080 while the API listened elsewhere.OPENRAG_INTERNAL_URLwas an escape hatch, but only for someone who already knew to reach for it.The default is now built by a factory reading
APP_iPORT, and the literal value is removed fromconf/config.yamlso the factory is what applies. Verified through the real loader, not just the model:APP_iPORT=9999 load_config().server.internal_url→http://openrag:9999. Three tests intest_server_internal_url.pypin the derived port, the 8080 fallback, and that an explicit value still wins — the last one matters becauseOPENRAG_INTERNAL_URLresolves onto this same field and must survive the default factory.Testing
ruff check/ruff format --check, the layer-import guard, and 2214 unit tests pass. The single failure,test_content_deduplication_can_be_disabled_by_env, reproduces ondevelopand is unrelated.Summary by CodeRabbit
top_k, and runner/grading timeouts).OPENRAG_INTERNAL_URL) for out-of-process workers (e.g., evaluation)."all"retrieval expansion (while still allowing direct access by name).OPENRAG_INTERNAL_URLand evaluation (EVAL_*) environment settings, including Kubernetes notes.