fix(serialization): remove monty dependency#979
Conversation
dosubot
Bot
added
size:L
Merging this PR will not alter performance
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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/msgpack
FormatBackend-->>dpdata_serialization: serialized or parsed data
dpdata_serialization->>process_decoded: decode payload
process_decoded-->>System: reconstructed data
🚥 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.42% -0.53%
==========================================
Files 90 91 +1
Lines 8330 8500 +170
==========================================
+ Hits 7243 7346 +103
- Misses 1087 1154 +67 ☔ 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 |
| ruamel_yaml = importlib.import_module("ruamel.yaml") | ||
| except ModuleNotFoundError as e: | ||
| raise RuntimeError( | ||
| "Loading YAML files requires PyYAML or ruamel.yaml." |
There was a problem hiding this comment.
Code review
Found 1 issue:
- Dropping
montyremoves the only YAML backend, so the documented.yamldump/load path now raises on a clean install.
monty declared ruamel.yaml as a hard dependency, so System.dump("x.yaml") / System.load("x.yaml") worked out of the box. This PR removes monty from dependencies in pyproject.toml without adding PyYAML or ruamel.yaml, while System.dump/System.load still advertise .yaml. With neither backend importable, _yaml_load/_yaml_dump raise RuntimeError here, a regression for a documented format. Consider adding a YAML backend to dependencies (or an optional extra) and/or noting the requirement in the dump/load docstrings.
There was a problem hiding this comment.
Added PyYAML to the core dependencies, so a clean installation retains the documented YAML dump/load capability after removing monty. Added a YAML dump/load regression test as well.
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
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
| def _detect_format(filename: str | Path, fmt: str | None = None) -> str: | ||
| if fmt is not None: | ||
| return fmt | ||
| basename = Path(filename).name.lower() | ||
| if ".mpk" in basename: | ||
| return "mpk" | ||
| if ".yaml" in basename or ".yml" in basename: | ||
| return "yaml" | ||
| return "json" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect formats from suffixes, not substrings.
Names such as results.yaml.json or model.mpk.backup are parsed as YAML/msgpack despite having different final extensions. Strip compression suffixes, then inspect the actual format suffix.
Proposed fix
def _detect_format(filename: str | Path, fmt: str | None = None) -> str:
if fmt is not None:
return fmt
- basename = Path(filename).name.lower()
- if ".mpk" in basename:
+ suffixes = [suffix.lower() for suffix in Path(filename).suffixes]
+ if suffixes and suffixes[-1] in {".gz", ".z", ".bz2"}:
+ suffixes.pop()
+ suffix = suffixes[-1] if suffixes else ""
+ if suffix == ".mpk":
return "mpk"
- if ".yaml" in basename or ".yml" in basename:
+ if suffix in {".yaml", ".yml"}:
return "yaml"
return "json"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _detect_format(filename: str | Path, fmt: str | None = None) -> str: | |
| if fmt is not None: | |
| return fmt | |
| basename = Path(filename).name.lower() | |
| if ".mpk" in basename: | |
| return "mpk" | |
| if ".yaml" in basename or ".yml" in basename: | |
| return "yaml" | |
| return "json" | |
| def _detect_format(filename: str | Path, fmt: str | None = None) -> str: | |
| if fmt is not None: | |
| return fmt | |
| suffixes = [suffix.lower() for suffix in Path(filename).suffixes] | |
| if suffixes and suffixes[-1] in {".gz", ".z", ".bz2"}: | |
| suffixes.pop() | |
| suffix = suffixes[-1] if suffixes else "" | |
| if suffix == ".mpk": | |
| return "mpk" | |
| if suffix in {".yaml", ".yml"}: | |
| return "yaml" | |
| return "json" |
🤖 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 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.
| def process_decoded(obj: Any) -> Any: | ||
| """Decode monty-style dictionaries used by existing dpdata JSON files.""" | ||
| if isinstance(obj, dict): | ||
| if "@module" in obj and "@class" in obj: | ||
| module_name = obj["@module"] | ||
| class_name = obj["@class"] | ||
| if module_name == "numpy" and class_name == "array": | ||
| return _decode_ndarray(obj) | ||
| if module_name == "datetime" and class_name == "datetime": | ||
| try: | ||
| return datetime.datetime.fromisoformat(obj["string"]) | ||
| except ValueError: | ||
| value = obj["string"].split("+")[0] | ||
| try: | ||
| return datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S.%f") | ||
| except ValueError: | ||
| return datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S") | ||
| if module_name == "uuid" and class_name == "UUID": | ||
| return UUID(obj["string"]) | ||
| if module_name == "pathlib" and class_name == "Path": | ||
| return Path(obj["string"]) | ||
| try: | ||
| module = importlib.import_module(module_name) | ||
| cls = getattr(module, class_name) | ||
| except (AttributeError, ImportError, ModuleNotFoundError): | ||
| cls = None | ||
| if cls is not None: | ||
| data = {k: v for k, v in obj.items() if not k.startswith("@")} | ||
| if hasattr(cls, "from_dict"): | ||
| return cls.from_dict(data) | ||
| if isinstance(cls, type) and issubclass(cls, Enum): | ||
| return cls(process_decoded(data["value"])) | ||
| return {process_decoded(k): process_decoded(v) for k, v in obj.items()} | ||
| if isinstance(obj, list): | ||
| return [process_decoded(v) for v in obj] | ||
| return obj |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Decode nested fields before invoking from_dict.
Line 170 passes raw marker dictionaries to from_dict, so nested NumPy arrays, datetimes, UUIDs, paths, enums, or serializable objects do not round-trip correctly.
Proposed fix
- data = {k: v for k, v in obj.items() if not k.startswith("@")}
+ data = {
+ k: process_decoded(v)
+ for k, v in obj.items()
+ if not k.startswith("@")
+ }
if hasattr(cls, "from_dict"):
return cls.from_dict(data)
if isinstance(cls, type) and issubclass(cls, Enum):
- return cls(process_decoded(data["value"]))
+ return cls(data["value"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def process_decoded(obj: Any) -> Any: | |
| """Decode monty-style dictionaries used by existing dpdata JSON files.""" | |
| if isinstance(obj, dict): | |
| if "@module" in obj and "@class" in obj: | |
| module_name = obj["@module"] | |
| class_name = obj["@class"] | |
| if module_name == "numpy" and class_name == "array": | |
| return _decode_ndarray(obj) | |
| if module_name == "datetime" and class_name == "datetime": | |
| try: | |
| return datetime.datetime.fromisoformat(obj["string"]) | |
| except ValueError: | |
| value = obj["string"].split("+")[0] | |
| try: | |
| return datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S.%f") | |
| except ValueError: | |
| return datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S") | |
| if module_name == "uuid" and class_name == "UUID": | |
| return UUID(obj["string"]) | |
| if module_name == "pathlib" and class_name == "Path": | |
| return Path(obj["string"]) | |
| try: | |
| module = importlib.import_module(module_name) | |
| cls = getattr(module, class_name) | |
| except (AttributeError, ImportError, ModuleNotFoundError): | |
| cls = None | |
| if cls is not None: | |
| data = {k: v for k, v in obj.items() if not k.startswith("@")} | |
| if hasattr(cls, "from_dict"): | |
| return cls.from_dict(data) | |
| if isinstance(cls, type) and issubclass(cls, Enum): | |
| return cls(process_decoded(data["value"])) | |
| return {process_decoded(k): process_decoded(v) for k, v in obj.items()} | |
| if isinstance(obj, list): | |
| return [process_decoded(v) for v in obj] | |
| return obj | |
| def process_decoded(obj: Any) -> Any: | |
| """Decode monty-style dictionaries used by existing dpdata JSON files.""" | |
| if isinstance(obj, dict): | |
| if "`@module`" in obj and "`@class`" in obj: | |
| module_name = obj["`@module`"] | |
| class_name = obj["`@class`"] | |
| if module_name == "numpy" and class_name == "array": | |
| return _decode_ndarray(obj) | |
| if module_name == "datetime" and class_name == "datetime": | |
| try: | |
| return datetime.datetime.fromisoformat(obj["string"]) | |
| except ValueError: | |
| value = obj["string"].split("+")[0] | |
| try: | |
| return datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S.%f") | |
| except ValueError: | |
| return datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S") | |
| if module_name == "uuid" and class_name == "UUID": | |
| return UUID(obj["string"]) | |
| if module_name == "pathlib" and class_name == "Path": | |
| return Path(obj["string"]) | |
| try: | |
| module = importlib.import_module(module_name) | |
| cls = getattr(module, class_name) | |
| except (AttributeError, ImportError, ModuleNotFoundError): | |
| cls = None | |
| if cls is not None: | |
| data = { | |
| k: process_decoded(v) | |
| for k, v in obj.items() | |
| if not k.startswith("@") | |
| } | |
| if hasattr(cls, "from_dict"): | |
| return cls.from_dict(data) | |
| if isinstance(cls, type) and issubclass(cls, Enum): | |
| return cls(data["value"]) | |
| return {process_decoded(k): process_decoded(v) for k, v in obj.items()} | |
| if isinstance(obj, list): | |
| return [process_decoded(v) for v in obj] | |
| return obj |
🤖 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 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.
3 participants
Summary
Tests
Summary by CodeRabbit
montyfrom runtime dependencies and adjusted lint rules accordingly.monty.