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 support for Python 3.8 #1253

Merged
merged 4 commits into from
Dec 14, 2024
Merged

Conversation

ksbeattie
Copy link
Member

@ksbeattie ksbeattie commented Dec 11, 2024

Fixes/Addresses:

PyQt5-sip was just updated to 12.16.0 which removed support for Python 3.8. This is now breaking the CI for PR #1252 in an entirely unrelated way.

Summary/Motivation:

Since Python 3.8 is now EOL, we should remove support for it as well.

Changes proposed in this PR:

  • Remove python 3.8 from our CI and documentation

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the copyright and license terms described in the LICENSE.md file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@ksbeattie ksbeattie added the Priority:High High Priority Issue or PR label Dec 11, 2024
@ksbeattie ksbeattie self-assigned this Dec 11, 2024
@ksbeattie
Copy link
Member Author

ksbeattie commented Dec 11, 2024

Things I noticed while doing this:

  • We need to update our docs on the use of Miniforge for conda
  • We should look into updating the versions of the following that we use (in requirements-dev.txt):
    • black: from 24.3.0 to 24.10.0
    • pylint: from 3.1.0 to 3.3.2
    • astroid: from 3.1.0 to 3.3.6
    • pytest: not <8.1
  • Look at thhe versions of Linux, Windows and GH actions used in our CI (.github/workflows/checks.yml)
  • The Matlab docs show support for now very old versions of Python

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.47%. Comparing base (62534b8) to head (c70ee09).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1253   +/-   ##
=======================================
  Coverage   38.47%   38.47%           
=======================================
  Files         164      164           
  Lines       37020    37020           
  Branches     5728     5728           
=======================================
  Hits        14243    14243           
  Misses      21650    21650           
  Partials     1127     1127           

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

@ksbeattie
Copy link
Member Author

After e11d490 which just fixed the version of PyQt5-sip to 12.15.0 for python 3.8, all tests passed.

@ksbeattie ksbeattie marked this pull request as ready for review December 11, 2024 23:53
- '3.9'
- '3.10'
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to set these to 3.12, so that the oldest and newest Python version are both tested

Comment on lines 9 to 10
pylint==3.1.0;python_version>"3.8"
astroid==3.1.0;python_version>"3.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to remove the python_version>3.8, since that's now always going to be true.

Suggested change
pylint==3.1.0;python_version>"3.8"
astroid==3.1.0;python_version>"3.8"
pylint==3.1.0
astroid==3.1.0

Copy link
Contributor

@lbianchi-lbl lbianchi-lbl left a comment

Choose a reason for hiding this comment

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

From a quick look (i.e. only looking at the changes in this PR, and not through the entire codebase), things look fine apart from a couple minor suggestions.

@ksbeattie
Copy link
Member Author

@boverhof & @bpaul4 can I get a quick review of this? I can then get #1252 working again.

user_plugins
turbine_aws.cfg
user_ml_ai_models/__init__.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this added here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I accidentally added them in fa2d5b0 ! I figured I (or others) might make the same mistake again so I added them to be ignored.

Copy link
Contributor

@bpaul4 bpaul4 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!

@ksbeattie ksbeattie merged commit 8fb9a8f into CCSI-Toolset:master Dec 14, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants