fix: Validate the completion params that are passed as input to the API #3 #17
Conversation
… openai param being passed in isn't supported
katwinkl3
force-pushed
the
katwinkl3/validate_completion_params
branch
from
866718e to
3230d76
Compare
kelsey-wong
left a comment
kelsey-wong
left a comment
There was a problem hiding this comment.
sorry I had these comments but never submitted them!
| REQUIRED_CHAT_COMPLETION_PARAMS: FrozenSet[str] = frozenset({"messages"}) | ||
|
|
||
|
|
||
| def _validate_chat_completion_params(params: CompletionParams) -> None: |
There was a problem hiding this comment.
should we also check that messages is non-empty? (i'm not sure if a chat completion can be generated without any messages)
There was a problem hiding this comment.
and can we also validate that the message items are properly formatted, e.g. include role and content?
https://docs.litellm.ai/docs/completion/input#required-fields
| @@ -0,0 +1,53 @@ | |||
| """Validation helpers for chat completion parameter dictionaries.""" | |||
|
|
|||
| # from litellm import get_supported_openai_params | |||
| if role != "assistant" or function_call is None: | ||
| raise ValueError(f"messages[{index}]['content'] must be a string.") |
There was a problem hiding this comment.
Can this error message be more specific/helpful? Not sure the best phrasing but something like this. Also I can't tell but it seems like there might be a logical error in the condition - is it never okay for function_call to be None if the content is None? Would be helpful to explain that in the error msg, probably by splitting the conditions
| if role != "assistant" or function_call is None: | |
| raise ValueError(f"messages[{index}]['content'] must be a string.") | |
| if role != "assistant": | |
| raise ValueError(f"Non-assistant message at index {index} must have content.") | |
| if function_call is None: | |
| raise ValueError(f"messages[{index}] must have either content or function_call key.") |
There was a problem hiding this comment.
Added this logic after referring to litellm's required field explanation: "content: string or list[dict] or null - The contents of the message. It is required for all messages, but may be null for assistant messages with function calls.", I'll split the the if logic up with more fitting err msg
|
There is some interaction between this PR and the new API i believe. Changes should not be huge but just wanted to make a note: #31 |
Co-authored-by: Kelsey Wong <kelsey.wong@cleanlab.ai>
Key Info
What changed?
Added hardcoded validation for openai_args to check if messages is a key. Nothing else is checked since LiteLLM raises an exception if the param being passed in isn't supported.
referenced https://docs.litellm.ai/docs/completion/input
What do you want the reviewer(s) to focus on?
Checklist