Python: recursively serialize nested container values - #7790
Python: recursively serialize nested container values#7790wang ke (CoffeeDrivenCoder) wants to merge 3 commits into
Conversation
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Error
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
|
@microsoft-github-policy-service agree |
GitHub Actions
Active
Esan (EsanRAHIMI)
left a comment
Esan (EsanRAHIMI)
left a comment
There was a problem hiding this comment.
Thanks for the focused change—the sentinel-based recursion preserves the existing skip behavior cleanly, and the nested protocol regression coverage exercises the original failure well.
One compatibility edge is worth confirming or pinning with tests: moving the (datetime, date, time) branch into the shared helper expands conversion beyond dictionary values. Top-level date/time attributes and date/time values inside lists were previously skipped as non-serializable, but are now emitted as strings. That may be a desirable consistency fix, but it is an observable behavior change beyond nested protocol serialization. Could you either confirm it is intentional and add coverage for those locations, or restrict the conversion to the prior dictionary-value context?
Keep date and time conversion scoped to dictionary values while retaining recursive protocol and container serialization.
GitHub Actions
Active
|
Esan (@EsanRAHIMI) You are right; the broader date/time conversion was not intentional. Addressed in 60d9e33 by moving |
Esan (EsanRAHIMI)
left a comment
Esan (EsanRAHIMI)
left a comment
There was a problem hiding this comment.
The follow-up restores the previous date/time serialization boundary while keeping the nested container recursion intact. The regression test now covers datetime, date, and time across top-level attributes, list items, and dictionary values. This addresses my compatibility concern—thanks!
| attribute_name=attribute_name, | ||
| ) | ||
| if serialized is not _SKIP_SERIALIZATION: | ||
| serialized_dict[str(raw_key)] = serialized |
There was a problem hiding this comment.
Could we pin the intended behavior for nested non-string keys? Recursion changes nested {True: ..., None: ...} keys to "True" / "None"; previously to_dict() retained those keys and to_json() produced "true" / "null". It may be best to preserve the old output or add a test for the new normalization.
There was a problem hiding this comment.
Addressed in 8f3b8be. The recursive helper now stringifies keys only for dictionaries that are direct object attributes, matching the previous to_dict() boundary. Keys in deeper dictionaries retain their original types, so to_json() again lets json.dumps() render True / None as "true" / "null". The regression test pins both the direct-key normalization and nested-key preservation.
| if isinstance(value, list): | ||
| serialized_list: list[Any] = [] | ||
| for item in cast(list[Any], value): | ||
| serialized = _serialize_value( |
There was a problem hiding this comment.
Should recursive containers get cycle detection? A self-referential list or dict now raises RecursionError during to_dict(). Before this change, to_dict() returned and to_json() raised the controlled ValueError: Circular reference detected. I think it would be good to preserve a predictable failure and add a regression test before recursive user state reaches this path.
There was a problem hiding this comment.
Addressed in 8f3b8be. Recursive list and dictionary traversal now tracks container IDs on the active recursion path and raises ValueError("Circular reference detected") on re-entry. IDs are removed in finally, so repeated acyclic references remain valid. Regression tests cover both self-referential lists and dictionaries. All 507 relevant serialization/session/type/workflow tests, Ruff lint/format, source Pyright, and test Pyright pass locally.
GitHub Actions
Active
Motivation & Context
SerializationMixin.to_dict()handled protocol objects only when they were direct list items or dictionary values. Protocol objects inside combinations such asdict -> list -> dictremained as Python objects, causing the correspondingto_json()call to fail withTypeError.Description & Review Guide
Related Issue
Fixes #7788
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.