Repository navigation
Python: Exclude ClassVar annotations from generated JSON schemas - #14528
Pushpak Siva Sai (Pushpak731) wants to merge 3 commits into
Conversation
KernelJsonSchemaBuilder.build_model_schema builds the instance schema
from get_type_hints, which includes ClassVar annotations, so a class
constant became a required object-valued property. Pydantic excludes
ClassVars from model fields and serialized instances, so the generated
schema rejected valid payloads (a serialized {'value': 1} failed
validation because 'marker' was required). The builder now skips
ClassVar-annotated hints (both the parameterized and bare forms,
including inherited ones), for KernelBaseModel subclasses, plain
annotated classes and instances, covering KernelParameterMetadata,
generated tool definitions and structured_output schemas.
Fixes microsoft#14527
GitHub Actions
Active
|
@microsoft-github-policy-service agree |
GitHub Actions
Active
semantic-kernel-automation
Bot
added
the
python
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
Community review. This does not clear the merge gate, it is one reader's check.
I reproduced the bug and checked the fix by running code. I loaded the installed semantic_kernel/schema/kernel_json_schema_builder.py (no ClassVar handling in it), then built a copy with your two added lines applied by string patch, and compared KernelJsonSchemaBuilder.build output for three classes.
class M(KernelBaseModel): marker: ClassVar[str] = "c"; value: int- current code: properties
marker: {"type": "object"}andvalue, required["marker", "value"] - with the patch: only
value, required["value"] M(value=1).model_dump()is{"value": 1}andM.model_fieldsis["value"], so the current schema demands a field the model never serializes, and it also types it wrongly asobject.
- current code: properties
Annotated[ClassVar[int], "x"]on a KernelBaseModel: same result, the patch drops it.- A plain (non-pydantic) class with a string annotation
"ClassVar[dict]": also dropped, becauseget_type_hintsresolves the string before the check.
The check field_type is ClassVar or get_origin(field_type) is ClassVar covers both the bare ClassVar and the subscripted form, which are the two shapes get_type_hints returns. It also fits with the loop: nothing after the continue depends on the skipped field, so required and properties stay consistent.
Notes:
- I did not run the repo's test file or the full suite, only the builder on the three classes above. The new tests (inherited ClassVar, structured output with
additionalProperties, via-instance) read correctly to me, but I did not execute them. - I did not check
typing_extensions.ClassVaron older Pythons. It is the same object astyping.ClassVaron current versions, so I expect it to work, but that is unverified. - Nit: the ten new tests overlap a lot. Two or three would cover the same branches (bare, subscripted, inherited).
Approving from my side.
GitHub Actions
Active
|
PRABHU KIRAN VANDRANKI (@VANDRANKI) thanks again for the approval! The branch had fallen behind main, so I merged main into it (46721b2 — no code changes beyond the merge; our ClassVar fix is untouched and main's new commits are unrelated). Could you re-confirm the approval on the updated head whenever convenient? Happy to make any further changes if the merge surfaced anything. |
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
Community re-review on head 46721b2. Still a community review, it does not clear the merge gate.
What I checked on the new head:
- The merge changes nothing in the PR.
git diff cc8a15fa3 46721b2(cc8a15f is the current main tip) is only the two files,kernel_json_schema_builder.py(+6/-1) andtest_schema_builder.py. Both files are byte-identical to the previous head 7139056 (empty diff on each). - I ran
tests/unit/schemaon the PR head: 45 passed. With the same test file against main's builder: 9 failed, 36 passed, so the ClassVar tests do guard the fix.tests/unit/functions,tests/unit/kernelandtests/unit/schematogether: 332 passed. - I did not run the full suite or the other connector test folders. At the time of checking, only
license/claandadd_labelhad reported on this head, so the Python test jobs had not run on it yet.
My earlier approval stands for this head.
Fixes #14527
Description
KernelJsonSchemaBuilder.build_model_schemabuilds the schema fromget_type_hints, which includesClassVarannotations, so a class constant became a required object-valued property in the generated schema. Pydantic excludesClassVars frommodel_fieldsand from serialized instances, so the resulting schema rejected valid serialized payloads ({'value': 1}failed becausemarkerwas required). The same wrong schema reachedKernelParameterMetadata, generated function-call tool definitions andstructured_output=Trueschemas.The fix skips
ClassVar-annotated hints before the required/property loop — covering the parameterized form (ClassVar[str]), the bare form (ClassVar, whoseget_originisNone), and inheritedClassVars, forKernelBaseModelsubclasses, plain annotated classes and model instances alike.Contribution Checklist
tests/unit/schema/(45 passed) andtests/unit/functions/test_kernel_parameter_metadata.py(7 passed) — 10 new regression tests cover pydantic models, bare/inheritedClassVar, plain classes, instances,structured_output, descriptions, optionals and the serialized-payload contractruff checkandruff format --checkare clean on both changed filesmodel_dump)