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

Job to retrieve Structure from MP API at run-time #176

Merged
merged 12 commits into from
Oct 5, 2022
Merged

Job to retrieve Structure from MP API at run-time #176

merged 12 commits into from
Oct 5, 2022

Conversation

mkhorton
Copy link
Member

@mkhorton mkhorton commented Oct 5, 2022

Summary

@utf would you be ok with this being added, or have any thoughts/concerns?

Motivation is setting up a lot of Flows in advance (e.g., potentially to be run over several months via FireWorks), and wanting to ensure that the latest Structure object is always retrieved from MP at the time the Flow is run, instead of ahead of time and baking it in, such that Flows do not have to be re-generated when a new MP database is released. Database version and task_id are also stored for provenance. This is intended to help with production for downstream properties.

Additional dependencies introduced (if any)

  • Optional dependency mp-api added

TODO (if any)

  • Tests -- where's an appropriate place for tests for this? I saw symmetrize_structure is also not tested currently or I'd have put one alongside that.

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request
title.

Before a pull request can be merged, the following items must be checked:

  • Code is in the standard Python style.
    The easiest way to handle this is to run the following in the correct sequence on
    your local machine. Start with running black on your new code. This will
    automatically reformat your code to PEP8 conventions and removes most issues. Then run
    pycodestyle, followed by flake8.
  • Docstrings have been added in theNumpy docstring format.
    Run pydocstyle on your code.
  • Type annotations are highly encouraged. Run mypy to
    type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #176 (4e7e875) into main (0fe9542) will decrease coverage by 0.02%.
The diff coverage is 73.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
- Coverage   75.11%   75.08%   -0.03%     
==========================================
  Files          52       52              
  Lines        4685     4704      +19     
  Branches      688      693       +5     
==========================================
+ Hits         3519     3532      +13     
- Misses        981      985       +4     
- Partials      185      187       +2     
Impacted Files Coverage Δ
src/atomate2/settings.py 94.11% <ø> (ø)
src/atomate2/common/jobs.py 81.81% <73.91%> (-18.19%) ⬇️

@utf
Copy link
Member

utf commented Oct 5, 2022

This looks great. I have two quick comments:

  1. Can you add mp-api and a version number to the "strict" requirements section so that this will be installed with the tests.
  2. You can add the test to test/common/jobs.py. We can add a test for structure symmetrisation later.

pyproject.toml Outdated
@@ -61,6 +61,7 @@ strict = [
"phonopy==2.16.2",
"seekpath==2.0.1",
"numpy",
"mp-api>=0.27.5"
Copy link
Member

Choose a reason for hiding this comment

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

Can this one be pinned to a specific version (e.g., ==) as it makes the testing environment reproducible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing -- fair warning, this package is still being updated quite significantly until its "official release". For the functionality we're relying on here I expect it to be stable, but we'll probably want to keep this updated to the latest version fairly regularly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the heads up. Hopefully dependabot should keep this in check.

@mkhorton
Copy link
Member Author

mkhorton commented Oct 5, 2022

Ok, PR updated. Added tests for the other common.jobs since it felt rude not to!

For the API test, do you want this mocked? Without mocking, it may introduce some brittleness to the tests, but with mocking we might miss when/if this breaks, so maintainer's choice :)

@utf
Copy link
Member

utf commented Oct 5, 2022

Thanks Matt, really helpful! I'm happy with the live tests to be honest. Once tests pass I'll merge this.

@utf
Copy link
Member

utf commented Oct 5, 2022

Great, thank you!

@utf utf merged commit 590bb5f into main Oct 5, 2022
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.

2 participants