Model Config: Add model configuration table and API endpoints#669
Closed
Model Config: Add model configuration table and API endpoints#669
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete model configuration management system with database schema, CRUD operations, and API endpoints for retrieving active AI model configurations with filtering and pagination support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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.
Comment on lines
+11
to
+16
| 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.
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
2 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.
Summary
Target issue is #635
Explain the motivation for making this change. What existing problem does the pull request solve?
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
Release Notes