fix(viewer): support cbor2 6.x deserialization (frozendict + tuple)#2390
fix(viewer): support cbor2 6.x deserialization (frozendict + tuple)#2390Bahtya wants to merge 2 commits intonewton-physics:mainfrom
Conversation
cbor2 6.0 introduces two breaking changes in deserialization: - Nested dicts return as `cbor2.frozendict` (a Mapping, but no longer a `dict` subclass) - Arrays return as `tuple` instead of `list` The `isinstance(data, dict)` guards in `deserialize()`, `transfer_to_model()`, and `_resolve_cache_refs()` silently short-circuit on `frozendict`, causing recorder round-trip tests to fail. Fix all three sites by switching to `isinstance(x, Mapping)` (already imported from `collections.abc`). Additionally update `_resolve_cache_refs` to treat `tuple` the same as `list` when iterating, and always return a plain `list` so callers aren't surprised by the cbor2 change. Also removes the `<6` upper bound on the cbor2 dependency that was added as a short-term workaround in newton-physics#2373, and updates the install hint in the ImportError message. Fixes newton-physics#2375 Bahtya
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated viewer deserialization to accept generic Mapping types (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/viewer/viewer_file.py`:
- Around line 1014-1016: The current _resolve_cache_refs post-pass
unconditionally converts tuples to lists; change it to preserve real tuples by
branching on type: if obj is a list, return a new list with _resolve_cache_refs
applied to each element; if obj is a tuple, return a tuple constructed from
applying _resolve_cache_refs to each element (so genuine tuple-valued fields
keep their type). Keep the existing behavior for other iterables unchanged and
reference the _resolve_cache_refs function (and the surrounding deserialize
flow) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa50d953-209f-471d-997c-cdf2f1e10542
📒 Files selected for processing (2)
newton/_src/viewer/viewer_file.pypyproject.toml
The previous implementation converted both lists and tuples to lists in the post-pass, causing genuine tuple-valued fields to round-trip as a different type. Now lists resolve to lists and tuples resolve to tuples, preserving the original container type for all non-cache-ref fields. Bahtya
|
Thanks for the review! I've addressed the CodeRabbit feedback: Preserve real tuples in |
|
Friendly ping 👋 This PR adds support for cbor2 6.x deserialization by handling frozendict and other type changes. CodeRabbit review found no issues. Would appreciate a review. |
Summary
Fixes #2375
cbor2 6.0 introduces two breaking changes:
cbor2.frozendict— aMapping, but not adictsubclasstupleinstead oflistThe
isinstance(data, dict)guards inviewer_file.pysilently short-circuit onfrozendict, so deserialization does nothing and recorder round-trip tests fail.Changes
newton/_src/viewer/viewer_file.pyisinstance(x, dict)→isinstance(x, Mapping)indeserialize(),transfer_to_model(), the warp callback, and_resolve_cache_refs(). Also update_resolve_cache_refsto handletuplethe same aslist(cbor2 6.x arrays), always returning a plainlist.pyproject.toml<6upper bound added as a workaround in #2373.Test plan
pytest newton/tests/ -k recorderpasses withcbor2>=5.7.0,<6pytest newton/tests/ -k recorderpasses withcbor2>=6.0(once 6.0 is released)pip install 'cbor2>=5.7.0'installs without version conflictNotes
Mappingis already imported fromcollections.abcat the top of the file, so no new imports are needed.Bahtya
Summary by CodeRabbit
Bug Fixes
Improvements