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

ci: style checks change python version from 3.6 to 3.8 #1080

Merged

Conversation

zekemorton
Copy link
Collaborator

@zekemorton zekemorton commented Sep 26, 2023

Problem: python 3.6 is end of life and is now cauing failures
for the ci pipeline linting and formatting stages.

Change python version to 3.8. Change black version to 22.3.0
to fix formatting error from new click release. This only
affects the ci linting and formatting checks.

@zekemorton zekemorton force-pushed the update-actions-python-version branch 3 times, most recently from 35db7d8 to 79714b3 Compare September 26, 2023 19:47
Problem: python 3.6 is end of life and is now cauing failures
for the ci pipeline linting and formatting stages.

Change python version to 3.8. Change black version to 22.3.0
to fix formatting error from new click release. This only
affects the ci linting and formatting checks.
@chu11
Copy link
Member

chu11 commented Sep 26, 2023

Perhaps this discussion needs to be brought up again, it's been almost 2 years

flux-framework/flux-core#3929

@zekemorton zekemorton force-pushed the update-actions-python-version branch from 79714b3 to 21f466d Compare September 26, 2023 20:04
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #1080 (ec9a6bc) into master (33ae91a) will decrease coverage by 0.1%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head ec9a6bc differs from pull request most recent head 21f466d. Consider uploading reports for the commit 21f466d to get more accurate results

@@           Coverage Diff            @@
##           master   #1080     +/-   ##
========================================
- Coverage    71.6%   71.6%   -0.1%     
========================================
  Files          89      89             
  Lines       11569   11569             
========================================
- Hits         8292    8289      -3     
- Misses       3277    3280      +3     

see 1 file with indirect coverage changes

@grondo
Copy link
Contributor

grondo commented Sep 26, 2023

Python 3.6 is still the /usr/bin/python3 in TOSS 4, so I don't think we can remove support for Python 3.6 any time soon since this is production software for those machines.

@grondo
Copy link
Contributor

grondo commented Sep 26, 2023

Eh, though if this is just for style checks maybe its fine? The only worry would be if the new style or lint checks caused a developer to write python that didn't work on 3.6.

@zekemorton
Copy link
Collaborator Author

That makes sense, unfortunately it looks like the change was made in the runner image, see here: actions/runner-images#8238 on September 18th. It looks like the other ubuntu image does not support python 3.6 either, so I am not sure if there is a better alternative?

@grondo
Copy link
Contributor

grondo commented Sep 26, 2023

Yes good point. I don't know of a better alternative at the moment. It does seem fairly low risk to update the runner python versions 🤷 I was just confused at first.

@zekemorton zekemorton changed the title ci: change python version from 3.6 to 3.8 ci: style checks change python version from 3.6 to 3.8 Sep 26, 2023
@milroy milroy mentioned this pull request Sep 27, 2023
1 task
@milroy
Copy link
Member

milroy commented Sep 28, 2023

The runner issue intimates that it may be possible to download and install a non-supported, archived version of Python on the runner: https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#python. I can't find any examples of how to do that, though. That approach might extend the action runtime considerably, too.

@vsoch
Copy link
Member

vsoch commented Sep 28, 2023

Python 3.6 is still the /usr/bin/python3 in TOSS 4, so I don't think we can remove support for Python 3.6 any time soon since this is production software for those machines.

You could make the same argument backwards - if we only use 3.6 here basically any system that isn't a crustacean could run into unforseen issues. A lot of new features have been added since then.

If it's just an issue of getting a working (old python from seven years ago lol then why not just install with conda (or miniconda is smaller)? They have versions that go back until the dawn of time. (cue sunrise with dinosaurs).

https://repo.anaconda.com/miniconda/ and then

@grondo
Copy link
Contributor

grondo commented Sep 28, 2023

You could make the same argument backwards - if we only use 3.6 here basically any system that isn't a crustacean could run into unforseen issues

We do test on python versions 3.6 - 3.10 in CI, so this is unlikely

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

This seems fine to me! Thanks!

@vsoch
Copy link
Member

vsoch commented Sep 28, 2023

yay! I love it when the answer to the problem is... what problem? :D

@vsoch
Copy link
Member

vsoch commented Sep 28, 2023

@grondo can we set merge-when-passing? I ask selfishly because I need this commit :)

@grondo
Copy link
Contributor

grondo commented Sep 28, 2023

Yes! This is approved so that should work

@vsoch vsoch added the merge-when-passing mergify.io - merge PR automatically once CI passes label Sep 28, 2023
@mergify mergify bot merged commit 4b16578 into flux-framework:master Sep 28, 2023
19 checks passed
@vsoch
Copy link
Member

vsoch commented Sep 28, 2023

boorah!! Thanks everyone!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing mergify.io - merge PR automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants