pyiron/pyiron_atomistics

[conda package] `emmet` cannot find `custodian` when `pyiron_atomistics` is imported

Closed this issue · 8 comments

mbruns91 commented

The daily workflows for pyiron_atomistics (I also look at pyiron_workflow, pyiron and pyiron_base) are failing with errors like from custodian.vasp.jobs import VaspJob; ModuleNotFoundError: No module named 'custodian'.

Following the suggestion of @liamhuber , I started to hunt this down. Please feel free to point out anything that screams "well that's not really how things work" to your face, as I really want to get a detailed understanding.

  • I checked CI and (if applicable) notebook runs for pyiron, pyiron_base, pyiron_atomistics and pyiron_workflow. Errors are indeed consistently caused by emmet failing to find custodian.
  • I looked at the recipe's meta.yaml at emmet-core-feedstock. No mention of custodian there.
  • I looked at this log from pyiron_atomistics. Indeed, they import via from custodian.vasp.jobs ... and thus depend implicitly on custodian.
  • In the very same log, you can see that emmet-core version 0.72.20 is installed. From the error mentioned before, you can see the already mentioned dependency on emmet.core via mp-api. mp-api is an explicit dependency of the pyiron_atomistics package. In the environemnt of the failed run, mp-api =0.37.5 is installed. In mp-api's feedstock you can see the explicit dependence on emmet-core but, of course, non on custodian.

So - if I'm understanding all of this correctly - the solution would be to put custodian as an explicit dependency in the meta.yaml in emmet-core-feedstock, right? At this point I just don't know where to raise an issue for this: in the feedstock repo? In the emmet-core repo?

liamhuber commented

@mbruns91 ok so the dependency is just totally missing then! That's a bit simpler, thankfully, than something complex with version pin gone awry.

We can temporarily patch this by adding custodian to our dependencies here. It might be nice to make a PR for this even if we don't ever merge it, just as a sanity check/demonstration that this solves the problem.

Then I recommend raising an issue right on the emmet repo (this will probably get more/faster eyes than something on the feedstock). If the above PR works solves our problem as expected then this can also be linked as a demo that adding the dependency is a good solution.

mbruns91 commented

Ok, I'll do it this way.

Regarding the patch: Where is the right place to fix this? Shall I just add a line custodian==2023.10.09 to setup.py like in the branch associated with this issue? (pyiron_atomistics==0.3.5 and custodian==2023.10.09 are both the most recent versions and work together)

mbruns91 commented

BTW: A similar patch has then to be supplied to the other repos as well.
I'd suggest to first do this here, see if out CI and notebooks run well, do the PR on emmet-core and see how fast they are with fixing. If they take too long, we can apply the path wherever necessary.

niklassiemer commented

Exactly the way to go forward! We need to include the custodian in the environment.yml files. They are responsible to build our environment for the tests.
If this is working, you could also directly fork the feedstock from emmet-core, add the dependency in the recipe, and start a PR.

  • 👍1
liamhuber commented

Also I think we only need to patch it here; I'm pretty sure our other repos only run into this because of their dependence on this repo

  • 👍1
niklassiemer commented

Which, however, means to fix it on the pyiron_atomistics-feedstock repo. I.e. another build with modified dependencies in the recipe

liamhuber commented

Which, however, means to fix it on the pyiron_atomistics-feedstock repo. I.e. another build with modified dependencies in the recipe

Jan got Greyskull running on pyiron_atomistics-feedstock, so the addition to setup.py in #1210 and the current bot behaviour should mean that at least this is all fully automated after we make a new release on the main repo. As I mentioned in the PR, it's really just a question whether we want to make two new releases because of this (hotfix+proper emmet update), or wait on materialsproject to fix it upstream and only release for the new emmet version (well, I guess for the new mp_api version, as the new version pins trickle down).

  • 👍1
liamhuber commented

Apparently the custodian dependency was a mistake and is removed. Since all the conda forge dependencies are >=, now that the new version of emmet is actually available, tests seem to be passing again, as evidenced in #1203 where I re-ran the tests.