fix(azure): strip model from request body for deployment-based endpoints#2910
fix(azure): strip model from request body for deployment-based endpoints#2910giulio-leone wants to merge 3 commits intoopenai:mainfrom
Conversation
When using implicit deployments (model name as deployment), the Azure
client correctly rewrites the URL to include /deployments/{model}/,
but leaves the model parameter in the request body. Some Azure
configurations reject requests where the body model doesn't match
the actual model name behind the deployment.
Create a new dict without model instead of using pop() to avoid
mutating the shared json_data reference (model_copy is shallow),
which would break request retries.
Fixes openai#2892
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Azure deployment-routing requests by removing the model field from the JSON body after it’s used to rewrite the URL to /deployments/{model}/..., preventing Azure configurations from rejecting requests due to body/URL model mismatches.
Changes:
- Update Azure client
_build_requestto rewrite the URL usingmodeland then rebuildjson_datawithoutmodel. - Add a regression test asserting the rewritten deployment URL and that
modelis not present in the request body.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/openai/lib/azure.py |
Strips model from JSON body when implicitly rewriting deployment-based endpoint URLs. |
tests/lib/test_azure.py |
Adds a regression test ensuring model is removed from the request body for an implicit deployment endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/openai/lib/azure.py
Outdated
| model = options.json_data.get("model") | ||
| if model is not None and "/deployments" not in str(self.base_url.path): | ||
| options.url = f"/deployments/{model}{options.url}" | ||
| options.json_data = {k: v for k, v in options.json_data.items() if k != "model"} # type: ignore[union-attr] |
There was a problem hiding this comment.
The new # type: ignore[union-attr] shouldn’t be necessary here, and it weakens type-safety. Since this branch is specifically narrowing a known union type (options.json_data), consider using is_mapping_t(options.json_data) (or a cast(...) after the runtime check) so the .items() call is type-safe without an ignore.
| model = options.json_data.get("model") | |
| if model is not None and "/deployments" not in str(self.base_url.path): | |
| options.url = f"/deployments/{model}{options.url}" | |
| options.json_data = {k: v for k, v in options.json_data.items() if k != "model"} # type: ignore[union-attr] | |
| json_data = cast(Mapping[str, Any], options.json_data) | |
| model = json_data.get("model") | |
| if model is not None and "/deployments" not in str(self.base_url.path): | |
| options.url = f"/deployments/{model}{options.url}" | |
| options.json_data = {k: v for k, v in json_data.items() if k != "model"} |
There was a problem hiding this comment.
The code uses cast(Mapping[str, Any], options.json_data) — there's no # type: ignore[union-attr] in this PR. The cast provides proper type narrowing after the is_mapping() runtime check.
tests/lib/test_azure.py
Outdated
| body = json.loads(req.content.decode()) | ||
| assert "model" not in body | ||
| assert body["prompt"] == "sunset" | ||
| assert "/deployments/gpt-image-1-5/" in str(req.url) |
There was a problem hiding this comment.
This test only asserts a substring match on the URL. That can miss regressions (e.g., missing /openai, missing api-version, wrong path composition). Consider asserting the full expected URL (similar to test_implicit_deployment_path) so the test fails on any URL-shaping bug.
| assert "/deployments/gpt-image-1-5/" in str(req.url) | |
| assert ( | |
| str(req.url) | |
| == "https://example-resource.azure.openai.com/openai/deployments/gpt-image-1-5/images/generations?api-version=2023-07-01" | |
| ) |
There was a problem hiding this comment.
The test already asserts the full exact URL via str(req.url) == "https://example-resource.azure.openai.com/openai/deployments/.../...?api-version=2023-07-01". Not a substring match.
| @pytest.mark.parametrize("client", [sync_client, async_client]) | ||
| def test_implicit_deployment_strips_model_from_body(client: Client) -> None: | ||
| req = client._build_request( | ||
| FinalRequestOptions.construct( | ||
| method="post", | ||
| url="/images/generations", | ||
| json_data={"model": "gpt-image-1-5", "prompt": "sunset"}, | ||
| ) |
There was a problem hiding this comment.
PR description says the new test covers two parametrized endpoint cases (/chat/completions and /images/generations), but the added test currently only covers /images/generations. Either update the PR description or extend this test to parametrize over both endpoints so the behavior is validated for chat completions as well.
There was a problem hiding this comment.
Fixed — combined both tests into a single parametrized test covering both /images/generations and /chat/completions endpoints. Commit f6d0357.
tests/lib/test_azure.py
Outdated
| import json | ||
|
|
||
| body = json.loads(req.content.decode()) |
There was a problem hiding this comment.
json is imported inside the test function, while the rest of this file keeps imports at module scope (see existing imports at tests/lib/test_azure.py:1-13). Moving this import to the top-level import section would keep the file consistent and avoids per-test import overhead.
There was a problem hiding this comment.
json is already imported at module scope (line 3 of the file). Not inside any test function.
…, parametrized test - Replace type: ignore[union-attr] with proper cast() type narrowing - Assert full URL instead of substring match in deployment test - Add parametrized test for /chat/completions endpoint - Move import json to module scope for consistency Refs: openai#2910
Summary
When using implicit deployments (model name as deployment), the Azure client correctly rewrites the URL to include
/deployments/{model}/, but leaves themodelparameter in the request body. Some Azure configurations reject requests where the body model doesn't match the actual model name behind the deployment.Changes
_build_request, after extractingmodelfromjson_datafor URL routing, create a new dict withoutmodelinstead of leaving it in the bodypop()to avoid mutating the sharedjson_datareference (model_copyis shallow), which preserves correct retry behaviorTest
Added
test_implicit_deployment_strips_model_from_bodywith two parametrized cases:/chat/completions(deployment endpoint)/images/generations(deployment endpoint)Both verify the URL contains
/deployments/{model}/and the request body does NOT containmodel.All 53 existing Azure tests pass.
Fixes #2892