Model Config: DB-driven validator, seed sarvamai/elevenlabs/google#859
Model Config: DB-driven validator, seed sarvamai/elevenlabs/google#859vprashrex wants to merge 11 commits into
Conversation
…ons for STT/TTS providers
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds migrations and seed data, expands ModelConfig provider values, removes static model/voice whitelists, implements modality-aware DB-backed model/voice validation, integrates validation into config CRUD and job execution, and updates tests to exercise the new validation. ChangesModel Provider Expansion and Database-Driven Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
…ai, and ElevenLabs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
vprashrex
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/models/llm/request.py (1)
265-282:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a return type annotation to
validate_params.The method at line 265 is missing a return type hint. Since it returns
self(aKaapiCompletionConfiginstance), add the return type annotation.Proposed fix
- def validate_params(self): + def validate_params(self) -> "KaapiCompletionConfig":As per coding guidelines:
**/*.pyrequires type hints on all function parameters and return values; Use Python 3.11+ with type hints throughout the codebase.🤖 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 `@backend/app/models/llm/request.py` around lines 265 - 282, The method validate_params currently lacks a return type annotation; update its signature to annotate the return as the containing class (KaapiCompletionConfig) — i.e., change def validate_params(self) to def validate_params(self) -> "KaapiCompletionConfig" (use a forward reference string if the class is defined later or add from __future__ import annotations) so that callers know it returns self; keep the implementation unchanged and ensure imports/annotations comply with Python 3.11 typing rules.
🧹 Nitpick comments (4)
backend/app/tests/crud/test_model_config.py (1)
214-214: ⚡ Quick winRemove unused
monkeypatchfixture parameters in these tests.Both functions currently trigger Ruff
ARG001and can be simplified safely.♻️ Proposed fix
-def test_validate_blob_none_provider_skips(monkeypatch: pytest.MonkeyPatch) -> None: +def test_validate_blob_none_provider_skips() -> None: @@ -def test_validate_blob_missing_model_raises(monkeypatch: pytest.MonkeyPatch) -> None: +def test_validate_blob_missing_model_raises() -> None:Also applies to: 220-220
🤖 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 `@backend/app/tests/crud/test_model_config.py` at line 214, The two tests test_validate_blob_none_provider_skips and the other test at the nearby block currently accept an unused monkeypatch fixture causing Ruff ARG001; remove the unused monkeypatch parameter from their function signatures (e.g., change def test_validate_blob_none_provider_skips(monkeypatch: pytest.MonkeyPatch) -> None: to def test_validate_blob_none_provider_skips() -> None:) and run tests to ensure no remaining references to monkeypatch exist.backend/app/alembic/versions/063_seed_stt_tts_model_configs.py (1)
32-67: ⚡ Quick winAdd return type hints for
upgradeanddowngrade.Please annotate both functions with
-> Nonefor consistency with the project typing rule.As per coding guidelines: "
**/*.py: Always add type hints to all function parameters and return values in Python code".🤖 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 `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py` around lines 32 - 67, The upgrade and downgrade functions lack return type annotations; add "-> None" to both function definitions (upgrade and downgrade) so their signatures read with a None return type, keeping the bodies unchanged and conforming to the project's typing rule for Python functions.backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.py (1)
35-56: ⚡ Quick winAdd explicit return type hints to migration functions.
upgrade/downgradeshould be annotated as-> Noneto match repo typing standards.As per coding guidelines: "
**/*.py: Always add type hints to all function parameters and return values in Python code".🤖 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 `@backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.py` around lines 35 - 56, Annotate the migration entry points with explicit return type hints: change the function signatures of upgrade and downgrade to include -> None (i.e., def upgrade() -> None: and def downgrade() -> None:) so they match the repository typing standards and satisfy the rule to add return type hints for all functions like the ones that call op.get_context().autocommit_block() and op.create_index/op.drop_index.backend/app/crud/model_config.py (1)
66-87: ⚡ Quick winType-annotate
_modality_filtersignature.
stmtand return type are currently implicit. Add explicit types (e.g.,Select-based typing) to satisfy repo-wide typing requirements.As per coding guidelines: "
**/*.py: Always add type hints to all function parameters and return values in Python code".🤖 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 `@backend/app/crud/model_config.py` around lines 66 - 87, Update the _modality_filter signature to add explicit SQLAlchemy Select typing: change def _modality_filter(stmt, completion_type: CompletionType): to def _modality_filter(stmt: Select, completion_type: CompletionType) -> Select: and add the appropriate import for Select (e.g. from sqlalchemy.sql import Select or from sqlalchemy.sql.selectable import Select) at the top of the module; keep all existing logic using ModelConfig, ARRAY, and sqltypes.String unchanged so the function still returns the filtered Select object.
🤖 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 `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py`:
- Around line 66-83: The current downgrade() uses op.execute(...) to
unconditionally DELETE rows matching provider/model_name which may remove
pre-existing data; either make downgrade() a no-op or modify both upgrade() and
downgrade(): in upgrade() (the INSERT ... ON CONFLICT ... DO NOTHING block) add
a deterministic marker (e.g., set a seeded_by or migration_tag column or INSERT
and capture RETURNING id into a temp table) so you can identify which rows this
revision actually created, and then change downgrade() to only DELETE rows that
match those inserted records (matching the marker or the captured ids) rather
than deleting by provider/model_name alone.
In `@backend/app/crud/model_config.py`:
- Around line 119-132: The docstring for the validation block is inconsistent
with the code: it says native configs must include completion.params.model but
the function returns early for raw_provider.endswith("-native") (see
blob.completion, raw_provider, completion_type) and thus skips that validation;
fix by making them consistent—either remove the early return and enforce the
model presence for all providers (delete the raw_provider.endswith("-native")
return and validate completion.params.model against model_config) or update the
docstring to explicitly state that native providers
(raw_provider.endswith("-native")) are exempt from the model requirement so
callers know native-provider behavior is allowed to omit
completion.params.model.
In `@backend/app/tests/crud/test_model_config.py`:
- Around line 166-168: Add explicit type hints for the helper functions:
annotate _make_blob parameters (e.g., provider: str, completion_type: str,
params: Mapping[str, Any]) and its return type (e.g., SimpleNamespace) and do
the same for the inner function boom (declare its parameters and return type).
Also rename boom's varargs to underscored names (e.g., _args, _kwargs) to
silence unused-arg lint warnings. Apply the same parameter/return annotation
updates to the other helper on lines ~201-203 as well.
---
Outside diff comments:
In `@backend/app/models/llm/request.py`:
- Around line 265-282: The method validate_params currently lacks a return type
annotation; update its signature to annotate the return as the containing class
(KaapiCompletionConfig) — i.e., change def validate_params(self) to def
validate_params(self) -> "KaapiCompletionConfig" (use a forward reference string
if the class is defined later or add from __future__ import annotations) so that
callers know it returns self; keep the implementation unchanged and ensure
imports/annotations comply with Python 3.11 typing rules.
---
Nitpick comments:
In `@backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.py`:
- Around line 35-56: Annotate the migration entry points with explicit return
type hints: change the function signatures of upgrade and downgrade to include
-> None (i.e., def upgrade() -> None: and def downgrade() -> None:) so they
match the repository typing standards and satisfy the rule to add return type
hints for all functions like the ones that call
op.get_context().autocommit_block() and op.create_index/op.drop_index.
In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py`:
- Around line 32-67: The upgrade and downgrade functions lack return type
annotations; add "-> None" to both function definitions (upgrade and downgrade)
so their signatures read with a None return type, keeping the bodies unchanged
and conforming to the project's typing rule for Python functions.
In `@backend/app/crud/model_config.py`:
- Around line 66-87: Update the _modality_filter signature to add explicit
SQLAlchemy Select typing: change def _modality_filter(stmt, completion_type:
CompletionType): to def _modality_filter(stmt: Select, completion_type:
CompletionType) -> Select: and add the appropriate import for Select (e.g. from
sqlalchemy.sql import Select or from sqlalchemy.sql.selectable import Select) at
the top of the module; keep all existing logic using ModelConfig, ARRAY, and
sqltypes.String unchanged so the function still returns the filtered Select
object.
In `@backend/app/tests/crud/test_model_config.py`:
- Line 214: The two tests test_validate_blob_none_provider_skips and the other
test at the nearby block currently accept an unused monkeypatch fixture causing
Ruff ARG001; remove the unused monkeypatch parameter from their function
signatures (e.g., change def test_validate_blob_none_provider_skips(monkeypatch:
pytest.MonkeyPatch) -> None: to def test_validate_blob_none_provider_skips() ->
None:) and run tests to ensure no remaining references to monkeypatch exist.
🪄 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: adea4ba3-9ad8-449f-9bb1-55caa06b92ef
📒 Files selected for processing (11)
backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.pybackend/app/alembic/versions/063_seed_stt_tts_model_configs.pybackend/app/crud/config/config.pybackend/app/crud/config/version.pybackend/app/crud/model_config.pybackend/app/models/llm/constants.pybackend/app/models/llm/request.pybackend/app/models/model_config.pybackend/app/tests/api/routes/configs/test_version.pybackend/app/tests/crud/test_model_config.pybackend/app/tests/models/llm/test_request.py
💤 Files with no reviewable changes (3)
- backend/app/tests/api/routes/configs/test_version.py
- backend/app/tests/models/llm/test_request.py
- backend/app/models/llm/constants.py
| return session.exec(stmt).first() is not None | ||
|
|
||
|
|
||
| def validate_blob_model_or_raise(session: Session, blob: Any) -> None: |
There was a problem hiding this comment.
inline-blob path on /llm/call and /llm/chain no longer validates model/voice. Before this PR, model/voice checks lived in KaapiCompletionConfig.validate_params, so FastAPI ran them on every request body that contained a ConfigBlob — including ad-hoc inline blobs sent to /llm/call. This PR moves the check into validate_blob_model_or_raise (which needs a DB session) and only wires it into ConfigCrud.create_or_raise and ConfigVersionCrud.create_or_raise. The inline-blob branch in services/llm/jobs.py:525 (config_blob = config.blob) never calls it, so a client can now POST {"provider": "google", "type": "tts", "params": {"model": "gemini-99-ultra", "voice": "Nonexistent"}} and the request will be accepted, a job row created, and the failure surfaces deep in the worker instead of as a 4xx at request time
| assert data["data"]["version"]["config_blob"]["completion"]["type"] == "text" | ||
|
|
||
|
|
||
| def test_create_version_with_kaapi_stt_provider_success( |
There was a problem hiding this comment.
can you add these coverage testcases as well
- test_create_version_cannot_change_type_from_stt_to_tts — start with a google + gemini-2.5-pro STT config, try to bump it to google + gemini-2.5-flash-preview-tts TTS, expect 400 immutability error.
- test_create_version_cannot_change_type_from_tts_to_text — start with a google + gemini-2.5-flash-preview-tts TTS config, try to bump it to openai + gpt-4o text, expect 400 immutability error.
- test_create_version_with_kaapi_stt_provider_success — create a google + gemini-2.5-pro STT config, post a new version that just tweaks instructions/temperature, expect 201 with version == 2 and type == "stt".
- test_create_version_with_kaapi_tts_provider_success — create a google + gemini-2.5-flash-preview-tts (voice Kore) TTS config, post a new version switching to e.g. gemini-2.5-pro-preview-tts with a different seeded voice, expect 201 with version == 2 and type == "tts".
| DEFAULT_TTS_VOICE, | ||
| SUPPORTED_MODELS, | ||
| SUPPORTED_VOICES, | ||
| ) |
There was a problem hiding this comment.
here we need to add check that STT/TTS is not supported for OpenAI and send appropriate error message
There was a problem hiding this comment.
in model config there is already an column for input and output modalities which is set .. that has AUDIO, TEXT AND IMAGE ... from that TTS/STT capabilities can be checked
There was a problem hiding this comment.
{
"success": false,
"data": null,
"error": "Provider 'openai' does not support completion type='tts'.",
"errors": null,
"metadata": null
}
…ng database schema for STT/TTS support
…nd update type hints in test functions
…ity and structure
3 participants
Target issue is: #693
Summary
Previously, config/version creation accepted any
completion.params.modelwithout validating it againstmodel_config, allowing unsupported or misspelled models to fail only at inference time. STT/TTS providers and models for Google, Sarvamai, and ElevenLabs were also missing frommodel_config.This update adds model validation during config/version creation, seeds missing STT/TTS provider configs, and centralizes provider/model validation within the CRUD layer.
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
Improvements
Performance