feat(show): add serialization-tree via DeepEval.serialize#5316
feat(show): add serialization-tree via DeepEval.serialize#5316njzjz-bot wants to merge 3 commits intodeepmodeling:masterfrom
Conversation
Authored by OpenClaw (model: gpt-5.2)
|
Implements #5185 (first step): add powered by a backend-unified wrapper.\n\nAuthored by OpenClaw (model: gpt-5.2) |
|
Implements #5185 (first step): add Authored by OpenClaw (model: gpt-5.2) |
|
(FYI) Previous comment got mangled by shell backticks; this one is the corrected text.\n\nAuthored by OpenClaw (model: gpt-5.2) |
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughAdds a "serialization-tree" show option and a model serialization visualization: show entrypoint now can call model.serialize(), validate serialized output, deserialize into a Node tree, and log it. Introduces a serialize() API on DeepEval and backend implementations; stores model_file on DeepEval for backend resolution. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Show as Show Entrypoint
participant DeepEval as DeepEval Interface
participant Backend as Backend Module
participant Node as Node (Serialization)
CLI->>Show: invoke show with "serialization-tree"
Show->>DeepEval: request serialize()
DeepEval->>Backend: resolve backend using model_file
Backend-->>DeepEval: serialized dict (includes "model")
Show->>Node: Node.deserialize(serialized["model"])
Node-->>Show: Node tree
Show->>Show: log serialization tree
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deepmd/infer/deep_eval.py (1)
444-446: Cyclic import flagged by static analysis.CodeQL reports a cyclic import starting from
deepmd.pretrained.deep_eval. While placing the import inside the method body mitigates runtime issues (the import only occurs when the pretrained branch is taken), consider whether these utilities could be imported from a lower-level module to avoid the cycle entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/infer/deep_eval.py` around lines 444 - 446, The import of parse_pretrained_alias inside deepmd.infer.deep_eval creates a cyclic dependency with deepmd.pretrained.deep_eval; to fix it, extract parse_pretrained_alias (and any small helper utilities it needs) into a lower-level module (e.g., deepmd.pretrained.utils or deepmd.utils.pretrained) and update both deepmd.pretrained.deep_eval and deepmd.infer.deep_eval to import parse_pretrained_alias from that new module; ensure the extracted function has no imports back to deepmd.pretrained.deep_eval to break the cycle and run tests to confirm no runtime regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/infer/deep_eval.py`:
- Around line 443-454: When resolving a pretrained alias you re-detect the
backend and immediately call serialize_hook on backend_cls (via
Backend.detect_backend_by_model and backend_cls().serialize_hook) without
checking that the backend supports the IO feature; add the same IO capability
check used in the regular path before calling serialize_hook: after determining
backend_cls = Backend.detect_backend_by_model(resolved), verify
Backend.feature(backend_cls, Backend.Feature.IO) (or equivalent feature-check
method used elsewhere) and raise the same NotImplementedError with the same
message if IO is not supported, otherwise call
backend_cls().serialize_hook(resolved).
---
Nitpick comments:
In `@deepmd/infer/deep_eval.py`:
- Around line 444-446: The import of parse_pretrained_alias inside
deepmd.infer.deep_eval creates a cyclic dependency with
deepmd.pretrained.deep_eval; to fix it, extract parse_pretrained_alias (and any
small helper utilities it needs) into a lower-level module (e.g.,
deepmd.pretrained.utils or deepmd.utils.pretrained) and update both
deepmd.pretrained.deep_eval and deepmd.infer.deep_eval to import
parse_pretrained_alias from that new module; ensure the extracted function has
no imports back to deepmd.pretrained.deep_eval to break the cycle and run tests
to confirm no runtime regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a8a6ae18-b1d4-465f-b5b3-2149b4fbd411
📒 Files selected for processing (3)
deepmd/entrypoints/show.pydeepmd/infer/deep_eval.pydeepmd/main.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5316 +/- ##
==========================================
- Coverage 82.29% 82.25% -0.05%
==========================================
Files 775 775
Lines 77627 77688 +61
Branches 3676 3676
==========================================
+ Hits 63887 63901 +14
- Misses 12566 12611 +45
- Partials 1174 1176 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Switch DeepEval.serialize() to delegate to DeepEvalBackend.serialize(), and implement serialize() in each backend by calling the underlying model's serialize(). Also move Node import in dp show to module top-level. Authored by OpenClaw (model: gpt-5.2)
|
Refactor update (per feedback): switch from
Latest commit: 1694360 Authored by OpenClaw (model: gpt-5.2) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/pt_expt/infer/deep_eval.py`:
- Around line 668-673: serialize() currently returns the raw model_dict from
serialize_from_file(self.model_path) but callers (e.g., the CLI in show.py)
expect a wrapper with keys "backend", "model", "model_def_script", and
"@variables"; change serialize() in the class to call
serialize_from_file(self.model_path), then construct and return a dict like
{"backend": "<appropriate backend name or value from model_dict if available>",
"model": model_dict, "model_def_script": <script or None>, "@variables": <vars
dict or empty dict>} so the returned structure contains those keys; ensure you
pull any available values from model_dict to populate "backend",
"model_def_script", and "@variables" or set sensible defaults if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7533fe2-e54e-4e12-ae64-e1712fa5c00b
📒 Files selected for processing (9)
deepmd/dpmodel/infer/deep_eval.pydeepmd/entrypoints/show.pydeepmd/infer/deep_eval.pydeepmd/jax/infer/deep_eval.pydeepmd/pd/infer/deep_eval.pydeepmd/pretrained/deep_eval.pydeepmd/pt/infer/deep_eval.pydeepmd/pt_expt/infer/deep_eval.pydeepmd/tf/infer/deep_eval.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/entrypoints/show.py
| def serialize(self) -> dict[str, Any]: | ||
| from deepmd.pt_expt.utils.serialization import ( | ||
| serialize_from_file, | ||
| ) | ||
|
|
||
| return serialize_from_file(self.model_path) |
There was a problem hiding this comment.
Contract violation: Missing wrapper structure for serialized output.
Per the relevant code snippet from deepmd/pt_expt/utils/serialization.py:201-220, serialize_from_file() returns only the raw model_dict, not the expected wrapper structure with "backend", "model", "model_def_script", and "@variables" keys.
The CLI consumer at deepmd/entrypoints/show.py:143-148 expects data["model"] to exist:
if "model" not in data:
raise RuntimeError("Serialized model data does not contain key 'model'.")This will cause a RuntimeError when users run dp show model.pte serialization-tree.
🐛 Proposed fix to wrap the serialized output
def serialize(self) -> dict[str, Any]:
from deepmd.pt_expt.utils.serialization import (
serialize_from_file,
)
- return serialize_from_file(self.model_path)
+ model_dict = serialize_from_file(self.model_path)
+ data: dict[str, Any] = {
+ "backend": "PyTorch Exportable",
+ "model": model_dict,
+ "model_def_script": self.get_model_def_script(),
+ "@variables": {},
+ }
+ return data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/pt_expt/infer/deep_eval.py` around lines 668 - 673, serialize()
currently returns the raw model_dict from serialize_from_file(self.model_path)
but callers (e.g., the CLI in show.py) expect a wrapper with keys "backend",
"model", "model_def_script", and "@variables"; change serialize() in the class
to call serialize_from_file(self.model_path), then construct and return a dict
like {"backend": "<appropriate backend name or value from model_dict if
available>", "model": model_dict, "model_def_script": <script or None>,
"@variables": <vars dict or empty dict>} so the returned structure contains
those keys; ensure you pull any available values from model_dict to populate
"backend", "model_def_script", and "@variables" or set sensible defaults if
missing.
1 participant
@/tmp/pr-body-5185.md
Summary by CodeRabbit
Release Notes