test(azure): guard the gpt-5 reasoning_effort=none base_model gate at the SDK layer - #33615
test(azure): guard the gpt-5 reasoning_effort=none base_model gate at the SDK layer#33615mateo-berri wants to merge 1 commit into
Conversation
Greptile SummaryThis PR adds four mock-only unit tests to
Confidence Score: 4/5Safe to merge; pure test addition with no production code changes and all tests use mocks. Only change is adding four mock-based tests — no production code is touched. The tests are well-structured and the mock_response approach correctly short-circuits before any network call. The single observation is a mildly imprecise docstring on the negative test: gpt-5.6-sol-e2e actually does match gpt-5 detection (the 'gpt-5' in model substring check), so the gate fails at the capability-lookup step rather than at the detection step as the comment implies. The test still validates the correct contract, but a future contributor reading it cold could be misled about where exactly the guard fires. The negative test docstring in test_azure_base_model_routing.py (lines 185-194) is slightly imprecise about the mechanism that triggers the rejection.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/llms/azure/chat/test_azure_base_model_routing.py | Adds four mock-only tests pinning the GH #31243 fix: two in TestGetOptionalParamsWithBaseModel for the get_optional_params layer, and two in a new TestCompletionThreadsBaseModelIntoParamGate class for the litellm.completion layer. Docstring on the negative test is slightly imprecise about why the gate fails closed. |
Reviews (1): Last reviewed commit: "test(azure): guard the gpt-5 reasoning_e..." | Re-trigger Greptile
| def test_should_reject_reasoning_effort_none_for_custom_deployment_without_base_model( | ||
| self, | ||
| ): | ||
| """GH #31243: without base_model the 'none' gate fails closed for unknown deployment names.""" | ||
| with pytest.raises(litellm.UnsupportedParamsError): | ||
| get_optional_params( | ||
| model="gpt-5.6-sol-e2e", | ||
| custom_llm_provider="azure", | ||
| reasoning_effort="none", | ||
| ) |
There was a problem hiding this comment.
Misleading negative-test docstring
The docstring says "the 'none' gate fails closed for unknown deployment names", implying the model name bypasses gpt-5 detection. It doesn't — gpt-5.6-sol-e2e contains the literal "gpt-5" substring, so is_model_gpt_5_model returns True even without base_model. The UnsupportedParamsError is raised one step later: _supports_reasoning_effort_level("gpt-5.6-sol-e2e", "none") returns False because the custom deployment name is not registered in model_prices_and_context_window.json.
The test still validates the right contract (the gate must reject an unregistered deployment), but a reader might incorrectly conclude the guard operates at the detection step. A note like "name matches gpt-5 detection but the deployment is not registered to support none" would make the intent clearer for future contributors.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 24 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 24 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
Relevant issues
Guards #31243 at the SDK layer. The fix itself shipped in PR #28490; live e2e coverage of the same shape is in #33468
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
The guarded behavior is client-side SDK param mapping, so the before/after proof is the same real Azure OpenAI call made through
litellm.completionon the last release without the fix versus this branch. The deploymentgpt-5.6-sol-e2eis a real custom-namedgpt-5.6-soldeployment on the e2e suite Azure resource; the after run reaches Azure and costs real $Before, captured on
litellm==1.86.7from PyPI (the fix first shipped in v1.87.0; it entered release branches as a cherry-pick, sogit tag --containson the staging commit misleadingly points at v1.91.0):The error is raised before any network call, which is exactly why the e2e row in #33468 cannot see it: the proxy's config-file registration resolves capabilities via
base_modelon its own, so a live proxy passes this shape even on images whose SDK path is broken (verified on the v1.86.2 image)After, captured at commit 2df2c6c (same call, printing the response):
Type
✅ Test
Changes
Adds four tests to
tests/test_litellm/llms/azure/chat/test_azure_base_model_routing.py(the test file PR #28490 introduced) pinning the exact GH #31243 shape at the SDK layer:reasoning_effort='none'against a custom-named Azure deployment whose true model is supplied viabase_modelTwo tests pin
get_optional_params: withbase_model="azure/gpt-5.6-sol"the param survives mapping asreasoning_effort='none', and withoutbase_modelthe gate fails closed withUnsupportedParamsError, so the positive test cannot pass vacuously. Two tests pin thelitellm.completionkwarg plumbing inmain.pyusingmock_response, which short-circuits afterget_optional_paramsruns, so they exercise the real param-mapping path with no networkWhy this is needed on top of the e2e row in #33468: the proxy's startup registration of config-file models resolves capabilities through
base_modelindependently, which masks an SDK-layer regression from any proxy-level test (the v1.86.2 image passes the e2e shape while its own SDK raises). These unit tests are the only guard that fails on an SDK-layer revertKill power was verified by mutation: reverting
_azure_detection_model = base_model or modeltomodelinlitellm/utils.pyfails 6 tests in the file including the new ones, and nulling thebase_modelkwarg extraction inlitellm/main.pyfails only the new completion-level test, coverage no existing test providedFinal Attestation