Repository navigation
Python: fix kernel_function decorator mutating shared Annotated metadata dicts - #14442
Manohar Paturi (ManoharPaturi) wants to merge 3 commits into
Conversation
…ata dicts
_parse_parameter() called meta.pop("description") on dict metadata used
in typing.Annotated, mutating the user-provided dict. When the same
metadata dict is reused across multiple kernel functions (shared
parameter docs), every function after the first silently lost its
parameter description, and the dict itself was permanently modified.
Use meta.get() instead of meta.pop() and filter the description key out
when applying the remaining metadata keys, so semantics are unchanged
but the caller's dict is left untouched.
GitHub Actions
Failure
GitHub Actions
Failure
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
The original mutation issue is fixed on main. The inline comment identifies an additional behavior change that is not described in this PR. Its old label/review/gate runs also need fresh validation if this is retained as a separate fix.
| if "description" not in ret and (description := meta.get("description", None)): | ||
| ret["description"] = description | ||
| ret.update(meta) | ||
| ret.update({key: value for key, value in meta.items() if key != "description"}) |
There was a problem hiding this comment.
The mutation bug is already fixed on main by #14496 using a local dict copy. This version also changes description precedence: Annotated[int, "earlier string", {"description": "later dictionary"}] currently produces "later dictionary", but this filtering produces "earlier string". I reproduced both results. Is that additional behavior change intentional? Please clarify the remaining bug and cover the precedence rule before rebasing; otherwise the original shared-metadata fix is already delivered.
GitHub Actions
Inactive
|
The precedence change was not intentional, and you were right to flag it: filtering the description key out of the update silently flipped which entry wins. The branch now carries main's implementation from #14496 unchanged and reduces to two regression tests: the precedence rule you reproduced, where the later dict description overrides the earlier string while the caller-owned dict stays untouched, and the shared metadata contract, where one dict decorates two functions and keeps working. So the PR no longer changes behavior at all, it only pins the rules. |
semantic-kernel-automation
Bot
added
the
python
Fixes #14441
Main already fixed the shared metadata mutation in #14496 with a local dict copy. This PR now changes no implementation; it pins the two contracts around that fix with regression tests:
A later dict metadata entry overrides an earlier string description (
Annotated[int, "earlier string", {"description": "later dictionary"}]yields the dict description), with the caller-owned dict left untouched by decoration.One shared metadata dict can decorate multiple kernel functions and keeps its description for each.
Both tests target current main behavior and pass against it.