Skip to content

fix(pt_expt): make graph trace samples representative - #6090

Queued
OutisLi wants to merge 2 commits into
deepmodeling:masterfrom
OutisLi:fix/graph-trace-representative-samples
Queued

OutisLi wants to merge 2 commits into
deepmodeling:masterfrom
OutisLi:fix/graph-trace-representative-samples

Conversation

@OutisLi

@OutisLi OutisLi commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Replace #6068 with a deliberately smaller change based on current master (4f78c2d0).

Current master already shares bounded, sel-independent graph trace inputs between compilation and export. The remaining gap is sample coverage: a seven-atom sample assigned types modulo the type-map length never exercises type 117 in a 118-type model, and random geometry does not guarantee an isolated owned center.

  • Keep the sample size and existing dynamic-axis strategy; place a connected three-atom cluster and an isolated owned center in the periodic sample.
  • Put the highest valid type index on a connected node, independent of type-map size.
  • Add real graph export regression coverage for changed frame/node/edge counts, including zero-edge runtime, with energy/force/virial eager parity.
  • Protect existing preparation and introspection semantics with strict-normalization, hybrid, DPA4/DPA4C dispatch, and both checkpoint-layout regressions.

No extra neighbor-contract abstraction, hot-path dispatch, graph ABI, C++ source, or metadata schema changes are introduced. Legacy sel/nnei remain available to dense consumers and existing C++ readers. DPA1 auto-sel and normalization/statistics are deliberately unchanged; removing their semantic dependence on sel belongs to #5824. This PR does not claim to complete all of #5825.

Validation

CPU, using the installed native extension:

  • 17 new graph sample/export cases, including 118 types and dynamic N/E energy/force/virial parity.
  • 5 existing graph regressions: actual DPA2 Inductor compilation, DPA1 attention 0/2 traces, DPA4 trace, dynamic-aparam export.
  • 40 checkpoint/preparation tests plus 33 subtests.
  • Native-spin traced forward/magnetic-force double-backward regression (compile backend intentionally mocked by that existing test).
  • ruff check ., ruff format ., and git diff --check.

GPU execution and a fresh C++ rebuild were not performed. Metadata compatibility is asserted, not presented as an executed C++ integration test.

Refs #5825, #5824, #5755. Supersedes #6068.

Summary by CodeRabbit

  • Tests
    • Added compatibility coverage for neighbor preparation, automatic and explicit selections, hybrid descriptors, and minimum-distance handling.
    • Added regression checks for descriptor and fitting outputs across standard and compiled checkpoints.
    • Expanded graph-tracing tests for deterministic sparse inputs, varied node/type counts, data types, canonicalization, and high-type models.
    • Added end-to-end validation confirming exported graphs preserve energy, force, and virial results across multiple inputs.

Copilot AI balanced review requested due to automatic review settings October 20, 2026 03:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5933c5eb-8d3a-4dbf-9e5c-150c52dbebc1
📥 Commits

Reviewing files that changed from the base of the PR and between 4f78c2d and 1640963.

📒 Files selected for processing: 4
  • deepmd/pt_expt/utils/serialization.py
  • source/tests/common/dpmodel/test_neighbor_preparation_compat.py
  • source/tests/pt_expt/infer/test_deep_eval_pt_checkpoint.py
  • source/tests/pt_expt/utils/test_graph_trace_samples.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The synthetic graph input builder now creates deterministic sparse fixtures and assigns the highest type index to atom 0. New tests cover sparse graph tracing and export, checkpoint introspection, and neighbor-preparation compatibility across DPA descriptors.

Changes: Sparse graph tracing and checkpoint evaluation

Layer / File(s) Summary
Deterministic sparse graph inputs
deepmd/pt_expt/utils/serialization.py
The builder creates an isolated center and connected three-atom cluster when at least four atoms are present. Atom 0 uses the highest available type index.
Sparse graph sample coverage
source/tests/pt_expt/utils/test_graph_trace_samples.py
Tests check deterministic sparse samples, high-type connectivity, canonicalization, and dtypes. They also verify that sample construction does not query dense capacities or build dense neighbor lists.
Graph export and checkpoint compatibility
source/tests/pt_expt/utils/test_graph_trace_samples.py, source/tests/pt_expt/infer/test_deep_eval_pt_checkpoint.py
Tests compare exported graph energy, force, and virial outputs with the source model. They also compare descriptor and fitting introspection outputs from graph checkpoints with dense references.

Neighbor preparation compatibility

Layer / File(s) Summary
Neighbor selection compatibility tests
source/tests/common/dpmodel/test_neighbor_preparation_compat.py
Tests cover automatic and explicit selection values, mixed hybrid descriptor handling, minimum-distance queries, descriptor construction, and input-model immutability.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: wanghan-iapcm

Merge Risk: ⚪ Minimal · up to 16409

No issue requiring a fix before merge was established. The changed tests cover zero-edge export, and the export path aligns devices for CUDA.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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 and concisely describes the main change: updating graph trace samples to be more representative, with related regression coverage.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@OutisLi
OutisLi requested a review from wanghan-iapcm October 20, 2026 03:19

@njzjz-bot njzjz-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: dot

Reviewed the four-file change at exact head 16409639b843f10491dcea9946e9c7a43bf4c009 against 4f78c2d0. No actionable production defect found in this bounded trace-sample change. The isolated center is an owned node even in the four-atom fixture; the connected cluster contains the highest valid type, while sample size, edge-count derivation, graph ABI and dense compatibility metadata remain unchanged. The added export and checkpoint tests check relevant behavior rather than introducing a second neighbor contract.

Independent verification: executed the exact NumPy geometry/type-construction statements for 288 combinations of frame count, node count, type count, cutoff and float precision. Periodic minimum-image checks confirm isolation, at least two incoming/outgoing neighbors for the highest-type node, valid type bounds and deterministic construction. The original modulo-only assignment fails the 118-type boundary control. All four changed Python files compile syntactically.

These checks do not execute Torch graph construction, export, eager/export energy-force-virial parity or native/CUDA integration: PyTorch is unavailable in this executor. The author's reported full CPU results are not independently reproduced here. Current-head Python and C++ CI are still running. The green Test CUDA workflow has both Python/C++ CUDA jobs skipped, so it is not GPU validation. No code, branch, workflow or merge changes were made.

@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.83%. Comparing base (4f78c2d) to head (1640963).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6090      +/-   ##
==========================================
- Coverage   78.08%   77.83%   -0.25%     
==========================================
  Files        1171     1171              
  Lines      141657   141661       +4     
  Branches     5072     5078       +6     
==========================================
- Hits       110615   110264     -351     
- Misses      29102    29460     +358     
+ Partials     1940     1937       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wanghan-iapcm wanghan-iapcm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a hardening of the graph trace sample coverage. Note that the end-to-end export parity test also passes on the pre-fix code, so the new structural tests pin the sample layout rather than reproduce a user-visible failure.

@OutisLi
OutisLi added this pull request to the merge queue Oct 20, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants