fix(serialization): remove monty dependency - #979
Conversation
dosubot
Bot
added
size:L
Merging this PR will not alter performance
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the ChangesReplace
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant System
participant dpdata_serialization
participant FormatBackend
participant process_decoded
System->>dpdata_serialization: dumpfn or loadfn
dpdata_serialization->>FormatBackend: write or read JSON, YAML, or msgpack
FormatBackend-->>dpdata_serialization: serialized or parsed data
dpdata_serialization->>process_decoded: decode payload
process_decoded-->>System: reconstructed data
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
dpdata/serialization.py (1)
27-34: Add return type annotation to_open_text.The function lacks a return type hint, which may cause type checkers to infer imprecise union types for
gzip.open/bz2.openeven though all code paths return text I/O objects compatible withjson.dumpandjson.load.Proposed fix
-from typing import Any +from typing import Any, TextIO, cast @@ -def _open_text(filename: str | Path, mode: str): +def _open_text(filename: str | Path, mode: str) -> TextIO: path = str(filename) lower_path = path.lower() if lower_path.endswith((".gz", ".z")): - return gzip.open(path, mode, encoding="utf-8") + return cast(TextIO, gzip.open(path, mode, encoding="utf-8")) if lower_path.endswith(".bz2"): - return bz2.open(path, mode, encoding="utf-8") + return cast(TextIO, bz2.open(path, mode, encoding="utf-8")) return open(path, mode, encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dpdata/serialization.py` around lines 27 - 34, The _open_text function is missing a return type annotation which prevents type checkers from accurately inferring the type. Add a return type hint to the function signature after the mode parameter by specifying the appropriate return type that represents a text I/O object (such as TextIO from the typing module) since all three code paths—gzip.open, bz2.open, and the built-in open function—all return compatible text I/O objects when called with encoding="utf-8".Sources: Linters/SAST tools, Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 135: The individual package installation example for troubleshooting is
incomplete and missing required core dependencies. Update the `uv pip install`
command that currently lists numpy scipy h5py wcmatch to also include lmdb and
msgpack-numpy at the end of the package list. This ensures users following the
troubleshooting step have all necessary core dependencies installed for a
complete setup.
- Line 96: The Core dependencies section in AGENTS.md is incomplete and missing
two dependencies that are listed in pyproject.toml. Update the Core line that
currently reads "Core: numpy>=1.14.3, scipy, h5py, wcmatch" to include lmdb and
msgpack-numpy in the comma-separated dependency list so it matches the complete
set of core dependencies defined in pyproject.toml.
- Line 12: The documentation comment on the `uv pip install -e .` line in
AGENTS.md is incomplete and does not match the actual core dependencies declared
in pyproject.toml. Update the inline comment that lists the core dependencies to
include all six dependencies: numpy, scipy, h5py, wcmatch, lmdb, and
msgpack-numpy. Ensure the comment accurately reflects what is actually installed
by the development mode installation.
In `@dpdata/serialization.py`:
- Around line 149-154: The datetime deserialization logic in the
datetime.datetime class handler is losing timezone information by using
split("+")[0] which strips positive UTC offsets and causes failures on negative
offsets. Replace the current approach with
datetime.datetime.fromisoformat(obj["string"]) as the primary decoder to
properly preserve timezone data, and keep the existing strptime calls as
fallback for backward compatibility with older formats. This ensures
round-tripping of timezone-aware datetimes without converting them to naive
datetimes.
---
Nitpick comments:
In `@dpdata/serialization.py`:
- Around line 27-34: The _open_text function is missing a return type annotation
which prevents type checkers from accurately inferring the type. Add a return
type hint to the function signature after the mode parameter by specifying the
appropriate return type that represents a text I/O object (such as TextIO from
the typing module) since all three code paths—gzip.open, bz2.open, and the
built-in open function—all return compatible text I/O objects when called with
encoding="utf-8".
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ccd9ec6-d46e-43ed-b7ca-eaa866d12722
📒 Files selected for processing (8)
AGENTS.mddocs/conf.pydocs/environment.ymldpdata/serialization.pydpdata/system.pypyproject.tomltests/test_json.pytests/test_to_pymatgen_entry.py
💤 Files with no reviewable changes (3)
- docs/environment.yml
- docs/conf.py
- pyproject.toml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #979 +/- ##
==========================================
+ Coverage 86.95% 87.88% +0.93%
==========================================
Files 90 91 +1
Lines 8330 9382 +1052
==========================================
+ Hits 7243 8245 +1002
- Misses 1087 1137 +50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7a6f287 to
e84be01
Compare
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Preserve timezone-aware datetime values, retain YAML support through a declared backend, and align dependency documentation with project metadata. Add focused regression coverage for datetime and YAML round trips. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
Addressed the review findings in commit Validation: focused serialization tests passed; Ruff lint/format and CLI smoke checks passed. The full suite ran 2,032 tests, with 13 errors caused by unavailable optional Coding agent: Codex |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dpdata/serialization.py`:
- Around line 16-24: Update _detect_format to determine the format from the
filename’s final meaningful suffix rather than substring matches. Remove a
trailing compression suffix before inspection, then map only the resulting .mpk,
.yaml, or .yml suffixes to their formats; otherwise retain the JSON default and
explicit fmt override.
- Around line 141-176: Update process_decoded before invoking a class’s
from_dict so the marker-filtered data mapping is recursively passed through
process_decoded first. Then call cls.from_dict with the decoded nested values,
while preserving the existing Enum handling and fallback behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fba5c27-ccb8-497a-86dc-7dd5aef35fe4
📒 Files selected for processing (8)
AGENTS.mddocs/conf.pydocs/environment.ymldpdata/serialization.pydpdata/system.pypyproject.tomltests/test_json.pytests/test_to_pymatgen_entry.py
💤 Files with no reviewable changes (2)
- docs/conf.py
- docs/environment.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- tests/test_to_pymatgen_entry.py
- dpdata/system.py
- tests/test_json.py
Detect serialization formats from the final meaningful suffix and recursively decode values before passing them to custom from_dict constructors. Add regression tests for both review findings. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_json.py`:
- Around line 21-30: Add an as_dict method to the NestedSerializable test helper
that returns its stored value in the expected dictionary form, ensuring
to_serializable() traverses and decodes nested numpy/datetime values through the
nested serialization path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22ba3ebf-ba1f-4b0f-824e-ba86ae3ac2ea
📒 Files selected for processing (2)
dpdata/serialization.pytests/test_json.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dpdata/serialization.py
Make the nested serialization helper expose as_dict so the regression test genuinely traverses NumPy and datetime values through the encoder. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
There was a problem hiding this comment.
Pull request overview
This PR removes the runtime dependency on monty by introducing an internal dpdata.serialization module and updating core I/O paths and tests to use it, while preserving compatibility with existing monty-style JSON payloads.
Changes:
- Added
dpdata.serializationwith JSON/YAML/msgpack dump/load helpers, monty-style numpy/datetime decoding, and gzip/bzip2 support. - Switched
System.dump/load/from_dictand pymatgen-entry tests to use the new serializer. - Updated dependencies/docs to drop
montyand addPyYAML, and expanded serialization round-trip tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
dpdata/serialization.py |
New internal serializer to replace monty functionality and preserve monty-style decoding. |
dpdata/system.py |
Routes System dump/load/from_dict through the new serializer. |
tests/test_json.py |
Adds/extends tests for dump/load round-trips and serializer helpers. |
tests/test_to_pymatgen_entry.py |
Uses new loadfn to load monty-style pymatgen objects. |
pyproject.toml |
Removes monty, adds PyYAML, updates import-ban list accordingly. |
docs/environment.yml |
Drops monty from docs environment dependencies. |
docs/conf.py |
Removes monty intersphinx mapping. |
AGENTS.md |
Updates contributor docs to reflect new core dependency set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
`dpdata/serialization.py` was at 64% line coverage, which is why codecov/patch is red on this PR. The gaps were the paths that only run on files the existing tests never write: msgpack, gzip/bz2 streams, the ruamel fallback, and every encoder branch except ndarray. Add 35 cases covering format detection and compression-suffix stripping, text and binary streams for each codec, complex arrays, numpy scalars, UUID/Path/Enum round trips, the datetime strptime fallbacks for payloads `fromisoformat` rejects, unknown `@module`/`@class` markers falling back to plain dicts, JSON/YAML/msgpack round trips including compressed variants, and the `Invalid format` errors on both dump and load. The optional-dependency branches are exercised by patching `importlib.import_module` and `sys.modules`, so PyYAML-missing, ruamel-missing, and msgpack-missing all raise their intended messages without changing the test environment. Coverage of the module is now 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Closed the
Two notes on how the harder branches are reached:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Correcting my earlier review first: I claimed YAML dump/load "worked out of the box" under monty. It did not. monty.serialization.dumpfn sets cls=MontyEncoder only in the json branch, so on master System.dump("x.yaml") raises TypeError: YAML.dump() got an unexpected keyword argument 'indent' (and without indent, RepresenterError: cannot represent an object: np.int64(2)), while loadfn returns a raw CommentedMap rather than a System. So the System-level YAML path never worked, and this PR genuinely fixes it.
The dependency half of my concern was real and is properly addressed: PyYAML is in the core dependencies array, monty is fully removed, and JSON output is byte-identical to monty's, so files round-trip in both directions with older dpdata.
Three things below I would like resolved before merge -- the silent .xz behavior in particular.
| 'monty', | ||
| 'scipy', | ||
| 'h5py', | ||
| 'PyYAML', |
There was a problem hiding this comment.
This line is the actual fix, but nothing in the suite guards it.
_yaml_dump / _yaml_load are byte-identical before and after this PR, so the only pre-fix/post-fix difference is this declaration -- and a round-trip test running inside an already-provisioned interpreter cannot observe a metadata declaration. CI installs .[test,amber,ase,pymatgen], and pymatgen -> monty -> ruamel.yaml, so a YAML backend is always importable; I confirmed the YAML tests pass on the pre-fix commit and fail only when both backends are blocked with an import hook. So the added "regression test" passes on the broken code too.
A cheap genuine guard already almost exists: .github/workflows/test_import.yml does a core-only uv pip install --system . and then just runs python -c 'import dpdata'. Extending that to an actual .yaml dump/load would fail pre-fix and pass post-fix.
| def dump(self, filename: str, indent: int = 4): | ||
| """Dump .json or .yaml file.""" | ||
| from monty.serialization import dumpfn | ||
| """Dump a JSON, YAML, or MessagePack file.""" |
There was a problem hiding this comment.
Two things on this line.
indent is forwarded unconditionally, but _detect_format routes .mpk into msgpack.dump(obj, fp, **kwargs), so System.dump("x.mpk") raises TypeError: Packer.__init__() got an unexpected keyword argument 'indent' -- while this PR newly changes the docstring from "Dump .json or .yaml file" to advertise MessagePack. dumpfn(s.as_dict(), "x.mpk") works when called directly, so gating indent to the text formats is enough.
Separately, please add a System-level YAML test. Both current YAML tests use plain dicts, so the indent=4 + numpy-payload + System-reconstruction path this PR actually repairs has no coverage. A TestYamlDumpLoad mirroring the existing TestJsonDumpLoad is about 12 lines, errors 15/15 on master, and passes here -- a proper regression guard for the fix.
| return "json" | ||
|
|
||
|
|
||
| def _open_text(filename: str | Path, mode: str) -> TextIO: |
There was a problem hiding this comment.
This drops .xz / .lzma, which monty's zopen handled, and it fails silently in both directions:
dumpfn(obj, "x.json.xz")now writes plain JSON under an.xzname --filereports "JSON text data" where master reports "XZ compressed data, checksum CRC64". No error, no warning.- Reading a
.xzfile written by an older dpdata raisesUnicodeDecodeError: 'utf-8' codec can't decode byte 0xfd in position 0.
Silent wrong output is worse than a hard failure here. Either add the lzma branch alongside .gz/.z/.bz2, or raise on an unrecognized compression suffix so the loss cannot go unnoticed.
Summary
Tests
Summary by CodeRabbit
montyfrom runtime dependencies and updated lint rules accordingly.monty.