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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #979 +/- ##
==========================================
- Coverage 86.95% 86.54% -0.41%
==========================================
Files 90 91 +1
Lines 8330 9351 +1021
==========================================
+ Hits 7243 8093 +850
- Misses 1087 1258 +171 ☔ 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
Summary
Tests
Summary by CodeRabbit
montyfrom runtime dependencies and updated lint rules accordingly.monty.