Repository navigation
Conversation
dp convert-backend stores it there, so compress recomputed it from the training data on every run. Add --recompute-min-nbor-dist to force a recompute, and log where the value comes from.
Sending a whole set to the device at once needs hundreds of GiB. Use AutoBatchSize, as the pt, pd, jax and tf backends already do.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 70.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled. Title check ✅ Passed The title clearly summarizes the two main changes: reusing stored min_nbor_dist values and batching neighbor-statistics computation. Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/pt_expt/test_compress_min_nbor_dist.py (1)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd execution-path tests for forced recomputation.
This test only confirms parser state. Add focused tests that verify
enable_compressionignores a saved value, assigns the value returned byUpdateSel.get_min_nbor_dist, and rejects a missingtraining_script.As per coding guidelines, use pytest for single test cases instead of the full test suite.
🤖 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 `@source/tests/pt_expt/test_compress_min_nbor_dist.py` around lines 66 - 70, Extend the compression tests around enable_compression to cover forced recomputation: verify the saved minimum-neighbor-distance value is ignored, the result from UpdateSel.get_min_nbor_dist is assigned, and a missing training_script is rejected. Use focused pytest test cases with mocked dependencies rather than invoking the full test suite.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@source/tests/pt_expt/test_compress_min_nbor_dist.py`:
- Around line 66-70: Extend the compression tests around enable_compression to
cover forced recomputation: verify the saved minimum-neighbor-distance value is
ignored, the result from UpdateSel.get_min_nbor_dist is assigned, and a missing
training_script is rejected. Use focused pytest test cases with mocked
dependencies rather than invoking the full test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dae69f2-f343-4c91-bb44-52c1212ea168
📒 Files selected for processing (5)
deepmd/main.pydeepmd/pt_expt/entrypoints/compress.pydeepmd/pt_expt/entrypoints/main.pydeepmd/pt_expt/utils/neighbor_stat.pysource/tests/pt_expt/test_compress_min_nbor_dist.py
There was a problem hiding this comment.
Pull request overview
This PR improves the dp --pt-expt compress workflow by avoiding unnecessary recomputation of min_nbor_dist when it is already stored in the model (including under @variables), and by reducing peak device memory use when computing neighbor statistics via frame batching.
Changes:
- Teach
pt_expt compressto reusemin_nbor_distfrom the serialized model buffer, the top-levelmin_nbor_distkey, or@variables, with a new--recompute-min-nbor-distoverride flag. - Batch
NeighborStat.iteratorevaluations viaAutoBatchSize.execute_allto avoid loading entire datasets onto device at once. - Add pytest coverage for the
min_nbor_distread precedence and CLI flag default behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
source/tests/pt_expt/test_compress_min_nbor_dist.py |
Adds tests for stored min_nbor_dist precedence and the new CLI flag default. |
deepmd/pt_expt/utils/neighbor_stat.py |
Uses AutoBatchSize to batch neighbor-stat execution over frames to reduce device memory pressure. |
deepmd/pt_expt/entrypoints/main.py |
Wires the new recompute_min_nbor_dist flag through to the compress entrypoint. |
deepmd/pt_expt/entrypoints/compress.py |
Adds @variables lookup for min_nbor_dist and supports forced recomputation via a new parameter. |
deepmd/main.py |
Exposes --recompute-min-nbor-dist on the compress CLI parser (scoped in help to pt-expt). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ce Liu <lc6866@outlook.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
deepmd/pt_expt/entrypoints/compress.py:43
_read_saved_min_nbor_distassumesmodel_dict["@variables"]is a dict by doing(model_dict.get("@variables") or {}).get(...). If@variablesis present but not a mapping (corrupt/older metadata, or other producers), this will raiseAttributeErrorduring compress. Other code paths in the repo defensively guard@variableswithisinstance(..., dict)(e.g. pt_expt DPA4 normalization).
return float(min_nbor_dist), "the model file"
min_nbor_dist = (model_dict.get("@variables") or {}).get("min_nbor_dist")
if min_nbor_dist is not None:
return float(min_nbor_dist), "the model file (@variables)"
return None, ""
deepmd/main.py:677
--recompute-min-nbor-distis added to the top-leveldp compressparser, so it will be accepted for all backends. However, only the PyTorch Exportable backend actually reads/usesrecompute_min_nbor_dist; other backends ignore it (e.g.deepmd/pt/entrypoints/main.pyanddeepmd/jax/entrypoints/main.pydon't pass it through). This can mislead users because the CLI will accept the flag but it will have no effect unless the backend is PyTorch Exportable.
Consider validating at dispatch time (or in each backend entrypoint) that this flag is only allowed with the PyTorch Exportable backend, and error out otherwise.
parser_compress.add_argument(
"--recompute-min-nbor-dist",
action="store_true",
help="(Supported backend: PyTorch Exportable) Ignore the minimal neighbor "
"distance saved in the model and recompute it from the training data. "
"Requires -t,--training-script",
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
deepmd/pt_expt/entrypoints/compress.py:41
model_dict.get("@variables")is assumed to be a dict, but if a malformed/older model file stores a truthy non-mapping (e.g. a list/string),(model_dict.get("@variables") or {}).get(...)will raiseAttributeError. Since this data comes from disk, it’s safer to guard the type and treat non-dict values as “not present” (or raise a clearerValueError).
min_nbor_dist = (model_dict.get("@variables") or {}).get("min_nbor_dist")
if min_nbor_dist is not None:
return float(min_nbor_dist), "the model file (@variables)"
return None, ""
deepmd/pt_expt/utils/neighbor_stat.py:82
- This change routes neighbor-stat computation through
AutoBatchSize.execute_all, but there’s no unit-level regression test to ensure batching is actually invoked (and keeps working) for the pt_expt backend. Consider adding a lightweight test (e.g. monkeypatchAutoBatchSize.execute_allto assert it’s called) so future refactors don’t accidentally revert to whole-set execution.
minrr2, max_nnei = self.auto_batch_size.execute_all(
self._execute,
data_set_data["coord"].shape[0],
data_set.get_natoms(),
data_set_data["coord"],
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The saved-distance lookup and frame batching address the main performance problem, but three integration gaps remain: graph-lower outputs discard the recovered metadata, virtual-spin wrappers do not apply it to the backbone that performs compression, and explicit CPU execution on CUDA hosts uses a GPU-style expanding batch policy that can again exhaust host memory. The inline comments describe the affected paths and fixes.
Codex quota is about to reset, so I am using the remaining token budget to complete a concentrated review pass over the outstanding PRs.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
DEVICE is CPU whenever DEVICE=cpu is set, even on a CUDA host, where the CUDA-OOM handler cannot recover from a host OOM. Base the growth policy on the selected device instead of CUDA availability.
model.serialize() does not carry the runtime buffer, so the graph branch dropped the value it had just recovered and the next run fell back to -t.
SpinModel.__getattr__ only delegates reads, so the assignment landed on the wrapper while get_min_nbor_dist and enable_compression kept reading the backbone.
for more information, see https://pre-commit.ci
|
Should wait for the merge of DPA4C, which refactors this. |
# Conflicts: # deepmd/pt_expt/entrypoints/compress.py
Merged master now that DPA4C has landed. Only |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
NEEDS HUMAN REVIEW
Re-reviewed the current head after the previous requested changes. The three earlier blocking issues are addressed in this revision: pt_expt batching now follows the selected DEVICE, graph-lower compression preserves min_nbor_dist in the exported archive, and SpinModel delegates min_nbor_dist assignment to its backbone. I reviewed all 8 changed files and the added tests and did not find another high-confidence functional blocker.
I cannot approve this head yet because relevant CI has not completed: the pull-request GitHub Actions runs are in action_required state with no jobs started, and the Read the Docs status is still pending. pre-commit.ci and CodeRabbit are successful. Please let the required Actions/RTD checks run to completion before approval.
Agent: ChatGPT
Model: GPT-5.6 Sol
GitHub account: njzjz-bot
Reviewed head: fcd067c
Trigger: scheduled review-request monitoring
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5956 +/- ##
==========================================
- Coverage 77.85% 77.60% -0.25%
==========================================
Files 1170 1171 +1
Lines 140914 140945 +31
Branches 5056 5056
==========================================
- Hits 109705 109378 -327
- Misses 29325 29684 +359
+ Partials 1884 1883 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Importing the value froze the device at import time, which defeats the point of a policy meant to follow the selected device.
It was reached both by `import` and by `import from`.
for more information, see https://pre-commit.ci
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
NEEDS HUMAN REVIEW: I re-reviewed the current head after the update. The current changes address the previously relevant device-selection concern by resolving the selected PyTorch device at call time, and I did not find another high-confidence functional, API/backward-compatibility, edge-case, concurrency/resource/security, or test-design defect in the reviewed diff that warrants an inline blocker.
I cannot approve this exact head yet because the repository's relevant GitHub Actions runs for this SHA have completed with action_required rather than actually running/passing (including Test Python, Test CUDA, Test C++, Build C/C++, CodeQL, and packaging). The available external checks (pre-commit.ci and Read the Docs) are successful, but they do not exercise the changed compression/neighbor-statistics paths sufficiently to replace the project test suite. Please allow/run the workflows and re-request review after the relevant checks pass.
Agent: ChatGPT
Model: GPT-5.6 Sol
GitHub account: njzjz-bot
Reviewed head: 2c69cff
Trigger: scheduled review-request monitoring
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. The previously reported integration gaps are addressed: graph-lowered archives retain min_nbor_dist, the spin wrapper delegates the property to the backbone that compression reads, and pt_expt batching follows the selected env.DEVICE rather than CUDA availability. The stored-value lookup order and recompute path are coherent, and the neighbor-stat batching matches the memory-bounded design used by the other backends. I did not find a blocking issue. Upstream currently exposes only the successful triage check on this head; the PR includes focused regression validation.
Agent: ChatGPT
Model: GPT-5.6 Sol
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P2] Keep unused compression arguments separate from measured distance metadata
In deepmd/pt_expt/entrypoints/compress.py:85-96, --recompute-min-nbor-dist discards the saved distance before checking compression_needs_min_nbor_dist(). For DPA4C, that check then skips statistics and substitutes 0.0, which is subsequently persisted as min_nbor_dist in the output archive.
I reproduced this through the actual CLI entry point with a DPA4C .pte, a saved distance of 0.75 angstrom, and a valid training script whose dataset has a measured minimum distance of 1.25 angstrom. Ordinary compression preserves 0.75; adding --recompute-min-nbor-dist skips statistics and writes 0.0 instead.
Please keep DPA4C's statistics-free compression behavior. This is not a request to scan data that DPA4C does not need. Its tabulation domain is fixed independently of the minimum distance, so this does not directly change its energy or force predictions. The defect is treating an unused argument's placeholder as a measured statistic and overwriting valid metadata.
Please structure the decision around whether compression consumes this statistic:
- If it does, reuse the saved value by default; recompute from the supplied training data when explicitly requested, retaining the required training-script validation.
- If it does not, skip statistics even when the flag is supplied, and clearly warn that the recompute option is inapplicable to this model. Preserve an existing real distance unchanged; leave it absent when no measurement is available.
- If the common compression interface still requires a numeric argument, a local
0.0placeholder is sufficient for a descriptor that ignores it. Do not assign or serialize that placeholder as measuredmin_nbor_dist. No broad compression-interface redesign is needed.
Please cover both saved-value and missing-value cases for DPA4C, with and without the flag, checking the output metadata and that no dataset scan occurs. The flag help/docstring should also reflect that it applies only to models whose compression needs the statistic.
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Reviewed at bd1c734.
Verification:
- Ran
source/tests/pt_expt/test_compress_min_nbor_dist.pyandsource/tests/pt_expt/model/test_model_compression.pyon CPU with the custom op library: 18 passed on this head. With the base versions of the six source files restored, all 11 new tests fail;test_compress_min_nbor_dist_from_variablesfails with the originalValueError(no min_nbor_dist without-t), so it does expose the@variablesbug. - The neighbor-statistics batching gives identical results to the whole-set path in a local check, but no test covers it (see inline).
- Also ran
test_spin_ener_model.py(10 passed),test_entrypoint.py(10 passed) andtest_dpa4c_cpu.py(2 passed, 31 skipped on CPU).
Findings are inline: the recompute flag and the placeholder 0.0 being persisted as a measured distance for models that do not use it (same issue as the open change request), and two test gaps.
--recompute-min-nbor-dist dropped the saved distance before checking whether compression consumes it, so a model that skips the statistics wrote the 0.0 placeholder over a valid one. Decide on that first, and record only a measured distance.
Fixed in 3b3f493. You were right that the flag discarded the saved distance The decision is now made on whether compression consumes the statistic:
Tests cover DPA4C with and without a saved value, each with and without the |
Nothing in source/tests imported the pt_expt NeighborStat and the consistent suite skipped that backend, so the batching had no test. Pin the split itself, since comparing results alone passes unbatched too. Also run the consistent suite against pt-expt, and recompute with nothing saved.
Thanks for the thorough verification — both findings were right. The placeholder (compress.py). Same defect @OutisLi raised, fixed in
The flag's The untested batching. You were right that nothing covered it — I had only plus I also added The recompute branches. Also addressed in 3b3f493: |
dp --pt-expt compressrecomputes the minimal neighbor distance from thetraining data on every run even when the model already carries it, and the
recomputation sends a whole set to the device at once.
Reading
min_nbor_distfrom@variablesdp convert-backendstores the value under@variablesinmodel.json— thelocation
deepmd/pt/utils/serialization.pywrites and the PyTorch and Paddlebackends read back.
enable_compressionlooked only atmodel.get_min_nbor_dist()and a top-levelmin_nbor_distkey, so it neverfound the value and fell back to a full pass over the training data. It now
checks
@variablestoo, and logs the source:--recompute-min-nbor-distforces a recompute, for a model compressed againsta data set other than the one it was trained on.
Batching the neighbor statistics
NeighborStat.iteratorpassed a whole set to_execute. The intermediatetensor is
[nframes, nloc, nall, 3], andnallis 27·nlocunder periodicboundaries: a 470-frame, 280-atom set allocates ~24 GiB per intermediate with
several live at once, and larger sets do not fit. It now goes through
AutoBatchSize, as thept,pd,jaxandtfbackends already do.Verification
On a 6-system, 1257-frame carbon nanotube data set (A800-80G),
min_nbor_distis
1.240096678690whether recomputed with batching (~3 s) or read from@variables(statistics skipped). The serialized compressed model, tabulateddata included, is identical in both cases and matches what the current code
produces.
Summary by CodeRabbit
New Features
--recompute-min-nbor-distoption for compression workflows, enabling supported PyTorch Exportable models to recalculate minimum neighbor distances from training data.Bug Fixes