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

Remove tests for Python 3.7 since it reached its end-of-life #851

Merged
merged 7 commits into from Sep 1, 2023
Merged

Remove tests for Python 3.7 since it reached its end-of-life #851

merged 7 commits into from Sep 1, 2023

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Aug 30, 2023

Python 3.7 reached its end-of-life on 06-27-2023. As such, I removed it from the test matrix and from the classifiers list in setup.py. I didn't change the python_requires minimum version in setup.py though to allow for some flexibility for those that are slow to upgrade.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Remove tests for Python 3.7 Remove tests for Python 3.7 since it reached its end-of-life Aug 30, 2023
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.09% ⚠️

Comparison is base (508c173) 88.14% compared to head (fb3c727) 88.06%.

❗ Current head fb3c727 differs from pull request most recent head 014b178. Consider uploading reports for the commit 014b178 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #851      +/-   ##
==========================================
- Coverage   88.14%   88.06%   -0.09%     
==========================================
  Files          44       44              
  Lines        3593     3593              
==========================================
- Hits         3167     3164       -3     
- Misses        426      429       +3     

see 1 file with indirect coverage changes

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

@rkingsbury
Copy link
Collaborator

Thanks @arosen93 ! This has been on my mind for a while but I wasn't sure when to pull the trigger. You have convinced me. Can you please also update the docs to "3.8+"? There is this line:

Maggma is written in Python and supports Python 3.7+.

I didn't change the python_requires minimum version in setup.py though to allow for some flexibility for those that are slow to upgrade.

Tell me more about this. I suppose this means that a user could install maggma in a Python 3.7 environment, whereas if we change it to 3.8, they would be prevented from doing so? I'm on the fence about this approach; I kinda feel like as long as it is possible to install with Py3.7, we ought to be testing against it. But it is becoming a pain to support so I'm in favor of dropping it.

@Andrew-S-Rosen
Copy link
Member Author

Happy to update the docs! Thanks for pointing that out.

And yes, you are interpreting the 'setup.py` details correctly. I'd personally vote to just update it to 3.8 but wasn't sure your opinion on it so took a more conservative approach. I suppose if a user really wants to use Maggma with Python 3.7, then they can just use a PyPI version that supports it.

@rkingsbury
Copy link
Collaborator

And yes, you are interpreting the 'setup.py` details correctly. I'd personally vote to just update it to 3.8 but wasn't sure your opinion on it so took a more conservative approach. I suppose if a user really wants to use Maggma with Python 3.7, then they can just use a PyPI version that supports it.

Thanks; yeah I think I'd prefer to keep everything consistent (e.g., we test until we truly drop support). @munrojm , any reservations about officially dropping Python 3.7 support?

@rkingsbury
Copy link
Collaborator

Side note, this question of how long to support 3.7 was a big part of what motivated my foundation PR on version support

emmet and atomate2 already set python_requires=3.8, and since those are probably the downstream packages that use maggma most, I think we're safe to drop 3.7

@Andrew-S-Rosen
Copy link
Member Author

@rkingsbury: I agree. I think consistency is smart and there shouldn't be any issues here. I have now updated the PR to reflect this discussion, including both the update to the docs and the setup.py.

@rkingsbury
Copy link
Collaborator

Given that pymatgen is now at 3.9+, I'm gonna pull the trigger on this.

See materialsproject/pymatgen#3283

@rkingsbury rkingsbury merged commit 7686860 into materialsproject:main Sep 1, 2023
3 of 6 checks passed
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

2 participants