use model chunking on all paths - #133
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A critical gradient regression and two moderate compatibility/feature-handling issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends memory-aware model chunking across XC integration, response, and CPU/GPU nuclear-gradient paths.
Changes:
- Unifies dense and screened feature evaluation.
- Adds feature planning and chunked gradient assembly.
- Updates tests and examples for the new APIs.
File summaries
| File | Review |
|---|---|
skala/tests/test_xc_integrator.py |
Updates integrator mocks and expectations. |
skala/tests/test_pyscf_gradients.py |
Migrates CPU gradient tests. |
skala/tests/test_model_chunking.py |
Tests chunked feature gradients. |
skala/tests/test_gpu4pyscf_gradients.py |
Migrates GPU gradient tests. |
skala/tests/test_evaluation.py |
Tests feature-spec unions. |
skala/tests/test_ao_screening.py |
Expands dense and screened coverage. |
skala/src/skala/pyscf/xc_integrator.py |
Unifies chunked integration and response paths. |
skala/src/skala/pyscf/model_chunking.py |
Adds feature plans and gradient chunking. Critical: disconnected or constant outputs can now fail instead of returning zero gradients. Moderate: bound-shape-only specifications can raise KeyError. |
skala/src/skala/pyscf/gradients.py |
Uses chunked CPU gradient evaluation. |
skala/src/skala/pyscf/features.py |
Removes legacy feature generation. Moderate: removing public generate_features breaks downstream imports without compatibility handling. |
skala/src/skala/pyscf/evaluation.py |
Adds FeatureSpec unions. |
skala/src/skala/pyscf/ao_evaluation.py |
Returns packed raw AO features. |
skala/src/skala/gpu4pyscf/gradients.py |
Uses chunked GPU gradient evaluation. |
model/examples/cpp/cpp_integration/prepare_inputs.py |
Migrates example input preparation. |
Review details
Suppressed comments (1)
skala/src/skala/pyscf/model_chunking.py:420
- The chunked branch has the same regression for constant energies or unused declared inputs:
torch.autograd.gradraises rather than returning the zero feature derivatives that the oldfeature_derivativespath guaranteed. This is especially relevant whennuc_grad_featsis an explicit superset. Add the constant guard plusallow_unused=Trueand zero-fill disconnected inputs.
energy_chunk = functional.get_exc(chunk_features)
local_gradients = torch.autograd.grad(energy_chunk, tuple(chunk_inputs))
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The CPU nuclear-gradient path does not honor configured memory limits, allowing chunks to exceed the requested budget.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Balanced
fa77708 to
fe6c735
Compare
a7906bb to
2fad690
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Restricted density handling is incorrect, and CUDA chunk sizing can exceed available VRAM.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
These are only minor version upgrades + the addition of pyspy.
| class ModelFeatureChunker: | ||
| """Prepare atom-aligned model inputs from globally evaluated AO features. | ||
|
|
||
| ``atom_major_raw_features`` must use the packed channel layout derived from |
There was a problem hiding this comment.
delete paragraph
Adding py-spy in 2bfe42f swept in a full environment re-solve. It changed 41 package versions (GCC 16.1->16.2, libcxx/llvm-openmp 22->23, CPython patch levels, libcudnn, ucx, rdma-core, huggingface_hub) and rebuilt 51 more against the new toolchain. None of that was intended by this PR, and it makes any CI failure ambiguous between the toolchain bump and the chunking changes. Regenerate the lockfile with a minimal `pixi add`. It resolves the same py-spy build (0.4.2-hbe0e3b8_1) against an unchanged pixi.toml, and shrinks the lockfile diff from +2781/-2787 to +20/-0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This PR extends memory-aware model chunking from screened first-order XC evaluation to every relevant model execution path:
This reduces the memory consumption on all levels with