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
[MAINT] Add python 3.12 to CI #4023
Conversation
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #4023 +/- ##
=======================================
Coverage 91.73% 91.73%
=======================================
Files 145 145
Lines 16230 16230
Branches 3367 3367
=======================================
Hits 14888 14888
Misses 797 797
Partials 545 545
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Python 3.12 was just released so shouldn't remove the allow-prereleases
parameter to make sure we're always testing on stable 3.12 releases?
.github/workflows/flake8.yml
Outdated
@@ -29,7 +29,8 @@ jobs: | |||
- name: Setup python | |||
uses: actions/setup-python@v4 | |||
with: | |||
python-version: '3.10' | |||
python-version: '3.11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked closely at the flake8 failures related to 3.12 but at some point we'll need to handle these. Perhaps put in a separate issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
they are not massive:
./examples/02_decoding/plot_haxby_grid_search.py:127:18: E226 missing whitespace around arithmetic operator
./nilearn/decoding/decoder.py:254:28: E201 whitespace after '{'
./nilearn/decoding/decoder.py:254:40: E231 missing whitespace after ':'
./nilearn/decoding/decoder.py:254:56: E202 whitespace before '}'
./nilearn/interfaces/fmriprep/tests/test_load_confounds.py:583:54: E226 missing whitespace around arithmetic operator
./nilearn/surface/surface.py:961:48: E201 whitespace after '{'
strangely though they are not "just" handled by black
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok maybe just manually handle here and update this workflow to 3.12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think this is related to the fact that f strings got upgraded in 3.12 and that you can have even more python code in them so code formatting rules will now start applying inside f strings too.
yeah why not |
@@ -43,7 +43,8 @@ jobs: | |||
- name: Setup python | |||
uses: actions/setup-python@v4 | |||
with: | |||
python-version: 3.11 | |||
python-version: '3.12' | |||
allow-prereleases: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set the pre-release to false, eventhough it is the default I feel it can be nice to keep it around for when we want to start checking new python pre-release
This looks good, then ? |
Yup. Merging |
Changes proposed in this pull request: