-
Notifications
You must be signed in to change notification settings - Fork 318
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 Python 3.8 from CI (#824) #826
base: main
Are you sure you want to change the base?
Conversation
@woodsp-ibm for future reference, where in the code would you recommend adding a deprecation message ahead of a Python version removal? For individual features in the code it's straightforward, but what about Python versions? |
Pull Request Test Coverage Report for Build 10487105761Details
💛 - Coveralls |
Qiskit does it in the main But.... as the apps are dependent on qiskit and since it deprecates support my recollection is that we simply removed support from the app release for whatever that version was in the release of the application pkg following on from when Qiskit did. The premise being the apps are dependent on qiskit, people install the latest of that, and having more than one deprecation notice may be annoying. E..g if you search for |
@@ -1,4 +1,4 @@ | |||
numpy>=1.20,<2.0 | |||
ipython<8.13;python_version<'3.9' | |||
ipython<8.13;python_version<'3.10' |
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.
Is this change due to bumping the version we use from 3.8 to 3.9 and we just did not have the constraint right before in that it really applied to more than just 3.8
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.
My understanding of <'3.9'
is that ipython
will pick 3.8
, which is the only allowed version strictly lower than 3.9
. So, to allow 3.9
I raised <'3.9'
to <'3.10'
. Is this correct?
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.
This is a constraint (limits) over what is installed when it's needed to be installed. So that basically says if ipython is installed it should be less than version < 8.13 but with the caveat that this only for when the python version its being installed into is less than that value. So before it would have installed some version of ipython < 8.13 when using 3.8 and was not constrained in other cases so it would have used whatever version it got resolved to, which is likely much newer. Now with bumping the Python version there it will be constraining it to < 8.13 in Python 3.9, which it was not before. But then I do not know which jobs exactly end up needing/using it - I imagine its for the notebook ones - which we would have run on 3.8 (and 3.12) before but not 3.9. You could try and see without that constraint.
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'll try out the tests without ipython<8.13;python_version<'3.10'
in the next commit.
On thing to note, that at the point when we want to merge this PR we should update the branch rules such that 3.9 jobs, that replaced the prior 3.8 ones, are Required to pass and those old 3.8 ones are not any more. While looking at the jobs, as we have had the new M1 jobs for some while, do we want to make them passing as Required for the merge too... |
Deprecation_Messages_and_Coverage: | ||
needs: [Checks, MachineLearning, Tutorials] | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: [3.8] | ||
python-version: [3.9] |
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.
On thing to note, that at the point when we want to merge this PR we should update the branch rules such that 3.9 jobs, that replaced the prior 3.8 ones, are Required to pass and those old 3.8 ones are not any more.
While looking at the jobs, as we have had the new M1 jobs for some while, do we want to make them passing as Required for the merge too...
I was expecting changing python version here and in the "needs" jobs would run them with 3.9 directly, but they appear to pick up 3.8 instead. What would you suggest in this case?
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.
How the (Branch Protection) rules work is you select from the jobs that are run which are required - its configured. The config still has the old jobs, but since they are no longer run they show up as required but of course they will never pass since they are not run. What needs to happen is that the new jobs that we require to pass are selected/configured into the rule and the old ones removed. You can see on PRs that do not change the jobs that things are still correct since the jobs configured in the rules are the ones that are run. As this changes things we will need to update the rules. Its something we end up doing from time to time is jobs are changed, such as dropping a Python version or adding a new one. Changing the rules just needs to be coordinated with this PR being merged as the rules will affect other PRs from the moment its changed since the rule is for the branch the PR targets.
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 understand, thanks @woodsp-ibm. Let's merge #827 first and then un-require the old jobs. In which file would you suggest changing the job configs?
Summary
This PR removed the CI test environment using Python 3.8, which is scheduled to reach EOL as in PEP 569.
Closes #824
Closes OkuyanBoga#44