Model Config: Add model configuration table and API endpoints#669
Model Config: Add model configuration table and API endpoints#669
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 a model configuration subsystem: DB migration and SQLModel schemas, CRUD helpers, two authenticated FastAPI endpoints to list and fetch model configs (registered in main router), API documentation pages, and tests covering listing, filtering, retrieval, 404, and default-model lookup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "FastAPI /models"
participant CRUD as "crud.model_config"
participant DB as "Postgres (global.model_config)"
Client->>API: GET /api/v1/models?provider=...&skip=&limit=
API->>CRUD: get_active_models(provider, skip, limit)
CRUD->>DB: SELECT ... FROM global.model_config WHERE is_active [AND provider=...]
DB-->>CRUD: rows
CRUD-->>API: list of ModelConfigPublic + count
API-->>Client: 200 OK (APIResponse{data, count})
Client->>API: GET /api/v1/models/{provider}/{model_name}
API->>CRUD: get_model_config(provider, model_name)
CRUD->>DB: SELECT ... WHERE provider=... AND model_name=... AND is_active
DB-->>CRUD: row / empty
CRUD-->>API: ModelConfigPublic / None
API-->>Client: 200 OK (model) / 404 Not Found
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/app/crud/model_config.py (1)
14-20: Use SQLAlchemy boolean predicates instead of== True.These filters work, but
ModelConfig.is_active == Trueis the noisy/non-idiomatic form here and is already tripping Ruff. PreferModelConfig.is_active.is_(True)in all three queries.♻️ Proposed fix
statement = ( select(ModelConfig) .where( - ModelConfig.is_active == True, + ModelConfig.is_active.is_(True), ModelConfig.default_for == completion_type, ) .limit(1) ) - statement = select(ModelConfig).where(ModelConfig.is_active == True) + statement = select(ModelConfig).where(ModelConfig.is_active.is_(True)) statement = select(ModelConfig).where( ModelConfig.provider == provider, ModelConfig.model_name == model_name, - ModelConfig.is_active == True, + ModelConfig.is_active.is_(True), )Also applies to: 32-32, 45-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/model_config.py` around lines 14 - 20, Replace non-idiomatic boolean comparisons using "== True" with SQLAlchemy boolean predicates: change occurrences like ModelConfig.is_active == True to ModelConfig.is_active.is_(True). Update this in all three query constructions that reference ModelConfig.is_active (the select where block filtering by ModelConfig.is_active and ModelConfig.default_for == completion_type and the other two similar queries), ensuring you use .is_(True) for each boolean filter while leaving other predicates (e.g., ModelConfig.default_for == completion_type) unchanged.backend/app/api/routes/model_config.py (2)
19-25: Confirm the auth gate here is intentional.Both handlers depend on
AuthContextDep, andbackend/app/api/deps.py:38-91makes that a hard 401/403 gate. If the model catalog needs to be available before login or project selection, this will block it. If auth is intentional, rename the parameter to_auth_context(or suppress ARG001) so the dependency-only use is explicit.Also applies to: 39-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/model_config.py` around lines 19 - 25, The handler list_models (and the other handler at lines 39-41) currently take AuthContextDep as a required parameter which enforces the hard 401/403 gate from backend/app/api/deps.py; decide whether the model catalog should be public or protected and update accordingly: if it must be public, remove or make the auth dependency optional (so the handlers don't require AuthContextDep), otherwise keep the dependency but rename the parameter to _auth_context (or add an ARG001 suppression) to make the dependency-only usage explicit; reference the AuthContextDep type and the list_models function (and the second handler) when making the change.
26-30: Makecountunambiguous for paginated responses.After applying
skip/limit,count=len(models)only reports the current page size. If clients use this field as pagination metadata, it will be misleading; either return the total matching rows or rename/drop the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/model_config.py` around lines 26 - 30, The code returns count=len(models) after applying skip/limit (in the route using get_active_models, APIResponse.success_response, and ModelConfigListPublic), which makes pagination metadata ambiguous; change the endpoint to return the total number of matching rows (not just the page length) by adding a separate count query (e.g., implement/get a get_active_models_count or modify get_active_models to return (items, total)), then pass that total into ModelConfigListPublic as count (or rename/drop the field if you prefer API-breaking behavior); update the route to use the new total value instead of len(models) so clients receive unambiguous pagination info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/alembic/versions/050_create_model_config_table.py`:
- Line 20: Add explicit return type hints to the Alembic entrypoint functions:
update the upgrade() and downgrade() function definitions to include return
annotations (e.g., def upgrade() -> None: and def downgrade() -> None:) so both
functions have explicit return types per the project typing guidelines; ensure
the annotations appear on the existing upgrade and downgrade functions in the
migration module.
In `@backend/app/models/model_config.py`:
- Around line 11-16: The model schema uses Python-side Literals (e.g., the
provider Field in model_config.py and the other Literal-backed fields around
lines 53-65 and 78-83) but does not enforce those allowed values at the DB
level; add SQL CHECK constraints to the SQLAlchemy model (e.g., on the provider
column and the default_for/other Literal-backed columns) to restrict values to
the same allowed set and update the Alembic migration to create the matching
CHECK constraints so values inserted outside the application cannot violate the
contract; ensure the constraint names are unique and descriptive and that the
model's sa_column includes sa.CheckConstraint(...) matching the Literal choices.
---
Nitpick comments:
In `@backend/app/api/routes/model_config.py`:
- Around line 19-25: The handler list_models (and the other handler at lines
39-41) currently take AuthContextDep as a required parameter which enforces the
hard 401/403 gate from backend/app/api/deps.py; decide whether the model catalog
should be public or protected and update accordingly: if it must be public,
remove or make the auth dependency optional (so the handlers don't require
AuthContextDep), otherwise keep the dependency but rename the parameter to
_auth_context (or add an ARG001 suppression) to make the dependency-only usage
explicit; reference the AuthContextDep type and the list_models function (and
the second handler) when making the change.
- Around line 26-30: The code returns count=len(models) after applying
skip/limit (in the route using get_active_models, APIResponse.success_response,
and ModelConfigListPublic), which makes pagination metadata ambiguous; change
the endpoint to return the total number of matching rows (not just the page
length) by adding a separate count query (e.g., implement/get a
get_active_models_count or modify get_active_models to return (items, total)),
then pass that total into ModelConfigListPublic as count (or rename/drop the
field if you prefer API-breaking behavior); update the route to use the new
total value instead of len(models) so clients receive unambiguous pagination
info.
In `@backend/app/crud/model_config.py`:
- Around line 14-20: Replace non-idiomatic boolean comparisons using "== True"
with SQLAlchemy boolean predicates: change occurrences like
ModelConfig.is_active == True to ModelConfig.is_active.is_(True). Update this in
all three query constructions that reference ModelConfig.is_active (the select
where block filtering by ModelConfig.is_active and ModelConfig.default_for ==
completion_type and the other two similar queries), ensuring you use .is_(True)
for each boolean filter while leaving other predicates (e.g.,
ModelConfig.default_for == completion_type) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44029a83-9a0c-41d5-8d67-c62c7e008119
📒 Files selected for processing (8)
backend/app/alembic/versions/050_create_model_config_table.pybackend/app/api/docs/model_config/get_model.mdbackend/app/api/docs/model_config/list_models.mdbackend/app/api/main.pybackend/app/api/routes/model_config.pybackend/app/crud/model_config.pybackend/app/models/__init__.pybackend/app/models/model_config.py
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade(): |
There was a problem hiding this comment.
Add return annotations to the Alembic entrypoints.
upgrade and downgrade are new Python functions and currently miss return type hints.
✏️ Proposed fix
-def upgrade():
+def upgrade() -> None:
op.create_table(
"model_config",
...
)
-def downgrade():
+def downgrade() -> None:
op.drop_table("model_config", schema="global")Also applies to: 131-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/alembic/versions/050_create_model_config_table.py` at line 20,
Add explicit return type hints to the Alembic entrypoint functions: update the
upgrade() and downgrade() function definitions to include return annotations
(e.g., def upgrade() -> None: and def downgrade() -> None:) so both functions
have explicit return types per the project typing guidelines; ensure the
annotations appear on the existing upgrade and downgrade functions in the
migration module.
| provider: Literal["openai", "google"] = Field( | ||
| default="openai", | ||
| sa_column=sa.Column( | ||
| sa.String, nullable=False, comment="provider name (e.g. openai, google)" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Encode the allowed domains in the schema, not just the type hints.
Literal only constrains Python-side validation; the backing columns are still plain VARCHAR. A row inserted outside the API can persist unsupported provider or default_for values and leave the API serving data its own model contract says is impossible. Add CheckConstraints here and mirror them in the migration.
🛡️ Proposed fix
class ModelConfig(ModelConfigBase, table=True):
__tablename__ = "model_config"
__table_args__ = (
+ sa.CheckConstraint(
+ "provider IN ('openai', 'google')",
+ name="ck_model_config_provider",
+ ),
+ sa.CheckConstraint(
+ "default_for IS NULL OR default_for IN ('text', 'stt', 'tts')",
+ name="ck_model_config_default_for",
+ ),
sa.UniqueConstraint("provider", "model_name"),
{"schema": "global"},
)Also applies to: 53-65, 78-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/model_config.py` around lines 11 - 16, The model schema
uses Python-side Literals (e.g., the provider Field in model_config.py and the
other Literal-backed fields around lines 53-65 and 78-83) but does not enforce
those allowed values at the DB level; add SQL CHECK constraints to the
SQLAlchemy model (e.g., on the provider column and the default_for/other
Literal-backed columns) to restrict values to the same allowed set and update
the Alembic migration to create the matching CHECK constraints so values
inserted outside the application cannot violate the contract; ensure the
constraint names are unique and descriptive and that the model's sa_column
includes sa.CheckConstraint(...) matching the Literal choices.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/alembic/versions/051_create_model_config_table.py`:
- Around line 1-17: The migration file has the wrong revision identifiers:
update the module-level variables so revision = "051" and down_revision = "050"
(and if present update any matching revision string in the file
header/docstring) to align the migration ordering; modify the revision and
down_revision symbols in the file to those exact values so Alembic picks up the
correct parent migration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27a6c79b-1439-45e3-a000-a189cfae5198
📒 Files selected for processing (1)
backend/app/alembic/versions/051_create_model_config_table.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/tests/api/routes/test_model_config.py`:
- Around line 16-20: The tests under
backend/app/tests/api/routes/test_model_config.py rely on seeded DB state;
replace those implicit assumptions by creating deterministic model records via
the project's factories (e.g., ModelFactory or equivalent) inside each test or a
fixture and then assert against those created instances: create at least one
active model before the count/active assertions (targeting the assertions that
currently check body["data"]["count"] and all(m["is_active"] ...)), create
specific model(s) named/typed to represent the openai/gpt-4o and default "text"
model before the tests that assert their presence, and update assertions to
check for the factory-created records rather than global seed data (use the
test's created model IDs/names in the response expectations and ensure cleanup
or transactional fixtures are used).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29372d38-b949-4f10-aa44-fb8b6b662119
📒 Files selected for processing (2)
backend/app/alembic/versions/051_create_model_config_table.pybackend/app/tests/api/routes/test_model_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/alembic/versions/051_create_model_config_table.py
| assert response.status_code == 200 | ||
| body = response.json() | ||
| assert body["success"] is True | ||
| assert body["data"]["count"] > 0 | ||
| assert all(m["is_active"] for m in body["data"]["data"]) |
There was a problem hiding this comment.
Make these tests deterministic with factory-created model records
Lines [19], [47]-[48], and [66]-[68] currently assume seeded data already exists (active models, openai/gpt-4o, and a default "text" model). This can make CI flaky across environments. Please create test data via factories in each test (or shared factory fixtures), then assert against those created records.
Suggested direction (example)
def test_get_model(
- client: TestClient, superuser_token_headers: dict[str, str]
+ client: TestClient,
+ superuser_token_headers: dict[str, str],
+ model_config_factory,
) -> None:
+ created = model_config_factory(
+ provider="openai",
+ model_name="test-model",
+ is_active=True,
+ default_for="text",
+ )
response = client.get(
- f"{settings.API_V1_STR}/models/openai/gpt-4o",
+ f"{settings.API_V1_STR}/models/{created.provider}/{created.model_name}",
headers=superuser_token_headers,
)As per coding guidelines backend/app/tests/**/*.py: Use factory pattern for test fixtures in backend/app/tests/.
Also applies to: 31-34, 45-49, 66-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/api/routes/test_model_config.py` around lines 16 - 20, The
tests under backend/app/tests/api/routes/test_model_config.py rely on seeded DB
state; replace those implicit assumptions by creating deterministic model
records via the project's factories (e.g., ModelFactory or equivalent) inside
each test or a fixture and then assert against those created instances: create
at least one active model before the count/active assertions (targeting the
assertions that currently check body["data"]["count"] and all(m["is_active"]
...)), create specific model(s) named/typed to represent the openai/gpt-4o and
default "text" model before the tests that assert their presence, and update
assertions to check for the factory-created records rather than global seed data
(use the test's created model IDs/names in the response expectations and ensure
cleanup or transactional fixtures are used).
| }, | ||
| "input_modalities": ["TEXT", "IMAGE"], | ||
| "output_modalities": ["TEXT"], | ||
| "default_for": "text", |
There was a problem hiding this comment.
i think this is removed, can you update doc
There was a problem hiding this comment.
default_for is removed but I didn't removed the modalities
|
|
||
|
|
||
| def upgrade(): | ||
| op.create_table( |
There was a problem hiding this comment.
add inserted_at and updated_at
There was a problem hiding this comment.
the table does have this two columns
| skip: int = 0, | ||
| limit: int = 100, |
There was a problem hiding this comment.
If skip and limit are being used here, then we should also include a has_more parameter in the metadata in response. Otherwise, pagination won’t work properly in this case.
If it’s required, we can keep it; otherwise, we can remove it if it’s not needed.
4 participants
Summary
Target issue is #635
Explain the motivation for making this change. What existing problem does the pull request solve?
Currently, there’s no structured way to expose model capabilities and provider support from the kaapi-backend. All models are shown to users, since different models support different parameters. This leads to issues like passing unsupported params (e.g., temperature for GPT-5), causing 400 errors, and the frontend has no way to validate configs.
To fix this, a model_config table is introduced with:
Example structure:
Now, the backend returns only curated models with their config schema
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores