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

Linting fixes and CI config for multiple Python 3.9+ #34

Merged
merged 13 commits into from
Oct 11, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Oct 11, 2023

Previously we had fail-fast: true (the default) which means that the first failing Python version would cancel all other tests. This PR changes it so that we can at least see which versions are failing and why (c.f. #33).

Also:

  • Removed the template docs workflow, which has never been working.
  • Use Python 3.9 specifically in pre-commit CI
  • Fix any outstanding linting issues
  • Fix dummy test on version number
  • Tweak CI config to cancel jobs on pushes to the same PR
  • Added a few cases of from __future__ import annotations that were missing for py39 compat
  • Add a test that checks that the top-level of the package can be imported successfully

@ml-evs ml-evs force-pushed the ml-evs/python_versioning branch from b3e5380 to fa5a1b3 Compare October 11, 2023 12:33
@ml-evs ml-evs changed the title Fix CI config for multiple Python versions Linting fixes and CI config for multiple Python 3.9+ Oct 11, 2023
@ml-evs
Copy link
Member Author

ml-evs commented Oct 11, 2023

@gpetretto this PR addresses #33 and shows that we indeed currently fail on Python 3.8 (due to qtoolkit... so I've dropped it) and 3.9 (due to some weird typing issue with pydantic + future annotations messing up -- see the test I added). With the other changes bundled here (enforcing Python 3.9 pre-commit, etc.), hopefully this means the CI will be useful once again at detecting these issues, ready to add some proper tests.

I'll try now to fix the 3.9 issue with pydantic, but if you already have some idea how, feel free...

@ml-evs ml-evs linked an issue Oct 11, 2023 that may be closed by this pull request
@utf
Copy link
Collaborator

utf commented Oct 11, 2023

As a side note, I found that I couldn't include the future annotations in modules defining pydantic models. So in atomate2 we remove the future import in those modules.

@ml-evs
Copy link
Member Author

ml-evs commented Oct 11, 2023

As a side note, I found that I couldn't include the future annotations in modules defining pydantic models. So in atomate2 we remove the future import in those modules.

Indeed, the issue as I understand it is that from __future__ import annotations is effectively just turning all annotations into postponed versions (i.e., strings), but when pydantic actually tries to use them it runs into issues. I think to support 3.9 we will have to downgrade the annotations in the config (as evidenced by the last test run).

@ml-evs ml-evs force-pushed the ml-evs/python_versioning branch from c663e3d to 8853680 Compare October 11, 2023 13:28
@ml-evs ml-evs requested a review from gpetretto October 11, 2023 13:34
@ml-evs
Copy link
Member Author

ml-evs commented Oct 11, 2023

@gpetretto this is ready for you to look at I think, unfortunately in the process of fixing the CI there are a lot of whitespace/file ending fixes, so to quickly summarise the actual code changes:

  • use py39 compatible type hints for pydantic 8853680
  • add stacklevels to warnings (some flake8 plugin was complaining but it does make sense) 8d24905
  • add from __future__ import annotations in a few additional places -- if these still break then the next testing overhaul PR should catch them
  • add a test that checks that settings etc can be imported

Copy link
Contributor

@gpetretto gpetretto left a comment

Choose a reason for hiding this comment

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

looks good to me. Thanks!

@ml-evs ml-evs merged commit 3542a70 into develop Oct 11, 2023
4 checks passed
@ml-evs ml-evs deleted the ml-evs/python_versioning branch March 26, 2024 16:46
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.

Targeted Python versions
3 participants