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

Enable caching for get_ion_reference_data #429

Merged
merged 8 commits into from Nov 30, 2021

Conversation

rkingsbury
Copy link
Contributor

Cleanups for #428 to enable caching

@rkingsbury
Copy link
Contributor Author

I'm having trouble understanding why @lru_cache doesn't play well with mypy here. Adding to my confusion, the code here passes mypy linting on my machine but not in the CI.

All unit tests also pass on my local machine, but in the CI there are several of these errors:

___________________ ERROR collecting tests/test_mprester.py ____________________
tests/test_mprester.py:9: in <module>
    from mp_api.matproj import MPRester
src/mp_api/__init__.py:6: in <module>
    from mp_api.client import MPRester
src/mp_api/client.py:39: in <module>
    class MPRester:
src/mp_api/client.py:582: in MPRester
    def _get_ion_reference_data(self, chemsys: tuple):
/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/functools.py:490: in lru_cache
    raise TypeError('Expected maxsize to be an integer or None')
E   TypeError: Expected maxsize to be an integer or None

So basically I'm confused :) Can anyone advise?

@munrojm
Copy link
Member

munrojm commented Nov 15, 2021

This appears to be an issue with Python 3.7x and not worth spending time fixing. For now, I would add # type: ignore to the end of lines 575 and 577.

Also, for consistency I would vote to change your type hint references to the ones from the typing module. Namely, dict to Dict and tuple to Tuple.

@rkingsbury
Copy link
Contributor Author

Thanks @munrojm . Linters pass now, and I updated the type hint refs per your suggestion.

@mkhorton mkhorton merged commit f31191a into materialsproject:main Nov 30, 2021
@mkhorton
Copy link
Member

Merging now, intend to make some further changes later.

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.

None yet

3 participants