Skip to content

fix: make top_p parameter optional for Azure AI models #347

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: make top_p parameter optional for Azure AI models #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions api/simple_chat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,11 @@ async def chat_completions_stream(request: ChatCompletionRequest):
model_kwargs = {
"model": request.model,
"stream": True,
"temperature": model_config["temperature"],
"top_p": model_config["top_p"]
"temperature": model_config["temperature"]
}
# Only add top_p if it exists in the model config
if "top_p" in model_config:
model_kwargs["top_p"] = model_config["top_p"]
Comment on lines 423 to +430
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While this change correctly makes top_p optional for the Azure provider, it highlights a code duplication issue. The logic for constructing model_kwargs is now identical for the openrouter, openai, and azure providers. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this common logic into a helper function. This would centralize the logic and make future modifications easier.

For example:

def _prepare_common_model_kwargs(model_config, request_model):
    model_kwargs = {
        "model": request_model,
        "stream": True,
        "temperature": model_config["temperature"],
    }
    if "top_p" in model_config:
        model_kwargs["top_p"] = model_config["top_p"]
    return model_kwargs


api_kwargs = model.convert_inputs_to_api_kwargs(
input=prompt,
Expand Down
6 changes: 4 additions & 2 deletions api/websocket_wiki.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,9 +502,11 @@ async def handle_websocket_chat(websocket: WebSocket):
model_kwargs = {
"model": request.model,
"stream": True,
"temperature": model_config["temperature"],
"top_p": model_config["top_p"]
"temperature": model_config["temperature"]
}
# Only add top_p if it exists in the model config
if "top_p" in model_config:
model_kwargs["top_p"] = model_config["top_p"]
Comment on lines 502 to +509
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the change in simple_chat.py, this logic for constructing model_kwargs is duplicated. It's identical for openrouter, openai, and azure providers. Refactoring this repeated code into a shared utility function would significantly improve code maintainability and reduce redundancy across both websocket_wiki.py and simple_chat.py.


api_kwargs = model.convert_inputs_to_api_kwargs(
input=prompt,
Expand Down