-
Notifications
You must be signed in to change notification settings - Fork 46
Fixing model loading logic (names and cache dir) for fairchem models #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fixing model loading logic (names and cache dir) for fairchem models #278
Conversation
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]>
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 |
@CompRhys thanks for your quick answer! |
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. |
Tried to implement it as you suggested while keeping the cache dir option which is None by default. |
Made a few tweaks for better back compatibility and to get the tests to work. |
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. |
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 validmodel_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, optionalmodel_cache_dir
string parameter that is passed to theget_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: