Skip to content

Conversation

niklashoelter
Copy link

@niklashoelter niklashoelter commented Oct 7, 2025

Previously: model (str | Path | None) was passed as to fairchem's get_predict_unit - although this is expecting a model name and thereby throws an error even if a correct model path is set.

According to Fairchem, when loading a cached model, the cache_dir parameter needs to be set in addition to a valid model_name. This is fixed here.

Summary

According to the fairchem's pretrained_mlip.get_predict_unit logic, I made the model_name a required parameter when initalizing a FairChemModel object and added a second, optional model_cache_dir string parameter that is passed to the get_predict_unit function if set.

As this is my first contribution, hope that I did not forget anything!

Checklist

Before a pull request can be merged, the following items must be checked:

  • Doc strings have been added in the Google docstring format.
  • Run ruff on your code.
  • Tests have been added for any new functionality or bug fixes.

previously: model (str | Path | None) was passed as to fairchem's get_predict_unit - although this is expecting a model name.

When loading a cached model, the cache_dir parameter needs to be set in addition to a valid model_name (one of the available ones). This is fixed here.

Signed-off-by: Niklas Hölter <[email protected]>
@CompRhys
Copy link
Member

CompRhys commented Oct 7, 2025

Thanks for contributing!

I would need a bit more time to dig into what's happened here but this may be because of #211 and #270 where FairChemModel now corresponds to the v2 fairchem library. Older fairchem models can be accessed in the farichem_legacy.py / FairChemV1Model. Does just swapping to using FairChemV1Model get the behavior you want?

@niklashoelter
Copy link
Author

@CompRhys thanks for your quick answer!
I did not check with the v1 fairchem models as I am relying on the UMA ones. The simple fix from my branch however worked for me.

@CompRhys
Copy link
Member

CompRhys commented Oct 8, 2025

I think the mistake is that we need something along the lines of https://github.com/facebookresearch/fairchem/blob/dcd772e7921969643dc8ff71f2430326e2afad57/src/fairchem/core/calculate/ase_calculator.py#L132-L149. We would still want to allow users to load checkpoints from paths even if we expect majority to reuse models released by meta.

@niklashoelter
Copy link
Author

Tried to implement it as you suggested while keeping the cache dir option which is None by default.

@CompRhys
Copy link
Member

CompRhys commented Oct 8, 2025

Made a few tweaks for better back compatibility and to get the tests to work.

@CompRhys CompRhys closed this Oct 8, 2025
@CompRhys CompRhys reopened this Oct 8, 2025
@CompRhys
Copy link
Member

CompRhys commented Oct 8, 2025

Fairchem tests fail because I believe your branch won't have access to the HF_TOKEN we have in the repo secrets. GraphPES errors are known to be flaky.

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.

2 participants