Skip to content

Avoid autograd leaves for energy-only pipelines#107

Open
kiwigitops wants to merge 2 commits into
NVIDIA:mainfrom
kiwigitops:fix-energy-only-autograd-inputs
Open

Avoid autograd leaves for energy-only pipelines#107
kiwigitops wants to merge 2 commits into
NVIDIA:mainfrom
kiwigitops:fix-energy-only-autograd-inputs

Conversation

@kiwigitops

Copy link
Copy Markdown

Summary

  • avoid preparing autograd leaves for autograd pipeline groups when only energy is requested
  • add a regression that keeps energy-only training backpropagating to model parameters without marking positions as requiring gradients

Closes #100.

Testing

  • reproduced the issue snippet before the fix
  • python -m pytest test/models/test_pipeline.py::TestPipelineAutogradGroup -q
  • python -m py_compile nvalchemi\models\pipeline.py test\models\test_pipeline.py
  • git diff --check

@copy-pr-bot

copy-pr-bot Bot commented Jun 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps

greptile-apps Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR avoids the unnecessary overhead of marking position tensors as autograd leaves when the pipeline is only asked to produce energies, by gating the grad_keys collection loop behind if requested_derivatives. A companion regression test confirms that energy-only training still backpropagates correctly through model parameters (e.g. scale) without promoting positions to autograd leaves.

  • nvalchemi/models/pipeline.py: A two-line guard (if requested_derivatives:) wraps the inner loop that collects autograd_inputs for use_autograd=True groups, so _prepare_autograd_leaves receives an empty set and skips the detach/requires_grad_(True) dance when no derivatives are requested.
  • test/models/test_pipeline.py: Adds a positions_requires_grad_seen sentinel to MockTrainableAutogradEnergyModel and a new test that asserts both that positions never saw requires_grad=True and that scale.grad is non-zero after an energy-only backward pass.

Important Files Changed

Filename Overview
nvalchemi/models/pipeline.py Adds a guard around the autograd-input collection loop so positions (and other autograd_inputs) are not promoted to autograd leaves when only energy is requested; leaves all other paths unchanged.
test/models/test_pipeline.py Adds a side-channel field to MockTrainableAutogradEnergyModel to record whether positions required grad during forward, and a new regression test that verifies energy-only training both skips the leaf preparation and still propagates gradients to model parameters.

Reviews (2): Last reviewed commit: "Strengthen energy-only autograd test" | Re-trigger Greptile

Comment thread test/models/test_pipeline.py Outdated
Comment on lines +526 to +527
assert model.scale.grad is not None
assert model.positions_requires_grad_seen is False

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.

P2 The new test checks that scale.grad is not None but never verifies it is non-zero. The analogous test_training_preserves_force_graph_for_backward test also asserts model.scale.grad.abs() > 0, which catches the case where the backward pass runs but produces a trivially zero gradient (e.g., if the energy output is constant w.r.t. scale). Without this check the regression only proves a gradient object exists, not that the training signal flows correctly.

Suggested change
assert model.scale.grad is not None
assert model.positions_requires_grad_seen is False
assert model.scale.grad is not None
assert model.scale.grad.abs() > 0
assert model.positions_requires_grad_seen is False

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread test/models/test_pipeline.py Outdated
out["energy"].sum().backward()

assert model.scale.grad is not None
assert model.positions_requires_grad_seen is False

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.

P2 Using is False for a boolean comparison is unconventional in Python and can surprise readers who associate is with identity checks on objects. tensor.requires_grad always returns a Python bool singleton so both forms are equivalent in practice, but the negation form communicates intent more clearly.

Suggested change
assert model.positions_requires_grad_seen is False
assert not model.positions_requires_grad_seen

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@kiwigitops

Copy link
Copy Markdown
Author

Updated the regression test to assert a non-zero parameter gradient and use the clearer boolean assertion. Verified with python -m pytest test/models/test_pipeline.py -q.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛[BUG]: Default derivative function set will include positions if energies is specified

1 participant