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

Make species hash and coord hash optional in MoleculeMetadata #1151

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Nov 29, 2024

This PR closes #1149 by making the species hash and coord hash fields optional in MoleculeMetadata, only generating them if OpenBabel is installed. In return, it is possible to generate MoleculeMetadata without relying on OpenBabel.

An alternate approach was given in #1150 where I used JMolNN as a fallbak option in place of OpenBabelNN, but I feel like the current PR is more suitable because it might be difficult to debug a scenario where a change in the environment's dependencies yields a different hash.

To reiterate what I mention in the issue report, the motivation behind this PR is that OpenBabel is a really annoying dependency to require since it is not on PyPI. This means any workflow infrastructure wanting to rely on MoleculeMetadata must rely on the user to install OpenBabel from source or Conda unless this PR is merged.

This PR closes materialsproject#1149 by making the species hash and coord hash fields optional in `MoleculeMetadata`, only generating them if OpenBabel is installed. In return, it is possible to generate `MoleculeMetadata` without relying on OpenBabel.
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.41%. Comparing base (a705a61) to head (522c17c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
emmet-core/emmet/core/structure.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1151      +/-   ##
==========================================
- Coverage   90.42%   90.41%   -0.01%     
==========================================
  Files         147      147              
  Lines       14448    14455       +7     
==========================================
+ Hits        13065    13070       +5     
- Misses       1383     1385       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Andrew-S-Rosen
Copy link
Member Author

Also, if approved, would it be possible to get a version release (or pre-release) after this PR? 🙏

@tsmathis
Copy link
Collaborator

tsmathis commented Dec 2, 2024

Thanks @Andrew-S-Rosen, this seems reasonable. Happy to merge this in if you're satisfied?

Will also get a release out ASAP as well

@tsmathis tsmathis closed this Dec 2, 2024
@tsmathis tsmathis reopened this Dec 2, 2024
@Andrew-S-Rosen
Copy link
Member Author

Yup that'd be great!

@tsmathis tsmathis merged commit faf1592 into materialsproject:main Dec 2, 2024
16 checks passed
@Andrew-S-Rosen
Copy link
Member Author

Thank you, @tsmathis!!!

@tsmathis
Copy link
Collaborator

tsmathis commented Dec 3, 2024

@Andrew-S-Rosen, we're working through a deployment for a full emmet release, but for now try using the 0.84.3rc6 pre-release and let us know if anything isn't working on your end!

@Andrew-S-Rosen
Copy link
Member Author

Thanks! Seems to work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Make OpenBabel optional for molecule metadata
3 participants