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

Install optional boltztrap2 and pygraphviz in CI #3786

Merged
merged 58 commits into from
Aug 6, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Apr 26, 2024

Summary

  • Install some optional dependencies in CI, to fix [Dev] Multiple dependencies for CI missing #3684.
  • Fix patch for bader_caller (setting PATH to empty is easier than changing the patch location, and serves the same result where bader cannot be found in PATH)

Partially installed on Ubuntu

@janosh
Copy link
Member

janosh commented May 7, 2024

do we want to merge this as partially complete and leave some of the todos for later?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented May 7, 2024

Thanks for asking. I was facing some resistance while trying to install them (maybe that's the reason they're not installed in CI). And what make things worse is that I never use any of these.
Some help would be appreciated:

UPDATE: boltztrap2 installation doesn't look quite reliable for some reason? It used to work just fine, but currently show some installation errors. We would have to drop it if so.

@DanielYang59 DanielYang59 marked this pull request as ready for review August 5, 2024 09:56
@DanielYang59
Copy link
Contributor Author

@janosh Perhaps we could merge this as partially finished (as you suggested) and I would work towards the remaining parts shortly. The CI test failure is reported in #3969 and would be fixed by #3972, thanks!

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks @DanielYang59! 👍

@janosh janosh enabled auto-merge (squash) August 5, 2024 10:54
auto-merge was automatically disabled August 5, 2024 11:28

Head branch was pushed to by a user without write access

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Aug 6, 2024

Regarding the installation of BolzTraP2 on Windows, I'm afraid it wouldn't be an easy fix owing to the difference in cmake behavior. Let's install it on Linux only for now until we have a solution.

I would look into why uv failed to install it on Linux later.

@janosh janosh added tests Issues with or changes to the pymatgen test suite ci Continuous integration pkg Package health and distribution related stuff labels Aug 6, 2024
@janosh janosh merged commit 7a01f3c into materialsproject:master Aug 6, 2024
33 checks passed
@DanielYang59 DanielYang59 deleted the install-opt-dep branch August 6, 2024 14:59
@@ -933,7 +933,7 @@ def draw_graph_to_file(
d["label"] = f"{d['weight']:.2f} {units}"

# update edge with our new style attributes
g.edges[u, v, k] |= d
g.edges[u, v, k].update(d)
Copy link
Contributor Author

@DanielYang59 DanielYang59 Aug 6, 2024

Choose a reason for hiding this comment

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

@janosh Forgot to mention this, I have to revert your update -> |= change here c67abbc because it seems to break unit test.

I haven't got a chance to look closer yet, perhaps the operator is not well defined for edges

Copy link
Member

Choose a reason for hiding this comment

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

yeah i noticed that, that was sloppy refactoring on my part. thanks for fixing!

perhaps the operator is not well defined for edges

that's very likely correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was sloppy refactoring on my part.

pygraphviz was not installed in CI at that time, so there's no way for you to catch that :)

Copy link
Member

@janosh janosh Aug 6, 2024

Choose a reason for hiding this comment

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

no, but i could have noticed that g.edges[u, v, k] is unlikely to be a dict in which case the first thought is to check whether the class in question implements __or__. (if not, |= is undefined behavior)

@DanielYang59 DanielYang59 changed the title Install some optional dependencies in CI Install optional boltztrap2 and pygraphviz in CI Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration pkg Package health and distribution related stuff tests Issues with or changes to the pymatgen test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dev] Multiple dependencies for CI missing
3 participants