Improve generation parameter validation and retriever initialization safety#49
Improve generation parameter validation and retriever initialization safety#49Sasyamerugu wants to merge 1 commit intosugarlabs:mainfrom
Conversation
Sasyamerugu
force-pushed
the
fix/validate-generation-params
branch
from
4afd5c6 to
9d28dd9
Compare
|
Looking at your change, there's no functional change, you just formatted what was already in existence, so I'll be closing this. |
|
Your commit message hasn't improved even after I talked about it, I'll leave this for the other maintainers to review. |
Sasyamerugu
force-pushed
the
fix/validate-generation-params
branch
from
9d28dd9 to
1cd4e45
Compare
Sasyamerugu
changed the title
|
Thanks for the feedback. I've now squashed the commits and rewritten the commit message to follow the recommended structure (imperative summary, blank line, and detailed explanation of the problem and fix). Please let me know if there’s anything specific you'd like adjusted further. |
mebinthattil
left a comment
mebinthattil
left a comment
There was a problem hiding this comment.
Kindly go through the comments and make the changes required, thanks.
app/routes/api.py
Outdated
| from fastapi import APIRouter, Depends, HTTPException, Header, Query, Request | ||
| from sqlalchemy.orm import Session | ||
| from pydantic import BaseModel, Field | ||
| from pydantic import BaseModel, Field |
| content: str | ||
|
|
||
| class PromptedLLMRequest(BaseModel): | ||
| """Request model for ask-llm-prompted endpoint""" |
There was a problem hiding this comment.
I'm not sure why you removed this docstring. Please add that back.
app/routes/api.py
Outdated
| top_p: float = Field(0.9, description="Top-p (nucleus) sampling parameter") | ||
| top_k: int = Field(50, description="Top-k sampling parameter") | ||
| chat: bool = False | ||
| question: Optional[str] = None |
There was a problem hiding this comment.
Why have the descriptions for chat, question, custom_prompt and messages been removed? Please add them back.
app/routes/api.py
Outdated
| 1024, | ||
| ge=1, | ||
| le=2048, | ||
| description="Must be between 1 and 2048" |
There was a problem hiding this comment.
The description for max_length should be "Maximum length of generated text" and not "Must be between 1 and 2048" as the latter is a constraint and not a description.
The same applies to truncation, repetition_penalty, temperature and top_k as well.
…r initialization Invalid generation parameters such as negative top_k were previously forwarded to the model layer, which could result in runtime errors. This change adds ge/le constraints to generation parameters in PromptedLLMRequest so that invalid inputs are rejected with a 422 validation error at the API layer. Additionally, retriever initialization is now guarded to prevent startup errors when DOC_PATHS is empty.
Sasyamerugu
force-pushed
the
fix/validate-generation-params
branch
from
1cd4e45 to
334be95
Compare
|
Thanks for the review. |
3 participants
Summary
This PR improves input validation and startup robustness in the API layer.
Previously, invalid generation parameters (e.g., negative top_k) were forwarded directly to the model, which could result in internal server errors. With this update, invalid inputs are rejected at the API layer and return a 422 validation error instead.
Additionally, this also prevents a crash during startup when DOC_PATHS is empty by conditionally initializing the retriever.
Changes
1. Added bounds checking for:
• max_length
• temperature
• top_p
• top_k
• repetition_penalty
2. Ensured invalid inputs return 422 validation errors instead of 500.
3. Added safe handling for empty DOC_PATHS to avoid retriever initialization errors.
Testing
Tested locally by running the FastAPI server and verifying:
• Valid inputs work as expected.
• Invalid inputs return proper validation errors (422).
• Application starts correctly even when DOC_PATHS is empty.