Skip to content
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

Patch emmet missing custodian #1210

Closed
wants to merge 7 commits into from
Closed

Conversation

mbruns91
Copy link

The materialsproject's emmet-core, on which we depend via mp-api, is missing to declare its dependency on custodian. This causes our CI and notebook runs to fail, with error messages like from custodian.vasp.jobs import VaspJob; ModuleNotFoundError: No module named 'custodian'. This patch provides a hotfix for this.

Maybe this shouldn't be merged, as I will also raise an issue on emmet's repo, as it's a cleaner solution if they declare this dependency explicitly.

@niklassiemer niklassiemer added the format_black reformat the code using the black standard label Nov 15, 2023
Copy link
Member

@niklassiemer niklassiemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will fix the unittests for this repo :) for the other repos, however, this will probably not yet fix the issue since they rely on the conda package if I recall correctly... thus, the feedstock would need a new build...

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too.

for the other repos, however, this will probably not yet fix the issue since they rely on the conda package if I recall correctly... thus, the feedstock would need a new build...

Yes, for sure. The feedstock will need a new release no matter what for the other repos to start working -- the only question is whether we want to make one new release (waiting for emmet-core to get a fixed release and then depend on that), or two new releases (merge and release this now, then again when emmet-core is fixed).

On the one hand, the materialsproject folks were super responsive the only other time I interacted with them, but on the other hand I'm suuuper tired of the tests failing... How about this: raise the issue on materialsproject/emmet, and if they fix it before Monday then we can stop worrying and make a single new version, but if they still haven't gotten to it then we can use the regular meeting to make sure the rest of the team is OK with a hotfix adding such an indirect dependency

@mbruns91
Copy link
Author

Sorry, I was kind of busy today due to unforeseen circumstances. To accelerate this a bit, I opened an issue in the emmet-core repo and also made a PR in the feedstock.

@mbruns91
Copy link
Author

mbruns91 commented Nov 16, 2023

This got resolved via materialsproject/emmet#884 and our CI seems to be running well again.

@mbruns91 mbruns91 closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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