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

Fix import error [ijson] #708

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Conversation

naik-aakash
Copy link
Contributor

Closes #702

Changes

  1. Enable MPVaspLobsterMaker only if lobsterpy and ijson are installed i.e atomate2 is installed using atomate2[lobster] dependencies
  2. Add warning message for the same incase MPVaspLobsterMaker is disabled.

Below is the snapshot of warning printed when MPVaspLobsterMaker is disabled.

image

@naik-aakash
Copy link
Contributor Author

Hi @jmmshn, I hope this addresses the problem now.

@JaGeo , just tagging you here incase you have any more suggestions or comments on this change.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (aaa08c0) 76.21% compared to head (62ae659) 76.19%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
- Coverage   76.21%   76.19%   -0.03%     
==========================================
  Files          87       87              
  Lines        7165     7178      +13     
  Branches     1057     1057              
==========================================
+ Hits         5461     5469       +8     
- Misses       1383     1388       +5     
  Partials      321      321              
Files Coverage Δ
src/atomate2/vasp/flows/mp.py 90.32% <100.00%> (+0.32%) ⬆️
src/atomate2/lobster/schemas.py 91.13% <66.66%> (-0.21%) ⬇️
src/atomate2/vasp/flows/lobster.py 83.92% <60.00%> (-5.21%) ⬇️

@utf
Copy link
Member

utf commented Feb 14, 2024

Hi @naik-aakash, thanks for this PR.

I think we could handle this differently. Ideally, the warning would be raised if you are actually trying to use the MPVaspLobsterMaker. Instead, this warning gets raised even if you just import the MP module.

The typical way of doing this is using the monty requires decorator. E.g., see here:

@classmethod
@requires(Analysis, "lobsterpy must be installed to create an CalcQualitySummary.")
def from_directory(

My suggestion would be:

  1. Replace the ijson import in with:
try:
    import ijson
except ImportError:
    ijson = None
  1. Add the requires decorator onto the function that needs ijson.

@naik-aakash
Copy link
Contributor Author

Thanks @utf, for the suggestion, I will make the required changes.

@utf utf mentioned this pull request Feb 15, 2024
13 tasks
@naik-aakash
Copy link
Contributor Author

Hi @utf , I have now made the changes and this PR could be merged if you think this is fine.

@utf
Copy link
Member

utf commented Feb 16, 2024

Looks great, thank you!

@utf utf merged commit 7f4d5a6 into materialsproject:main Feb 16, 2024
6 of 7 checks passed
@utf utf added the fix Bug fix PR label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ijson is an optional import
2 participants