-
Notifications
You must be signed in to change notification settings - Fork 101
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
BUG: custom CHGNET model in PhononFlow throws jsanitize
error
#781
Comments
@naik-aakash did you get something similar recently? @badw could you list all version numbers of your different codes? I guess it is some unrelated change in another code again. We, unfortunately, have this very often. |
Hi @JaGeo, I did not encounter this yet. |
ah right,
I thought I was up to date with atomate2, so will download the latest version as well and try that |
Did not, I guess, right? |
Yes, many things changed recently. Good to check out the latest version. If it persists, we will check. |
I get the same error with 0.0.14 I have attached a tarball with a jupyter notebook that hopefully make it easier to reproduce! thanks |
Likely a recent change in one of the other packages but not atomate2. I am not sure we can get to this fast. @naik-aakash can you list the version numbers here? Maybe, that already helps. |
I am getting jsanitize error too with the example workflow:
Produces:
However the installation example is running fine:
|
Yeah, sorry. We need to investigate. But you are also invited to do so. |
Can you try downgrading to emmet-core==0.78.0rc4? E.g.,
|
for me |
Yes, it worked for me now, here is the current setup:
|
Hi @badw , after a bit more checking in detail into this issue, I think the issue is not specific to PhononFlow, but how the forcefield based jobs are generated. When using custom models, this models are also stored in the python objects and as jobflow enforces I was able to reproduce the same error using the custom model or trying to load locally saved pre-trained chgnet model, even when just when trying to run a relax job. from jobflow import run_locally
from chgnet.model import CHGNet
from atomate2.forcefields.jobs import CHGNetRelaxMaker
bulk_relax_maker=CHGNetRelaxMaker(relax_kwargs={'fmax':0.1}, steps=3,optimizer_kwargs={'model':custom_model}).make(structure=struct)
run_locally(bulk_relax_maker, create_folders=False) |
@janosh @esoteric-ephemera @matthewkuner any ideas? I am in Munich at the moment and then on Easter holiday. I will not be able to work on this at the moment. But maybe relevant for CHGNet people that the support for fine-tuned models is there as well |
I haven't tried using a custom CHGNet model before, nor am I familiar enough with CHGNet to debug this sort of issue. Maybe Janosh (already tagged) or @bowend-ucb would be able to chime in? |
I guess adding a link to the model and then loading it in the init of CHGNet instead of directly passing it, should work but would require some refactoring |
Something similar to: atomate2/src/atomate2/forcefields/jobs.py Line 301 in 33d747a
But this should probably be reviewed and merged first: #722 |
If I understand correctly, |
Hi @BowenD-UCB , I think adding an arg to StructureOptimizer so user can provide path to load locally saved model instead of only CHGNet object should help resolve the issue. I feel this will help bypass the current issue when jobflow tries to serialize the CHGNet model object, leading to jsanitize errors. |
Hi @naik-aakash The calculator class is more foundamental while relaxer should be a downstream function class. |
Overall, I think this is more of a feature request than a bug as no one has ever used CHGNet in atomate2 with an individual model. @badw do you maybe want to implement such a solution yourself and ask us or the CHGNet developers for help in case you cannot sort it with the current implementations? |
Yes, I think this should work. We just need to implement it like this. |
Just made the commit If this is not urgent, we will release it in the next release |
Thanks! This isn't particularly urgent for me, but I can play around a little bit and see what I manage! |
This may not be an issue after the refactored workflows are merged, since the interface to a custom calculator would just take JSONable args (like the class/module/callable to load through monty, and the path to a custom model file as a kwarg) |
Found the issue: emmet-core PR #944 removed the |
i will close this as the new emmet version has been released. If it shows up again, let us know |
Describe the bug
Including a custom CHGNet model when doing PhononFlow (locally) with CHGNETRelaxMaker and CHGNetStaticMaker results in an error
see full error output below
To Reproduce
This was following the jupyter notebook: https://jageo.github.io/TutorialAtomate2Forcefields/phonon.html
and then:
Expected behavior
it is also possible i have done this the wrong way....
The text was updated successfully, but these errors were encountered: