-
Notifications
You must be signed in to change notification settings - Fork 27
fix: properly handle extra_body parameter in completions API #413
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
Conversation
The extra_body parameter was being passed incorrectly to the OpenAI SDK,
causing the entire kwargs dict (including the 'extra_body' key itself)
to be merged into the request body.
Before this fix:
- User calls: create(..., extra_body={'a': 'b'})
- Request body contained: {'extra_body': {'a': 'b'}} (wrong)
After this fix:
- User calls: create(..., extra_body={'a': 'b'})
- Request body contains: {'a': 'b'} (correct)
The fix extracts extra_body from kwargs, merges its contents with
remaining kwargs, and passes the merged result to the OpenAI SDK's
extra_body parameter. This matches the expected OpenAI SDK behavior.
Fixed in:
- chat_complete.py: stream_create, normal_create (sync + async)
- complete.py: stream_create, normal_create (sync + async)
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.
Pull request overview
This PR fixes a bug where the extra_body parameter was incorrectly passed to the OpenAI SDK, causing the entire kwargs dict (including the extra_body key itself) to be nested in the request body instead of merging its contents at the top level.
Key Changes:
- Extract and merge
extra_bodycontents with remaining kwargs before passing to OpenAI SDK - Properly handle special parameters (
extra_headers,extra_query,timeout) by extracting them separately - Apply fix to both sync and async methods in completions and chat completions APIs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| portkey_ai/api_resources/apis/complete.py | Fixed extra_body parameter handling in stream_create and normal_create methods (sync + async) |
| portkey_ai/api_resources/apis/chat_complete.py | Fixed extra_body parameter handling in stream_create and normal_create methods (sync + async) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| extra_headers = kwargs.pop("extra_headers", None) | ||
| extra_query = kwargs.pop("extra_query", None) | ||
| timeout = kwargs.pop("timeout", None) | ||
| user_extra_body = kwargs.pop("extra_body", None) or {} |
Copilot
AI
Nov 25, 2025
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.
Using or {} after pop() will replace {} (falsy empty dict) with {}, but more critically, it will replace None with {} which masks the distinction. If the intent is to treat missing extra_body differently from explicit extra_body=None, this logic is incorrect. If extra_body={} is passed explicitly, it becomes falsy and gets replaced with a new empty dict. Consider using if user_extra_body is None: user_extra_body = {} or kwargs.pop('extra_body', {}) instead.
| user_extra_body = kwargs.pop("extra_body", None) or {} | |
| user_extra_body = kwargs.pop("extra_body", None) | |
| if user_extra_body is None: | |
| user_extra_body = {} |
| extra_body=kwargs, | ||
| extra_headers=extra_headers, | ||
| extra_query=extra_query, | ||
| extra_body=merged_extra_body if merged_extra_body else None, |
Copilot
AI
Nov 25, 2025
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.
The conditional if merged_extra_body else None evaluates to None when merged_extra_body is an empty dict {}, which is falsy in Python. This means if both user_extra_body and kwargs are empty, extra_body=None is passed instead of extra_body={}. Depending on the OpenAI SDK's handling, this could behave differently. Consider using extra_body=merged_extra_body or None or simply extra_body=merged_extra_body if empty dicts are acceptable.
| extra_body=merged_extra_body if merged_extra_body else None, | |
| extra_body=merged_extra_body, |
| extra_headers = kwargs.pop("extra_headers", None) | ||
| extra_query = kwargs.pop("extra_query", None) | ||
| timeout = kwargs.pop("timeout", None) | ||
| user_extra_body = kwargs.pop("extra_body", None) or {} |
Copilot
AI
Nov 25, 2025
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.
Using or {} after pop() will replace {} (falsy empty dict) with {}, but more critically, it will replace None with {} which masks the distinction. If the intent is to treat missing extra_body differently from explicit extra_body=None, this logic is incorrect. If extra_body={} is passed explicitly, it becomes falsy and gets replaced with a new empty dict. Consider using if user_extra_body is None: user_extra_body = {} or kwargs.pop('extra_body', {}) instead.
| user_extra_body = kwargs.pop("extra_body", None) or {} | |
| user_extra_body = kwargs.pop("extra_body", {}) |
| extra_headers=extra_headers, | ||
| extra_body=kwargs, | ||
| extra_query=extra_query, | ||
| extra_body=merged_extra_body if merged_extra_body else None, |
Copilot
AI
Nov 25, 2025
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.
The conditional if merged_extra_body else None evaluates to None when merged_extra_body is an empty dict {}, which is falsy in Python. This means if both user_extra_body and kwargs are empty, extra_body=None is passed instead of extra_body={}. Depending on the OpenAI SDK's handling, this could behave differently. Consider using extra_body=merged_extra_body or None or simply extra_body=merged_extra_body if empty dicts are acceptable.
| extra_body=merged_extra_body if merged_extra_body else None, | |
| extra_body=merged_extra_body, |
Remove unnecessary conditional when passing extra_body to OpenAI SDK. Empty dict and None are both handled correctly by the SDK.
Responses to Review CommentsOn keeping
|
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Problem
The
extra_bodyparameter was being passed incorrectly to the OpenAI SDK inchat.completions.create()andcompletions.create(). This caused the entire kwargs dict (including theextra_bodykey itself) to be merged into the request body.Before this fix:
After this fix:
Root Cause
In the internal
stream_createandnormal_createmethods, the code was passingextra_body=kwargsdirectly to the OpenAI SDK. When a user passedextra_body={'a': 'b'}, it became part of kwargs as{'extra_body': {'a': 'b'}}, and this entire dict was passed to OpenAI'sextra_bodyparameter.Solution
The fix extracts
extra_bodyfrom kwargs, merges its contents with remaining kwargs (excluding special params likeextra_headers,extra_query,timeout), and passes the merged result to the OpenAI SDK'sextra_bodyparameter. This matches the expected OpenAI SDK behavior.This fix changes the behavior of the
extra_bodyparameter to match the OpenAI SDK convention:extra_body={'a': 'b'}{"extra_body": {"a": "b"}}{"a": "b"}custom_field='value'(direct kwarg){"custom_field": "value"}{"custom_field": "value"}(unchanged)Who might be affected:
extra_bodyand unknowingly relying on the buggy behavior where"extra_body"appeared as a literal key in the request bodyextra_bodyas a field nameWho benefits:
extra_bodyto work like the OpenAI SDK (merging contents at the top level)thinkingfor ClaudeWorkaround users are unaffected:
custom_field='value') will see no changeFiles Changed
portkey_ai/api_resources/apis/chat_complete.py: Fixedstream_createandnormal_create(sync + async)portkey_ai/api_resources/apis/complete.py: Fixedstream_createandnormal_create(sync + async)Note
This same bug pattern (
extra_body=kwargs) exists in ~120 other places across the codebase (assistants, threads, images, audio, etc.). This PR fixes the core completions endpoints. Additional PRs can address the other endpoints if needed.