Skip to content

Conversation

fcharras
Copy link
Contributor

@fcharras fcharras commented Jul 28, 2023

The package more_itertools, which is a dependency of pluggy/pytest, has lost compatibility with python 3.7 when releasing the version more_itertools>=10, and the conda distributions for more_itertools don't embed version specifiers for compatibility with python so the conda resolver installs version 10 anyway in the python 3.7 pipelines.

The pipelines with python3.7 can be fixed by requiring "more_itertools<10" in the pipeline setup script here and here

However python 3.7 also recently reached end of life so rather than working around this issue, this PR proposes to bump the python3.7 pipelines to python3.8 (and also bump numpy==1.15 to numpy==1.16 which is the smallest version that supports python 3.8, much like 1.15 was the smallest version that supported python 3.7)

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6a5f1b9) 95.08% compared to head (e36f0bf) 95.09%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
+ Coverage   95.08%   95.09%   +0.01%     
==========================================
  Files          44       44              
  Lines        7564     7564              
==========================================
+ Hits         7192     7193       +1     
+ Misses        372      371       -1     

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

@fcharras fcharras changed the title wip: fix ci pipeline setup FIX fix python 3.7 ci pipelines Jul 28, 2023
@fcharras fcharras self-assigned this Jul 28, 2023
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@jeremiedbb
Copy link
Contributor

So this is not just fixing CI, it completely drops support for python 3.7. I'm not sure we want to do that for a bug fix release. To me if we wanted to drop support for 3.7 we should have done that for 1.3.0. Now we're kind of stuck with it until 1.4.0, don't you think ?

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Jul 28, 2023 via email

@fcharras
Copy link
Contributor Author

fcharras commented Jul 28, 2023

The first commit in this branch contains the bugfix: d75cd9e I can open a separate PR with this commit only if it is preferable.

I'm not sure we want to do that for a bug fix release

If bug fix releases are needed it can start from a branch starting from the latest commit release, that keeps the 3.7 pipelines + apply the workaround to the more_itertools problem to go on. Depends how you want to manage releases. Applying the workaround on master means one has to remember reverting it at some point.

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

If I understand correctly, the released version is compatible with python 3.7 and the bug fix release is too. But pytest is not so this breaks our pipelines.

I would say, we fix the pipelines with this PR as a maintenance commit by bumping up the version we test on. Then we do a bug fix release and drop python 3.7 afterward?

To me, the only changes relative to dropping python 3.7 are the changes in the setup. I would not overcomplicate things by making a temporary pin for pytest version as the current code has already been tested on python3.7 extensively but if you feel this is mandatory, we can do it.

but let's do the bf release soon!

- Add more pypy pipelines to test matrix
    + oldest supported python (3.8) + oldest compatible pypy version (7.3.6)
    + oldest supported python (3.8) + most recent compatible pypy version (7.3.11)
    + most recent pypy version (7.3.12) + most recent supported python version (3.10)

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>
@tomMoral tomMoral changed the title FIX fix python 3.7 ci pipelines MTN update ci pipelines to use python3.8 + test more pypi versions Aug 7, 2023
@ogrisel ogrisel changed the title MTN update ci pipelines to use python3.8 + test more pypi versions MTN update ci pipelines to use python3.8 + test more pypy versions Aug 29, 2023
@ogrisel
Copy link
Contributor

ogrisel commented Aug 29, 2023

Do we really want to support several PyPy versions? I would personally be find just testing on the most recent stable release of PyPy.

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Seems like we forgot that we already had a PR to drop 3.7 and did the change in #1515...
Sorry about that.

This PR is still worth merging as it improves the support for pypy.

@tomMoral
Copy link
Contributor

tomMoral commented Oct 20, 2023

Do we really want to support several PyPy versions? I would personally be find just testing on the most recent stable release of PyPy.

Just saw this comment, I have -0 opinion on this. As the pipelines are already quite long, there is maybe a fair point in only trying the latest version.

So this would mean closing this PR as this is the main change in the end.

@fcharras fcharras removed their assignment Nov 13, 2023
@fcharras
Copy link
Contributor Author

fcharras commented Nov 16, 2023

I thought this PR was closing so I un-assigned. Should we still push it forward ? The last pipeline fails with a deadlock with the multiprocessing backend, I've never seen it before, probably rarely occuring bug. Shouldn't we also bump this pipeline to python 3.11 and (pypy doesnt work with 3.11) pypy 7.3.13 if it moves forward ?

@fcharras
Copy link
Contributor Author

The readthedocs pipeline failure comes from expired certificate on docs.scipy.org

Relevant issue scipy/docs.scipy.org#80

I added configuration to bypass the certificate in the readthedoc pipeline. It's green again. But it sounds better to just wait for scipy/docs.scipy.org#80 to be solved before continuing forward, all the more so since sphinx doesn't support setting tls_verify per domain and only can register a global configuration.

@ogrisel
Copy link
Contributor

ogrisel commented Nov 16, 2023

Yes, let's revert the tls_verify change and merge this PR.

@fcharras fcharras merged commit b8e3f7a into joblib:master Nov 16, 2023
@fcharras fcharras deleted the fix/fix_ci branch November 16, 2023 11:29
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.

5 participants