fix(serialization): remove monty dependency#979
Conversation
dosubot
Bot
added
size:L
Merging this PR will not alter performance
|
📝 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.49% -0.46%
==========================================
Files 90 91 +1
Lines 8330 8503 +173
==========================================
+ Hits 7243 7355 +112
- Misses 1087 1148 +61 ☔ 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
| class NestedSerializable: | ||
| """Test helper that records values passed through ``from_dict``.""" | ||
|
|
||
| def __init__(self, value): | ||
| self.value = value | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data): | ||
| """Construct the helper from decoded serialized data.""" | ||
| return cls(data["value"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Applies the fix and runs the specific test module to verify functionality.
sed -i '/def from_dict/i \ def as_dict(self):\n return {"value": self.value}\n' tests/test_json.py
python -m unittest tests.test_json.TestSerialization.test_from_dict_receives_decoded_nested_valuesRepository: deepmodeling/dpdata
Length of output: 876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- tests/test_json.py (outline) ---\n'
ast-grep outline tests/test_json.py --view expanded || true
printf '\n--- search for NestedSerializable / to_serializable / process_decoded ---\n'
rg -n "NestedSerializable|to_serializable|process_decoded|from_dict_receives_decoded_nested_values|as_dict" tests src . -g '!**/__pycache__/**' || true
printf '\n--- relevant slice of tests/test_json.py ---\n'
nl -ba tests/test_json.py | sed -n '1,220p'Repository: deepmodeling/dpdata
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tests/test_json.py")
print(path.read_text())
PYRepository: deepmodeling/dpdata
Length of output: 3936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/test_json.py excerpt ---'
nl -ba tests/test_json.py | sed -n '1,120p'
echo
echo '--- dpdata json helpers ---'
rg -n "def to_serializable|def process_decoded|as_dict|from_dict" -g '*.py' .Repository: deepmodeling/dpdata
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- locate json helper implementation ---\n'
rg -n "to_serializable|process_decoded" -g '*.py' .
printf '\n--- locate tests/test_json.py lines ---\n'
nl -ba tests/test_json.py | sed -n '1,220p'Repository: deepmodeling/dpdata
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path("tests/test_json.py").read_text()
for needle in ["class NestedSerializable", "def as_dict", "def from_dict_receives_decoded_nested_values"]:
print(f"{needle}: {needle in text}")
PYRepository: deepmodeling/dpdata
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
def show(path, start=None, end=None):
text = Path(path).read_text().splitlines()
if start is None:
start, end = 1, len(text)
for i in range(start - 1, min(end, len(text))):
print(f"{i+1:4}: {text[i]}")
print("=== tests/test_json.py ===")
show("tests/test_json.py", 1, 120)
print("\n=== search serialization helpers ===")
for p in Path(".").rglob("*.py"):
txt = p.read_text(errors="ignore")
if "def to_serializable" in txt or "def process_decoded" in txt:
print(p)
PYRepository: deepmodeling/dpdata
Length of output: 4641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
# Find likely serialization implementation files without importing repository code.
for p in Path(".").rglob("*.py"):
if "serialization" in p.name:
print(f"FILE: {p}")
text = p.read_text(errors="ignore").splitlines()
for i, line in enumerate(text, 1):
if "def to_serializable" in line or "def process_decoded" in line or "class" in line and "Serializable" in line:
start = max(1, i - 8)
end = min(len(text), i + 35)
for j in range(start, end + 1):
print(f"{j:4}: {text[j-1]}")
print("-" * 60)
PYRepository: deepmodeling/dpdata
Length of output: 4340
Add as_dict to NestedSerializable so this case exercises the nested serialization path. to_serializable() only recurses into objects that expose as_dict, so without it this test can pass without decoding the nested numpy/datetime values.
🤖 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 `@tests/test_json.py` around lines 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.
Source: Coding guidelines
3 participants
Summary
Tests
Summary by CodeRabbit
montyfrom runtime dependencies and adjusted lint rules accordingly.monty.