Add Phase 14 admin schemas for endpoints and presets - #442
Closed
hedhoud wants to merge 7 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Phase 14 admin Pydantic schemas (model endpoints, partitions, presets), admin routers for model endpoints and presets, DI providers for optional Phase‑14 services, extends admin package exports, and adds unit tests covering normalization, defaults, explicit-null rejection, and update payload requirements. ChangesPhase 14 Admin Schemas & Routers
Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminRouter
participant ModelEndpointService
participant ModelEndpointStore
Client->>AdminRouter: POST/PUT/GET /model-endpoints...
AdminRouter->>ModelEndpointService: create/update/get/validate (payload/model_type,name)
ModelEndpointService->>ModelEndpointStore: persist/read/set-default/validate
ModelEndpointStore-->>ModelEndpointService: record/validation result
ModelEndpointService-->>AdminRouter: ModelEndpointResponse / ValidateEndpointResponse
AdminRouter-->>Client: HTTP response (201/200/204)
classDiagram
class CreatePartitionRequest {
string name
string embedder_name = "default"
string indexation_preset = "default"
string retrieval_preset = "default"
int chat_history_depth = 0
}
class UpdatePartitionRequest {
string|None name
string|None indexation_preset
string|None retrieval_preset
string|None description
int|None chat_history_depth
}
class CreatePresetRequest {
string name
PresetType preset_type
dict config = {}
}
class UpdatePresetRequest {
string|None name
dict|None config
}
class PresetResponse {
string name
PresetType preset_type
dict config
datetime created_at
datetime updated_at
}
CreatePartitionRequest --|> UpdatePartitionRequest
CreatePresetRequest --|> UpdatePresetRequest
UpdatePresetRequest --|> PresetResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/schemas/admin/model_endpoint_schemas.py`:
- Around line 37-43: The current validate_endpoint field_validator trims
whitespace but checks non-emptiness before removing trailing slashes, so inputs
like "/" or "///" pass then normalize to an empty string; update
validate_endpoint to first strip whitespace, then compute a normalized value =
value.rstrip("/") (or similar) and validate that normalized is non-empty,
raising ValueError("endpoint must be non-empty") if it is empty, and finally
return the normalized value; ensure you update the classmethod validate_endpoint
accordingly so callers get the sanitized, non-empty endpoint.
In `@openrag/api/schemas/admin/partition_schemas.py`:
- Around line 38-54: UpdatePartitionRequest currently accepts explicit nulls for
fields that should not be cleared; add validators to reject None when the client
explicitly sets embedder, indexation_preset, retrieval_preset, description, or
chat_history_depth. Concretely, update or add field validators (e.g.,
on_validate for "embedder", "indexation_preset", "retrieval_preset",
"description", "chat_history_depth") that raise a ValueError if the incoming
value is None (but allow omission), keep using _normalize_name for non-empty
strings in validate_non_empty_name, and leave require_at_least_one_update and
model_fields_set logic intact so clients must provide at least one non-null
update.
In `@openrag/api/schemas/admin/preset_schemas.py`:
- Around line 36-48: Update UpdatePresetRequest so PATCH payloads with explicit
nulls are rejected: add field validators for "name" and "config" that raise when
value is None (so UpdatePresetRequest(name=None) / (config=None) are invalid),
and change the existing model_validator require_at_least_one_update to check
that at least one of the updatable fields is both present and not None (e.g.,
use self.model_fields_set plus a check like any(getattr(self, f) is not None for
f in ("name","config"))). Reference UpdatePresetRequest, the
validate_name/_normalize_name flow, and require_at_least_one_update when making
these changes.
🪄 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
Run ID: fbba401e-ec47-4ef6-8e30-d4c2e7abc1ce
📒 Files selected for processing (5)
openrag/api/schemas/admin/__init__.pyopenrag/api/schemas/admin/model_endpoint_schemas.pyopenrag/api/schemas/admin/partition_schemas.pyopenrag/api/schemas/admin/preset_schemas.pytests/unit/api/schemas/admin/test_phase14_schemas.py
hedhoud
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/api/routers/admin/partitions.py (1)
36-43: 💤 Low value503 vs 501 for an unimplemented method.
HTTP_503_SERVICE_UNAVAILABLEsignals a transient/overloaded condition and may trigger client/proxy retries with backoff. Since this guard reflects a method that isn't wired yet (phased rollout),501 Not Implementedcommunicates "permanently unavailable in this build" more accurately and avoids retry storms. If the tests intentionally pin 503 as the contract, feel free to disregard.🤖 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/api/routers/admin/partitions.py` around lines 36 - 43, The guard function _require_service_method currently raises HTTPException with status.HTTP_503_SERVICE_UNAVAILABLE when a service method is not present; change it to use status.HTTP_501_NOT_IMPLEMENTED so missing/unimplemented methods return 501 instead of 503. Locate _require_service_method and update the status_code value in the HTTPException construction (keeping the detail message and callable check intact), and run/adjust any tests that assert the previous 503 behavior to expect 501.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openrag/api/routers/admin/partitions.py`:
- Around line 36-43: The guard function _require_service_method currently raises
HTTPException with status.HTTP_503_SERVICE_UNAVAILABLE when a service method is
not present; change it to use status.HTTP_501_NOT_IMPLEMENTED so
missing/unimplemented methods return 501 instead of 503. Locate
_require_service_method and update the status_code value in the HTTPException
construction (keeping the detail message and callable check intact), and
run/adjust any tests that assert the previous 503 behavior to expect 501.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba6338c4-e5bc-41ee-9b46-923efb8082ab
📒 Files selected for processing (2)
openrag/api/routers/admin/partitions.pytests/unit/api/routers/admin/test_phase14_partition_routes.py
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.
Context: Phase 14 introduces model endpoint and pipeline preset management, and Person B needs stable admin API contracts before wiring routers and runtime behavior.\n\nThis PR adds the dependency-light admin schemas for model endpoints, presets, and partition preset assignment. The goal is to make the API contract reviewable early while Person A finishes the backing services and database work.\n\nValidation: Ruff, layer guard, and the full unit suite pass locally.
Summary by CodeRabbit
New Features
Tests