fix(backends): strict-ify OpenAI response schemas for OpenAI-compatible providers - #1493
fix(backends): strict-ify OpenAI response schemas for OpenAI-compatible providers#1493abhiojha8 wants to merge 2 commits into
Conversation
…le providers OpenAI structured outputs (and OpenAI-compatible proxies that terminate on the OpenAI platform, e.g. OpenRouter routing to an OpenAI model) require `additionalProperties: false` on every object and reject `$ref` entries, so schemas must be self-contained. Pydantic emits `$defs`/`$ref` for nested models, and the `@generative` wrapper references the result type via `$ref`, so any such call failed with a 400 regardless of server-type detection. Remove the `_ServerType.OPENAI` gate from the `response_format` block and apply the same inlined, fully-patched schema to the raw completions path (`structured_outputs`/`guided_json` for vLLM-style backends). Closes generative-computing#1491 Assisted-by: Reasonix Signed-off-by: Abhi Ojha <Abhi.Ojha@ibm.com>
AngeloDanducci
left a comment
There was a problem hiding this comment.
Will let @jakelorocco weigh in since he was involve in the original issue.
Thanks for the contribution! One small note from me.
| def _patch_object(obj: dict[str, Any]) -> None: | ||
| if obj.get("type") == "object": | ||
| obj["additionalProperties"] = False | ||
| props = obj.get("properties") | ||
| if isinstance(props, dict): | ||
| for prop_schema in props.values(): | ||
| if isinstance(prop_schema, dict): | ||
| _patch_object(prop_schema) | ||
| items = obj.get("items") | ||
| if isinstance(items, dict): | ||
| _patch_object(items) | ||
| for key in ("anyOf", "oneOf", "allOf"): | ||
| for branch in obj.get(key, []): | ||
| if isinstance(branch, dict): | ||
| _patch_object(branch) |
There was a problem hiding this comment.
I think we should only set to false when it's not already a schema dict. Should still solve the original problem.
| def _patch_object(obj: dict[str, Any]) -> None: | |
| if obj.get("type") == "object": | |
| obj["additionalProperties"] = False | |
| props = obj.get("properties") | |
| if isinstance(props, dict): | |
| for prop_schema in props.values(): | |
| if isinstance(prop_schema, dict): | |
| _patch_object(prop_schema) | |
| items = obj.get("items") | |
| if isinstance(items, dict): | |
| _patch_object(items) | |
| for key in ("anyOf", "oneOf", "allOf"): | |
| for branch in obj.get(key, []): | |
| if isinstance(branch, dict): | |
| _patch_object(branch) | |
| def _patch_object(obj: dict[str, Any]) -> None: | |
| add_props = obj.get("additionalProperties") | |
| if obj.get("type") == "object" and not isinstance(add_props, dict): | |
| # Only assert a closed object when the value schema isn't itself a | |
| # constraint (e.g. dict[str, Model] emits additionalProperties as a | |
| # schema — overwriting it with False would drop the value type). | |
| obj["additionalProperties"] = False | |
| elif isinstance(add_props, dict): | |
| _patch_object(add_props) | |
| props = obj.get("properties") | |
| if isinstance(props, dict): | |
| for prop_schema in props.values(): | |
| if isinstance(prop_schema, dict): | |
| _patch_object(prop_schema) | |
| items = obj.get("items") | |
| if isinstance(items, dict): | |
| _patch_object(items) | |
| for key in ("anyOf", "oneOf", "allOf"): | |
| for branch in obj.get(key, []): | |
| if isinstance(branch, dict): | |
| _patch_object(branch) |
jakelorocco
left a comment
There was a problem hiding this comment.
@abhiojha8, thanks for opening the PR. I tested the change against a vllm hosted model and saw no issues with our existing tests.
I think these changes look good, but I agree with @AngeloDanducci's comments and commented some tests we could add to ensure we don't hit regressions here.
Please let us know if you agree / disagree with the suggestions!
|
|
||
|
|
There was a problem hiding this comment.
We should add tests to capture the scenario @AngeloDanducci commented on:
class _ExtractUserMapResponse(BaseModel):
"""Wrapper with a dict-of-model field.
Pydantic emits the value type as `additionalProperties: {schema}` (not
`false`) on the map object, which is the case that must not be clobbered.
"""
users: dict[str, _NestedProfile]
def test_make_response_schema_openai_strict_preserves_dict_value_schema():
"""A `dict[str, Model]` value-type schema is preserved, not overwritten.
Regression guard for the `additionalProperties`-as-schema case: pydantic
emits `additionalProperties: {<value schema>}` for `dict[str, Model]`
fields. Overwriting that with `False` would silently drop the value type
(turning "string keys -> Model" into "no extra properties allowed"), so
the patcher must recurse into the value schema instead of clobbering it.
"""
schema = _make_response_schema_openai_strict(
_ExtractUserMapResponse.model_json_schema()
)
users = schema["properties"]["users"]
assert users["type"] == "object"
# The value-type schema must survive as a dict, NOT be replaced by False.
value_schema = users["additionalProperties"]
assert isinstance(value_schema, dict), (
"dict[str, Model] value type was clobbered by additionalProperties=False"
)
# The nested model reachable through the map is inlined and closed.
assert "$ref" not in value_schema
assert value_schema["type"] == "object"
assert value_schema["additionalProperties"] is False
assert set(value_schema["properties"]) == {"name", "age"}
def test_make_response_schema_openai_strict_patches_freeform_dict_scalar_values():
"""A `dict[str, scalar]` value-type schema (e.g. int) is preserved too."""
class _Counts(BaseModel):
counts: dict[str, int]
schema = _make_response_schema_openai_strict(_Counts.model_json_schema())
counts = schema["properties"]["counts"]
assert counts["type"] == "object"
# Scalar value type stays intact instead of being turned into False.
assert counts["additionalProperties"] == {"type": "integer"}
|
FYI @HendrikStrobelt. Can you try testing on this branch and see if it addresses your issue? |
…elds Address review feedback on generative-computing#1493: when pydantic emits a schema dict as `additionalProperties` (e.g. `dict[str, Model]`), recurse into it instead of overwriting it with `False`, which would silently drop the value type. Add the reviewer-suggested regression tests for `dict[str, Model]` and `dict[str, int]`. Assisted-by: Reasonix Signed-off-by: Abhi Ojha <Abhi.Ojha@ibm.com>
|
Thanks for the review — and thanks for testing against a real vLLM backend, glad the existing tests passed there. I agree with the
Local verification: |
Pull Request
Issue
Fixes #1491
Description
OpenAI's structured outputs require
additionalProperties: falseon every object and reject$refentries (schemas must be self-contained). Mellea only patched the top level ofresponse_formatschemas, and only whenbase_urlwas exactlyapi.openai.com. Any@generativecall routed through an OpenAI-compatible proxy (e.g. OpenRouter → OpenAI model) failed with400 invalid_json_schema— and because pydantic emits$defs/$reffor nested models, nested return types would fail on realapi.openai.comtoo.This PR removes the server-type gate and applies the fix universally for
OpenAIBackend:_make_response_schema_openai_strict()inlines$refs (reusing_recursively_inline_refsfrommellea/backends/tools.py) and recursively setsadditionalProperties: falseon every object.response_formatpath and the raw completions path (structured_outputs/guided_jsonfor vLLM-style backends), per maintainer guidance on@generativewith OpenAI-compatible proxies (e.g. OpenRouter) fails with 400: response_format schema missingadditionalProperties: false#1491.Verified empirically (see #1491): top-level-only patching is insufficient; OpenAI-platform endpoints require inlined + fully-patched schemas.
Testing
New tests in
test/backends/test_openai_unit.py:$refinlined, all objects patched), list-of-modelitems,anyOfbranchesresponse_formatand rawguided_json/structured_outputspayloads reaching the provider are inlined + fully patched (covers thestructured_outputsbranch via_use_structured_output_for_raw)Local verification:
uv run pytest test/backends/test_openai_unit.py→ 38 passedgpt-5.6-luna-pro) now returns typed output (previously 400); Google + Anthropic providers regression-checked, still workingruff format/ruff check,mypy,pre-commit(SPDX, ruff, mypy, codespell, DCO) all cleanNot run locally (no Ollama/vLLM on this machine): the full
-m "not qualitative"suite and a real vLLM backend — happy to have CI and/or maintainers run those (as offered on #1491).Attribution
AI-assisted (Reasonix harness; commit includes
Assisted-by: Reasonixtrailer). The complete diff was reviewed and approved by the human author before submission.Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.