Repository navigation
perf(pt): reuse SO2 residual workspace during inference - #6058
macqueen09 wants to merge 2 commits into
Conversation
Reuse owned residual storage only when weight gradients and saved layer state are not required. Preserve caller inputs and training paths. Add generated-input regressions for alias guards, supported dtypes, partial tiles and force-loss derivatives with optimizer updates.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation 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. Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled. Title check ✅ Passed The title clearly identifies the main change: reusing the SO2 residual workspace. The inference qualifier is somewhat narrow because the optimization also applies when weight gradients and saved state…
✨ Finishing Touches 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
-
- Autopilot · 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.
Comment @coderabbitai help to get the list of available commands.
njzjz-bot
left a comment
There was a problem hiding this comment.
Agent: dot
Reviewed the exact two-file delta against dbca0b1; no actionable source defect found. Reuse is restricted to internally allocated g_cur without weight gradients or retained layer state. Gated launches keep gz separate, the final launch stays out-of-place, and the residual load/output store use the same uniquely owned (focus, edge, column) tile. Caller inputs and forward-saved tensors are not mutated.
Independent CPU checks using the actual traversal and four kernel ASTs: 40 baseline/candidate cases pass across weights/keep/alpha, zero-gated and narrow/wide-BMM paths; four reversed/shuffled multi-tile fp32/fp16 comparisons pass; caller buffers remain unchanged. An intentionally unsafe gz/output alias fails the negative control. Address enumeration covers 360 supported shape/edge partitions and 71,176,320 addresses without overlap or gaps. The actual autograd-dispatch AST passes all 64 combinations: trainable/partially frozen operands, create_graph and saved-output cotangents route through unchanged training handling; second-order replay uses keep=True. Both changed files compile syntactically and the new test matrix contains 106 CUDA cases.
These are CPU semantic/address probes, not CUDA execution or Inductor functionalization validation. Torch/GPU are unavailable here. Author-reported CUDA/memcheck, optimizer equivalence and measured memory reduction remain unverified locally. The disclosed 4096-atom self-repeatability failures on both base and candidate do not establish determinism or a causal regression; no timing speedup is inferred from the near-neutral latency measurements.
At review time all seven returned build/test workflows remain action_required. No workflow activation, code modification or merge action was taken; relevant actual CUDA/compile coverage must be distinguished from skipped aggregate checks.
|
Local inference and training validation, benchmark results, and reproduction commands are documented in the PR description. Could a maintainer approve the pending runs and trigger the CUDA tests using the Test CUDA label? Please also verify that the relevant CUDA and compile tests execute rather than skip. Thanks! |
OutisLi
left a comment
There was a problem hiding this comment.
Requesting changes to the test scope and organization, not to the production optimization.
The residual-workspace reuse is a small, coherent change. The matrix operand gz remains separate, each output tile reads only its own residual elements before overwriting them, the final-layer output stays out-of-place, and weight-gradient/saved-state traversals retain their original buffers. I did not find an actionable production correctness defect within this review. Please keep that implementation focused; it needs neither a new acceleration level nor a new autotuning mechanism.
Proposed test simplification
1. Replace the 90-case Cartesian product with a representative case list.
The main parameterization expands to 5 shapes × 3 edge counts × 3 layer counts × 2 alpha settings. Thirty cases have zero edges and return before the changed traversal; another twenty have nonzero edges but no gated layer, so the aliased gated update is never exercised. These boundaries deserve coverage, but not the full cross product. Even empty-edge cases still construct and snapshot weights.
For example, one parameterization over the following seven cases retains all five original shape profiles, both alpha modes, zero/partial-edge workloads, zero/single/multiple gated layers, and narrow/wide gate-projection paths:
# (lmax, focus, width, edges, gates, apply_alpha)
CASES = [
(2, 2, 32, 0, 1, True), # Empty-edge return.
(2, 2, 32, 37, 0, True), # Final layer only.
(2, 2, 32, 37, 1, False), # Single gated layer without alpha.
(3, 2, 32, 129, 3, True), # Multi-layer recurrence and partial tiles.
(3, 1, 64, 37, 1, True), # Single focus, wider narrow-channel path.
(6, 2, 96, 37, 3, False), # BMM gate path, non-power-of-two width.
(6, 3, 128, 129, 1, True), # BMM gate path and multiple focuses.
]Keep the existing numerical, output-shape and caller-input-preservation assertions. This list is a concrete starting point, not a hard case-count target: add or adjust a case only when it covers a distinct ownership/branch/layout condition missing from the others.
2. Keep the four workspace-identity guard cases.
test_reuse_only_without_weights_or_saved_state directly tests the intended change and has a useful baseline-fails/candidate-passes assertion. Preserve all four with_weights/keep combinations, the out-of-place final layer, and the assertion that gz never aliases the output.
3. Keep the six dtype/alpha alias-versus-separate comparisons.
The three supported dtypes × two alpha settings in test_reuse_matches_separate_workspace cover a distinct contract. Preserve their numerical and input-preservation checks rather than weakening tolerances to reduce work.
4. Separate training dispatch protection from broad training equivalence.
The six frozen/trainable-weight cases each repeat first/second derivatives and two parameter updates. Much of this checks paths deliberately excluded from the changed reuse branch.
Use lightweight dispatch checks for trainable, partially frozen and fully frozen weights, especially create_graph=True, to establish that they select the protected training/saved-state path. Reuse existing model-level gradient/training regressions where they actually cover the same contract. Those existing tests do not automatically prove every frozen-weight direct-stack case, so identify any genuinely missing coverage before removing it. If a distinct numerical higher-order path still needs protection, retain the smallest representative single-step check for that path, rather than automatically crossing every weight-freezing mode with both widths and repeating optimizer updates.
This yields 17 focused core cases (7 numerical + 4 ownership guards + 6 dtype/alpha checks), plus any justified unique derivative coverage. Please explain that coverage mapping briefly in the updated PR. Do not add a new timing study, production-size benchmark, or full-model GPU test suite just to validate this test reorganization.
Verification and limits
The prior review's 11 small CPU-interpreter comparisons of the exact base/head traversal passed bitwise, preserved caller inputs, and confirmed the expected buffer identities. Ruff and whitespace checks passed. No GPU execution, test-runtime benchmark or independent reproduction of the author's GPU memory measurements was performed. The test-cost concern above is based on redundant paths and compilation/workload structure, not a claimed measured CI speedup.
Please retain the production change and make the regression suite proportionate to its contract before merging.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6058 +/- ##
==========================================
- Coverage 77.79% 77.55% -0.25%
==========================================
Files 1171 1171
Lines 141562 141564 +2
Branches 5066 5065 -1
==========================================
- Hits 110135 109784 -351
- Misses 29550 29905 +355
+ Partials 1877 1875 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Reuse the internally owned residual-gradient workspace in the SO2 mixing-stack
backward traversal when neither weight gradients nor saved per-layer state are
needed. This removes one edge-sized allocation without changing kernel
arithmetic, model parameters, precision settings or public APIs.
This is an isolated workspace change. It does not include compiler scheduling
passes, the inference-compiler flag from #6056, additional fusion, checkpoint
changes, or local benchmark infrastructure.
Why the alias is safe
The gated-layer backward GEMM computes
residual + gz @ W.T. Its matrix inputgzremains separate from the output. Each output tile reads only its matchingresidual elements before writing those same locations; distinct programs do not
read one another's residual tiles. The final-layer GEMM remains out-of-place.
Only buffers allocated inside this traversal are reused. Caller inputs and
forward-saved tensors are not mutated, so the operator's
mutates_args=()contract is unchanged. The weight-gradient and saved-state traversals keep their
original allocation paths. The guard uses actual gradient/state requirements,
not simply
model.eval(), which matters for frozen-weight and force-loss cases.The generated full-model graph was checked as well: four gated GEMM calls alias
residual/output, while both final-layer calls and every matrix-input/output pair
remain separate.
Scope
deepmd/pt_expt/kernels/triton/sezm/so2_value_path.py: local allocation changeand the alias-safety explanation.
source/tests/pt_expt/kernels/test_so2_workspace.py: generated-input CUDAregressions, with no checkpoint or machine-specific dependencies.
The new tests cover zero/partial edge tiles, multiple layer counts, narrow and
BMM gate-projection widths, FP32/FP16/BF16, input preservation, actual workspace
identity, and first/second derivatives plus two updates with trainable,
partially frozen and frozen weights. The allocation-identity test is designed
to fail on the unmodified base and pass with this change.
Full-model measurements
Base:
dbca0b1cfabc9be166feffcbbc93e9c833603aca.RTX 5090, driver 595.91.07; Python 3.11.12, PyTorch 2.11.0+cu128, Triton 3.6.0,
Warp 1.16.0. Same DPA4-Neo checkpoint and normal ASE/DP energy/forces/virial/stress
evaluation, including neighbor construction.
DP_COMPILE_INFER=1,DP_TRITON_INFER=2, TF32/AMP off, one host thread,PYTORCH_ALLOC_CONF=expandable_segments:True.Diamond C, lattice 3.567 Angstrom, seed 0, position noise sigma 0.02 Angstrom.
Each process: 8 warmup calls including the first, 30 synchronized wall-clock
samples, and a separate 30-call memory replay. Three independent processes for
4096 atoms; two for 1000. Timing values below are medians of process quantiles;
memory values are the maximum measured peaks. GB means 1e9 bytes.
This is a memory optimization, not a claim of significant speedup. The small
observed 1000-atom latency increase is disclosed rather than classified as a
speed improvement. The 4096-atom allocation reduction is exactly 1,656,755,200
bytes, matching one
[2, 647170, 320]FP32 workspace. First-call compilation isexcluded from steady timing but included in the full memory window. GPU clocks
were not locked, and these short research runs are not a long-duration
performance acceptance test.
Numerical validation and limitations
Full-model comparison uses all energy, force, virial and stress elements with
finite-value checks and
abs(actual-reference) <= 1e-5 + 1e-4*abs(reference).independent processes; all 10 three-frame self-repeatability rounds pass.
self-repeatability is not universally satisfied: candidate 6/15 and base
9/15 rounds pass. The failed checks are the third frame's virial. Baseline
cross-process comparisons also fail that component in both repeat runs.
No tolerance was relaxed and no output was excluded. These observations do
not establish either a fix or a regression in global numerical determinism.
An additional independent dense eager GPU reference, with compilation and
Triton/CUDA fast paths disabled, also passes for both 1000-atom candidate runs
and both base runs at the same elementwise thresholds. This was one three-frame
reference evaluation, not an exhaustive determinism test.
Local test evidence
hooks passed.
127 subtests passed. All nine
TestSeZMModelCompiletests passed. The fiveskips require DPA4 native CUDA operators absent from the installed library.
expected; the candidate passes.
the installed customized-op library was built without GPU support. This is
an environment limitation, not a passing test or a newly introduced failure.
The local source-selection harness used the new Python checkout with existing
native libraries from the measured environment. This does not claim a fresh
C++/wheel build or TensorFlow/JAX/Paddle validation. Other GPU architectures and
Torch 2.12/2.13 are not locally validated here. A CUDA-native rebuild is needed
to cover the missing native paths; this PR does not claim that every local
or required CI check is green.
Additional cross-checkout training equivalence
Separate BASE and candidate processes, identical generated inputs/initialization,
TF32 disabled, same GPU/environment:
2516/2516 finite arrays pass the same
atol=1e-5, rtol=1e-4elementwise comparison. 1384 arrays are bitwise identical; global maximum
absolute difference is 3.725290298461914e-9; maximum error/tolerance ratio is
0.0003452008823. This is bounded multi-step equivalence evidence, not a
claim that arbitrary long training is bitwise identical.
The normal
model.train()route is excluded from this inference stack bythe existing
not self.trainingguard inso2.py. Direct-stack force-losstests additionally cover trainable, partially frozen and frozen operands:
observed backward calls requiring weight gradients retain separate workspace,
and calls saving second-order state retain the saved-state path. No public
training flags or APIs change.
An additional tiny-model configuration combining Triton training with compilation
triggered a Torch 2.11 Inductor
replace_by_examplepattern-matcher assertionon the unmodified BASE. The final comparison therefore explicitly separates
eager Triton training from default compiled training, rather than claiming that
combined configuration passed. Candidate equivalence for that failing BASE
configuration is not established. The installed native library also lacks CUDA
operators; local CUDA toolkit 13.2 does not match this Torch cu128 build, and a
fresh matching-toolkit CUDA build remains for CI/a suitable environment.
Reproduction / reviewer requests
Run the new test file using the normal project installation. It generates all
operands and does not download a model. Also run the original
TestSeZMModelCompile,TestSeZMTritonValuePath, descriptor and training-pathregressions. The commands and standalone scripts below provide reproduction without
committing machine-specific benchmark infrastructure.
The exact checkpoint is
DPA4-Neo-OMat24-v20260627.pt, SHA2568d8b2b02b9575cb9e0af420cc1e8b1d9ffcbc04752c2203d29f48ccd0cd5f07d;it is not bundled. Exact full-model numbers require an authorized copy.
Please trigger the upstream CUDA workflow and verify that relevant compile
tests actually run rather than skip under the workflow's Torch version. Local
results do not replace required status checks, maintainer review, or the merge
queue.
Reproducible commands
Install each revision following the project's CUDA/PyTorch build instructions
in separate environments, then verify
deepmd.__file__andtorch.__version__. Do not accidentally import another editable checkout.Tests below need no checkpoint:
Memcheck does not prove absence of all global-memory races; see the tile-level
read/write argument above.
For full-model reproduction, save the following script outside the repository.
Run serially on an otherwise idle GPU, with a fresh output directory and cache
for every invocation. With BASE installed/imported first:
CUDA_VISIBLE_DEVICES=0 MODEL=/path/to/authorized/model.pt OUTPUT=/scratch/base-r0 \ TORCHINDUCTOR_CACHE_DIR=/scratch/cache-base-r0 NREP=8 \ python /scratch/public_benchmark.py # Switch to the candidate environment/checkout: CUDA_VISIBLE_DEVICES=0 MODEL=/path/to/authorized/model.pt OUTPUT=/scratch/candidate-r0 \ TORCHINDUCTOR_CACHE_DIR=/scratch/cache-candidate-r0 NREP=8 \ REFERENCE=/scratch/base-r0/outputs.npz python /scratch/public_benchmark.pyRepeat 3 processes for
NREP=8(4096 atoms), 2 forNREP=5(1000 atoms),using different paths; interleave/reverse order when repeating. Inspect every
JSON comparison and finite check. This portable script reproduces normal API
timing/memory and cross-revision checks; the reported main experiment additionally
ran five three-frame self-repeatability rounds per process and audited generated
wrappers. Those extra diagnostics are not implemented by this short script.
Do not use
torch.inference_mode(): conservative force evaluation needsdifferentiation. No private compiler hooks are needed for this change.
Standalone public_benchmark.py (normal DP / ASE APIs)
Supplementary multi-step training comparison (outside the production branch)
Save this as
training_equivalence.pyoutside the repository. Run once fromeach installed checkout's root in a fresh process/cache:
CUDA_VISIBLE_DEVICES=0 PYTHONPATH="$PWD" TRAIN_OUTPUT=/scratch/train-base TORCHINDUCTOR_CACHE_DIR=/scratch/train-cache-base python /scratch/training_equivalence.py.Use distinct candidate output/cache paths. The code below only makes GPU
selection configurable relative to the measured local script; computations are
the same. It uses existing repository test helpers and generated data; no
pretrained model is required.
Compare the two generated archives with shape/key/dtype and finite checks:
Contribution / merge checklist
benchmark caches, raw customer data, or machine-specific files committed.
Prepared against the contributing guide
and linked coding conventions
and CI documentation.
Current default-branch rules require seven check contexts (C++ tests, C++ build,
C-library build, Python tests, wheel builds, CUDA aggregate, pre-commit.ci),
at least one approval, resolved threads, and the squash merge queue.
The CUDA workflow needs maintainer activation (e.g. the
Test CUDAlabel);a passing aggregate with skipped GPU jobs is not CUDA validation. Please also
check Torch-version gates on compile tests. No claim is made that local tests
replace these server-side requirements.
Summary by CodeRabbit