Repository navigation
fix(serialization): support set and frozenset generic containers in JsonDataConverter - #310
Hardik K (hardikkaurani) wants to merge 3 commits into
Conversation
|
I've opened this PR to add native support for Previously, serializing objects or dataclasses containing sets raised This PR addresses both issues by adding deterministic serialization and recursive coercion for generic set containers, along with comprehensive test coverage. Whenever you have a chance, could you please review it? Thank you for your time! |
andystaples
left a comment
There was a problem hiding this comment.
Requesting changes for the replay-compatibility and deterministic-serialization issues noted inline.
Existing persisted orchestration history is a compatibility contract. An SDK upgrade must not change how an already-running instance reconstructs historical payloads in a way that changes its orchestration decisions. Keeping the JSON readable is necessary, but it is not enough: the same history must still replay with the same behavior. If new reconstruction semantics would change that behavior, they need an explicit opt-in or versioned compatibility path that preserves the old behavior for existing instances, plus regression coverage using pre-change histories.
The legacy marker handling remains intact, and histories that do not exercise the newly recognized set annotations appear unaffected. However, this change is not universally safe for in-flight orchestration upgrades as written. The set encoder also needs deterministic ordering for supported custom objects rather than falling back to set iteration order.
Please also fix the failing pyright CI check. The current typecheck_core-3.10 log reports five errors in the added code: partially unknown return/argument types at serialization.py:324 and :326, and an unknown item type at :489. Please resolve the collection/element typing and get this check passing.
|
andystaples just doing ! |
|
Thanks. The PR was closed earlier, and GH records show it was me, but I didn't do it deliberately, so I re-opened it |
…tion, and pyright typing - Preserve replay compatibility by gating generic set reconstruction behind enable_generic_sets=False default on JsonDataConverter - Ensure deterministic set serialization with fallback sort key on recursive encoded values, qualname, and repr - Retain primitive set element annotations in type discovery when set reconstruction is enabled - Resolve strict pyright errors and update tests
|
Hi andystaples, thank you for the detailed review and feedback! All requested changes and reviewer findings have been addressed: 1. Replay Compatibility & Opt-in Generic Set Reconstruction
2. Deterministic Set Serialization Across Processes & Hash Seeds
3. Type Discovery & Activity Execution Support
4. Pyright & Test Suite Verification
|
andystaples
left a comment
There was a problem hiding this comment.
The default-off reconstruction flag addresses the original existing-history replay regression, and primitive-set discovery now works when opted in. There is still one blocking determinism case: nested frozensets take the native sorting path without raising, but that comparison does not provide a total order. Details and a reproduction are inline.
| if isinstance(o, (set, frozenset, AbstractSet)): | ||
| items: AbstractSet[Any] = cast("AbstractSet[Any]", o) | ||
| try: | ||
| return sorted(items) |
There was a problem hiding this comment.
[P1] Use a deterministic total ordering even when native sorting succeeds
The new fallback fixes the non-orderable dataclass case, but sorted(items) can succeed without producing a deterministic order. Frozensets compare by subset relationships, not a total order, so disjoint frozensets keep hash-dependent iteration order and never reach the fallback.
For example:
JsonDataConverter().serialize({
frozenset({"alpha"}),
frozenset({"beta"}),
frozenset({"gamma"}),
})In separate processes, I get:
PYTHONHASHSEED=1:[["alpha"], ["gamma"], ["beta"]]PYTHONHASHSEED=2:[["gamma"], ["alpha"], ["beta"]]
This still violates the deterministic-serialization contract across worker processes and replay. Please use a deterministic total ordering based on recursively encoded values rather than relying on native sorting merely because it does not raise, and extend the cross-process regression to nested frozensets.
There was a problem hiding this comment.
Thank you for identifying this edge case, andystaples.
I have updated the set serialization ordering in durabletask/serialization.py to use a dedicated deterministic total-ordering sort key (_deterministic_set_sort_key) based on recursively encoded values rather than relying on native sorting to succeed. This ensures disjoint and nested frozenset values establish a deterministic total order across processes and replays, while preserving numerical ordering for numeric sets.
I also updated and extended test_set_serialization_deterministic_across_hash_seeds in tests/durabletask/test_serialization.py to exercise disjoint frozensets (Case A), nested frozensets across multiple levels (Case B), and unorderable frozen dataclasses (Case C) across multiple PYTHONHASHSEED values (0, 1, 2, 42, 999) using portable repository-relative path resolution.
Validation Results
pytest tests/durabletask/test_serialization.py: 56 passedpytest tests/durabletask/test_data_converter.py: 35 passedpytest tests/durabletask/test_data_converter_roundtrip.py: 48 passedpytest tests/durabletask/test_type_discovery.py: 37 passednox -s lint: passed (flake8 clean, 0 issues)- Strict
pyright durabletask/serialization.py: 0 errors, 0 warnings nox -s functions_unit-3.13: 406 passed
The fix is pushed to fix/serialization-generic-sets in commit 6c924e5.
|
Hi andystaples, Thank you for the detailed review. I've addressed the remaining deterministic-serialization issue by using recursively encoded ordering keys for unordered set elements, including nested frozenset values. The implementation no longer relies on native sorting merely because it completes without raising an exception. I've added cross-process regression coverage to verify that the serialized payload remains identical across multiple Validation
The changes have been pushed to the existing PR branch |


Summary
set,frozenset, andcollections.abc.Setin_encode_custom_object, with deterministic ordering for sortable elements._coerce_genericinJsonDataConverterto recursively reconstruct genericset[T]andfrozenset[T]containers, including dataclass fields.can_reconstructto validate element types for generic set containers.CHANGELOG.mdunder theUnreleasedsection.Validation
test_serialization.pyandtest_data_converter.pypassed successfully.