-
Notifications
You must be signed in to change notification settings - Fork 10
feat(llm): Introduce type safety checks #1089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |||||||||||
|
|
||||||||||||
| from fastapi import APIRouter, Depends, HTTPException | ||||||||||||
| from opentelemetry import trace | ||||||||||||
| from pydantic import TypeAdapter | ||||||||||||
| from sqlmodel import Session | ||||||||||||
|
|
||||||||||||
| from app.api.deps import AuthContextDep, SessionDep | ||||||||||||
| from app.api.permissions import Permission, require_permission | ||||||||||||
|
|
@@ -19,13 +21,50 @@ | |||||||||||
| LLMJobPublic, | ||||||||||||
| JobStatus, | ||||||||||||
| ) | ||||||||||||
| from app.models.llm.response import LLMResponse, Usage | ||||||||||||
| from app.models.llm.response import LLMOutput, LLMResponse, Usage | ||||||||||||
| from app.services.llm.jobs import start_job | ||||||||||||
| from app.utils import APIResponse, validate_callback_url, load_description | ||||||||||||
|
|
||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||
|
|
||||||||||||
| router = APIRouter(tags=["LLM"]) | ||||||||||||
|
|
||||||||||||
| _LLM_OUTPUT_ADAPTER: TypeAdapter[LLMOutput] = TypeAdapter(LLMOutput) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _resolve_llm_output( | ||||||||||||
| raw_content: dict, | ||||||||||||
| project_id: int, | ||||||||||||
| session: Session, | ||||||||||||
| job_id: UUID, | ||||||||||||
| ) -> LLMOutput | None: | ||||||||||||
| """Parse the persisted `llm_call.content` dict into the typed LLMOutput, | ||||||||||||
| presigning the audio URL in place first. | ||||||||||||
|
|
||||||||||||
| Persisted TTS content marks a not-yet-presigned S3 path with format="uri" — | ||||||||||||
| not a valid AudioContent literal ("base64"/"url") — so that sentinel must be | ||||||||||||
| resolved to a real "url" before the dict can validate into the typed model. | ||||||||||||
| """ | ||||||||||||
| inner = raw_content.get("content") | ||||||||||||
| if ( | ||||||||||||
| raw_content.get("type") == "audio" | ||||||||||||
| and isinstance(inner, dict) | ||||||||||||
| and inner.get("format") == "uri" | ||||||||||||
| ): | ||||||||||||
| s3_path = inner.get("value", "") | ||||||||||||
| try: | ||||||||||||
| storage = get_cloud_storage(session, project_id) | ||||||||||||
| inner["value"] = storage.get_signed_url(s3_path, expires_in=3600) | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Name the presigned-URL TTL.
Proposed fix _LLM_OUTPUT_ADAPTER: TypeAdapter[LLMOutput] = TypeAdapter(LLMOutput)
+PRESIGNED_AUDIO_URL_TTL_SECONDS = 3_600
- inner["value"] = storage.get_signed_url(s3_path, expires_in=3600)
+ inner["value"] = storage.get_signed_url(
+ s3_path, expires_in=PRESIGNED_AUDIO_URL_TTL_SECONDS
+ )📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||
| except Exception as e: | ||||||||||||
| logger.warning( | ||||||||||||
| f"[get_llm_call_status] Failed to generate presigned URL for audio: {e} | job_id={job_id}" | ||||||||||||
| ) | ||||||||||||
|
Comment on lines
+59
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Use the helper’s name in this log prefix. This warning originates in 🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||
| inner["value"] = "" | ||||||||||||
| inner["format"] = "url" | ||||||||||||
|
|
||||||||||||
| return _LLM_OUTPUT_ADAPTER.validate_python(raw_content) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| llm_callback_router = APIRouter() | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
|
@@ -155,32 +194,19 @@ def get_llm_call_status( | |||||||||||
| # Get the first LLM call from the list which will be the only call for the job id | ||||||||||||
| # since we initially won't be using this endpoint for llm chains | ||||||||||||
| llm_call = llm_calls[0] | ||||||||||||
| output_payload = copy.deepcopy(llm_call.content) | ||||||||||||
| if ( | ||||||||||||
| isinstance(output_payload, dict) | ||||||||||||
| and output_payload.get("type") == "audio" | ||||||||||||
| and isinstance(output_payload.get("content"), dict) | ||||||||||||
| and output_payload["content"].get("format") == "uri" | ||||||||||||
| ): | ||||||||||||
| s3_path = output_payload["content"].get("value", "") | ||||||||||||
| try: | ||||||||||||
| storage = get_cloud_storage(session, project_id) | ||||||||||||
| output_payload["content"]["value"] = storage.get_signed_url( | ||||||||||||
| s3_path, expires_in=3600 | ||||||||||||
| ) | ||||||||||||
| except Exception as e: | ||||||||||||
| logger.warning( | ||||||||||||
| f"[get_llm_call_status] Failed to generate presigned URL for audio: {e} | job_id={job_id}" | ||||||||||||
| ) | ||||||||||||
| output_payload["content"]["value"] = "" | ||||||||||||
| output_payload["content"]["format"] = "url" | ||||||||||||
| raw_content = copy.deepcopy(llm_call.content) | ||||||||||||
| output = ( | ||||||||||||
| _resolve_llm_output(raw_content, project_id, session, job_id) | ||||||||||||
| if isinstance(raw_content, dict) | ||||||||||||
| else None | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| llm_response = LLMResponse( | ||||||||||||
| provider_response_id=llm_call.provider_response_id or "", | ||||||||||||
| conversation_id=llm_call.conversation_id, | ||||||||||||
| provider=llm_call.provider, | ||||||||||||
| model=llm_call.model, | ||||||||||||
| output=output_payload, | ||||||||||||
| output=output, | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| usage_payload = llm_call.usage | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -414,7 +414,13 @@ def submit_assessment_batch( | |
| completion = config_blob.completion | ||
| provider_name = completion.provider or "openai" | ||
|
|
||
| params = dict(completion.params) | ||
| # Native params are a plain dict; Kaapi params are now a typed submodel. | ||
| raw_params = completion.params | ||
| params = ( | ||
| dict(raw_params) | ||
| if isinstance(raw_params, dict) | ||
| else raw_params.model_dump(exclude_none=True) | ||
| ) | ||
|
Comment on lines
+417
to
+423
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Locate file =="
git ls-files | rg '(^|/)backend/app/crud/assessment/batch\.py$|TextLLMParams|completion\.params|model_dump' || true
echo
echo "== Relevant snippets =="
if [ -f backend/app/crud/assessment/batch.py ]; then
nl -ba backend/app/crud/assessment/batch.py | sed -n '380,450p'
fi
echo
echo "== Search TextLLMParams definition/usages =="
rg -n "class TextLLMParams|TextLLMParams|model_dump\\(|params\\.model_dump" backend/app -S || true
echo
echo "== Pydantic/model validation =="
rg -n "from pydantic import|import pydantic|BaseModel|Field\\(" backend/app -S || trueRepository: ProjectTech4DevAI/kaapi-backend Length of output: 290 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== File exists and line range =="
head -n 450 backend/app/crud/assessment/batch.py | tail -n 80
echo
echo "== Locate TextLLMParams definition/usages =="
grep -RIn "class TextLLMParams\|TextLLMParams\|model_dump\|completion\.params" backend/app -S || true
echo
echo "== Pydantic deps/framework snippets =="
grep -RIn "from pydantic import\|import pydantic\|BaseModel\|Field(" backend/app pyproject.toml requirements*.txt poetry.lock -S 2>/dev/null || trueRepository: ProjectTech4DevAI/kaapi-backend Length of output: 3254 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== TextLLMParams definition/usages =="
grep -RIn "class TextLLMParams\|TextLLMParams\|model_dump\|completion\.params" backend/app || true
echo
echo "== Pydantic deps/framework snippets =="
grep -RIn "from pydantic import\|import pydantic\|BaseModel\|Field(" backend/app pyproject.toml requirements*.txt poetry.lock 2>/dev/null || true
echo
echo "== Behavioral probe for Pydantic exclude_none vs exclude_unset =="
python3 - <<'PY'
import sys
print("python", sys.version)
try:
import pydantic
from pydantic import BaseModel, Field
print("pydantic", pydantic.__version__)
except Exception as e:
print("pydantic unavailable:", type(e).__name__, str(e))
sys.exit(0)
class Params(BaseModel):
temperature: float = 0.1
top_p: float | None = None
p = Params()
print("exclude_none:", p.model_dump(exclude_none=True))
print("exclude_unset:", p.model_dump(exclude_unset=True))
print("both:", p.model_dump(exclude_none=True, exclude_unset=True))
PYRepository: ProjectTech4DevAI/kaapi-backend Length of output: 50387 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== TextLLMParams definition =="
sed -n '1,90p' backend/app/models/llm/request.py
echo
echo "== Assessment model params default =="
sed -n '300,340p' backend/app/models/assessment.py
echo
echo "== Mapper behavior for unset temperature =="
sed -n '1,90p' backend/app/services/llm/mappers.py
sed -n '1,180p' backend/app/tests/services/llm/test_mappers.py
echo
echo "== Pydantic behavioral probe =="
python3 - <<'PY'
import sys
print("python", sys.version)
try:
import pydantic
from pydantic import BaseModel, Field
print("pydantic", pydantic.__version__)
except Exception as e:
print("pydantic unavailable:", type(e).__name__, str(e))
sys.exit(0)
class Params(BaseModel):
temperature: float = 0.1
top_p: float | None = None
p = Params()
print("exclude_none:", p.model_dump(exclude_none=True))
print("exclude_unset:", p.model_dump(exclude_unset=True))
print("both:", p.model_dump(exclude_none=True, exclude_unset=True))
PYRepository: ProjectTech4DevAI/kaapi-backend Length of output: 13820 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pydantic import BaseModel, Field
class Params(BaseModel):
temperature: float | None = 0.1
top_p: float | None = None
p = Params()
print("exclude_none:", p.model_dump(exclude_none=True))
print("exclude_unset:", p.model_dump(exclude_unset=True))
print("both:", p.model_dump(exclude_none=True, exclude_unset=True))
PYRepository: ProjectTech4DevAI/kaapi-backend Length of output: 290 🌐 Web query:
💡 Result: In Pydantic V2, the model_dump method (and model_dump_json) provides specific parameters to control which fields are included in the serialized output. These parameters are essential for tasks like partial updates (PATCH requests) where you only want to send fields that the user explicitly provided [1][2]. Key Parameters for Field Exclusion: exclude_unset: When set to True, this excludes any fields that were not explicitly set during model instantiation [1][3]. Pydantic tracks which fields were provided using the model_fields_set property [1][4]. If you instantiate a model and only provide one field, only that field will appear in the output when exclude_unset=True [1]. Note that if you modify a field on the instance after it has been created, it is considered set and will be included [1][4]. exclude_none: When set to True, this excludes any fields whose value is None, regardless of whether they were explicitly set or assigned a default [1][3]. This is useful for cleaning up outputs by removing null values [5]. exclude_defaults: When set to True, this excludes any fields whose value is equal to their default value (using the == equality operator) [1][4]. Distinction: exclude_unset focuses on the origin of the value (was it provided at creation?) [1]. exclude_none focuses on the actual value itself (is it None?) [1]. exclude_defaults focuses on whether the current value matches the defined default value [1]. These parameters can be used independently or in combination to precisely tailor the serialization output [1][2]. If a field is explicitly set to None, exclude_unset=True will keep it in the output, while exclude_none=True will remove it [5][6]. If you need to detect if a field was missing versus set to None, you can inspect the model_fields_set attribute directly [6]. Citations:
Preserve unset Kaapi params during batch normalization.
🤖 Prompt for AI Agents |
||
| params.pop("instructions", None) | ||
| params.pop("system_instruction", None) | ||
| if isinstance(system_instruction, str) and system_instruction.strip(): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,38 @@ class Modality(StrEnum): | |||||||||||||
| FILES = "FILES" | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| # BCP-47 language codes accepted by the speech-to-speech endpoint (STT input / | ||||||||||||||
| # TTS output). Single source of truth: `SUPPORTED_LANGUAGE_CODES` in | ||||||||||||||
| # `app/services/llm/chain/utils.py` derives from this via `get_args`. | ||||||||||||||
|
Comment on lines
+66
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Make the alias the documented source of truth. This comment calls Suggested wording-# TTS output). Single source of truth: `SUPPORTED_LANGUAGE_CODES` in
-# `app/services/llm/chain/utils.py` derives from this via `get_args`.
+# TTS output). This Literal is the single source of truth; `SUPPORTED_LANGUAGE_CODES`
+# in `app/services/llm/chain/utils.py` is derived from it via `get_args`.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| STSLanguageCode = Literal[ | ||||||||||||||
| "auto", | ||||||||||||||
| "unknown", | ||||||||||||||
| "en-IN", | ||||||||||||||
| "hi-IN", | ||||||||||||||
| "bn-IN", | ||||||||||||||
| "kn-IN", | ||||||||||||||
| "ml-IN", | ||||||||||||||
| "mr-IN", | ||||||||||||||
| "od-IN", | ||||||||||||||
| "pa-IN", | ||||||||||||||
| "ta-IN", | ||||||||||||||
| "te-IN", | ||||||||||||||
| "gu-IN", | ||||||||||||||
| "as-IN", | ||||||||||||||
| "ur-IN", | ||||||||||||||
| "ne-IN", | ||||||||||||||
| "kok-IN", | ||||||||||||||
| "ks-IN", | ||||||||||||||
| "sd-IN", | ||||||||||||||
| "sa-IN", | ||||||||||||||
| "sat-IN", | ||||||||||||||
| "mni-IN", | ||||||||||||||
| "brx-IN", | ||||||||||||||
| "mai-IN", | ||||||||||||||
| "doi-IN", | ||||||||||||||
| ] | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| DEFAULT_STT_MODEL = "gemini-2.5-pro" | ||||||||||||||
| DEFAULT_TTS_MODEL = "gemini-2.5-flash-preview-tts" | ||||||||||||||
| DEFAULT_TTS_VOICE = "Kore" | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the helper contract.
dictis unconstrained, and this function never returnsNone: validation either returnsLLMOutputor raises. As per coding guidelines, every parameter and return value needs a narrow type.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines