Skip to content

update mace wrapper to enable training - #97

Merged
laserkelvin merged 3 commits into
NVIDIA:mainfrom
ys-teh:feature/mace_wrapper_for_training
May 20, 2026
Merged

update mace wrapper to enable training#97
laserkelvin merged 3 commits into
NVIDIA:mainfrom
ys-teh:feature/mace_wrapper_for_training

Conversation

@ys-teh

@ys-teh ys-teh commented May 20, 2026

Copy link
Copy Markdown
Collaborator

ALCHEMI Toolkit Pull Request

Description

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Performance improvement
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or infrastructure change

Related Issues

Changes Made

  • Changed training=False to training=self.training in MACEWrapper using the PyTorch's flag to be compatible with the upcoming model training workflow.
  • Changed mock model used for MACE in unit test to be dependent on a trainable parameter so train mode test makes sense.
  • Added a unit test to make sure that optimization now works when using MACEWrapper.

Testing

  • Unit tests pass locally (make pytest)
  • Linting passes (make lint)
  • New tests added for new functionality meets coverage expectations?

Checklist

  • I have read and understand the Contributing Guidelines
  • I have updated the CHANGELOG.md
  • I have performed a self-review of my code
  • I have added docstrings to new functions/classes
  • I have updated the documentation (if applicable)

Additional Notes

Tip

This repository uses Greptile, an AI code review service, to help conduct
pull request reviews. We encourage contributors to read and consider suggestions
made by Greptile, but note that human maintainers will provide the necessary
reviews for merging: Greptile's comments are not a qualitative judgement
of your code, nor is it an indication that the PR will be accepted/rejected.
We encourage the use of emoji reactions to Greptile comments, depending on
their usefulness and accuracy.

Signed-off-by: Ying Shi Teh <yteh@nvidia.com>
@ys-teh
ys-teh requested a review from laserkelvin May 20, 2026 18:40
@copy-pr-bot

copy-pr-bot Bot commented May 20, 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 May 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR enables training support in MACEWrapper by replacing the hardcoded training=False with self.training in the forward method, and updates MockMACEModel to wire a learnable parameter through its forward pass so the new optimizer round-trip test is meaningful.

  • forward now forwards self.training to the inner MACE model, making dropout/BN-style layers respect the wrapper's current mode during fine-tuning.
  • compute_embeddings still passes training=False unconditionally — this is a latent inconsistency if the wrapper is used in train mode for embedding-based training pipelines.
  • The new test_train_mode_works_with_optimizer test verifies end-to-end that a gradient step updates the model parameter when the wrapper is in train mode.

Important Files Changed

Filename Overview
nvalchemi/models/mace.py Propagates self.training to the inner MACE model in forward; compute_embeddings still hardcodes training=False, which is inconsistent. The from_checkpoint(compile_model=True) path returns a wrapper in train mode despite calling model.eval() internally (flagged in a prior review).
test/models/test_mace.py Adds test_train_mode_works_with_optimizer to verify gradients flow through the wrapper; updates MockMACEModel to wire a learnable parameter into the forward computation so the test is meaningful.

Reviews (2): Last reviewed commit: "add mace train mode test" | Re-trigger Greptile

@laserkelvin laserkelvin added the enhancement New feature or request label May 20, 2026
ys-teh added 2 commits May 20, 2026 18:58
Signed-off-by: Ying Shi Teh <yteh@nvidia.com>
Signed-off-by: Ying Shi Teh <yteh@nvidia.com>
@dallasfoster

Copy link
Copy Markdown
Collaborator

/ok to test 1397531

@laserkelvin laserkelvin 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

@laserkelvin
laserkelvin added this pull request to the merge queue May 20, 2026
Merged via the queue into NVIDIA:main with commit ae51207 May 20, 2026
5 checks passed
@ys-teh
ys-teh deleted the feature/mace_wrapper_for_training branch July 21, 2026 21:12
@nikitafedik nikitafedik mentioned this pull request Jul 31, 2026
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants