fix(dpmodel): preserve model dictionaries during serialization#5788
fix(dpmodel): preserve model dictionaries during serialization#5788njzjz wants to merge 1 commit into
Conversation
Copy only the dict and list containers that serialization traversal rewrites. This keeps caller-owned model dictionaries unchanged without duplicating large array buffers or requiring variable objects such as h5py datasets to support deepcopy. Add YAML and native-format regressions for nested input preservation, plus an HDF5 dataset case that protects the non-copyable variable path. Fixes deepmodeling#5639. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSerialization mutation isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5788 +/- ##
==========================================
- Coverage 79.85% 79.58% -0.27%
==========================================
Files 1022 1022
Lines 117351 117358 +7
Branches 4313 4311 -2
==========================================
- Hits 93706 93401 -305
- Misses 22101 22409 +308
- Partials 1544 1548 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Correct and minimal. save_dp_model did a shallow model_dict.copy(), but traverse_model_dict reassigns nested dict/list entries in place, so serialization mutated the caller's dict (arrays replaced by YAML/HDF5 references). _copy_model_containers rebuilds only the container tree and shares the variable objects, which is safe because the save callbacks read variables (data=x, x.dtype, x.tolist()) and create new @is_variable dicts rather than mutating inputs. The regression tests genuinely fail pre-fix -- passing a caller-owned dict directly and asserting equality/identity -- and the h5py.Dataset test additionally prevents a naive full deepcopy from being the fix, since Datasets aren't deep-copyable. Good catch on why the existing tests missed it (they fed in a deepcopy). LGTM.
Summary
Why existing tests missed this
The existing save/load tests pass deepcopy(self.model_dict) into save_dp_model() and then validate only the serialized round trip. That disposable copy hides mutations of the object supplied by a real caller, so the shallow-copy bug was never observed.
The new tests pass a caller-owned nested structure directly, assert that its containers and variables remain unchanged, and cover both serialization implementations. The HDF5 dataset case also prevents a memory-heavy full deepcopy from becoming the fix.
Validation
Closes #5639.
Coding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit