MNT Support and test Python 3.9 #18761

terencehonles · 2020-11-05T06:23:19Z

What does this implement/fix? Explain your changes.

Updates Python 3.8 to Python 3.9 in latest tests, and updates trove classifiers to mention Python 3.9 support. I'm not sure what else is needed to build the Python 3.9 wheels since I didn't see anything obvious in this repo that should be changed.

gsheni · 2020-11-05T16:40:19Z

build_tools/azure/install.sh

+    # NOTE(honles): scikit-image does not include pyproject.toml in its
+    #               current release and these are what it needs to build.
+    #               This can be removed when
+    #               https://github.com/scikit-image/scikit-image/pull/5016


That MR has been merged: scikit-image/scikit-image#5016

Yes, but it has not been released yet. I will create a separate PR which can be merged after that is released to pypi/conda.

I'm also trying to get Python 3.9 wheels built too, so hopefully that will all come together in the next release

See: scikit-image/scikit-image#5052

I would rather not build scikit-image from source for now. Maybe we can only install scikit-image for the builds that are for Python PYTHON_VERSION < "3.9" for now instead.

It's an optional dependency of scikit-learn, so it should still work.

So would you prefer I change this to: python -m pip install --only-binary :all: sckit-image || true? So it only installs binary packages and only if they exists? I'd have to see if anything else has to be altered, but I'd prefer not to add config that has to be updated manually and just install it automatically if it can.

I agree with @ogrisel.

ogrisel

Thanks for the PR but we need to pin the version of numpy in the build dependency:

ogrisel · 2020-11-06T14:47:48Z

build_tools/azure/install.sh

+    # NOTE(honles): scikit-image does not include pyproject.toml in its
+    #               current release and these are what it needs to build.
+    #               This can be removed when
+    #               https://github.com/scikit-image/scikit-image/pull/5016


I would rather not build scikit-image from source for now. Maybe we can only install scikit-image for the builds that are for Python PYTHON_VERSION < "3.9" for now instead.

It's an optional dependency of scikit-learn, so it should still work.

ogrisel · 2020-11-06T14:49:08Z

pyproject.toml

+    "numpy==1.16.5; python_version=='3.7'",
+    "numpy==1.17.3; python_version=='3.8'",
+    # do not pin numpy on future versions of python to avoid incompatible numpy and python versions
+    "numpy; python_version>='3.9'",


We need to pin the oldest version of numpy that supports python_version == '3.9' because it will define the ABI compatibility expectations for the scikit-learn wheel.

I'm not sure I follow this comment. This is the same as scipy, and I figured that would be acceptable. I'm not sure if you can explain your thinking a little more.

I can change it to 1.19.3 if needed, since that is the first version that has released wheels for Python 3.9. I just figured it might cut down on the maintenance burden and I assumed that was why scipy set that as their default.

We use the version of numpy pyproject.toml to build the wheels in a github actions workflow. scikit-learn has Cython extension that need numpy at build time and the generated wheel need to be built with the oldest supported version of numpy to make sure that our build do not depend on recently changes of the numpy ABI.

For instance the future scikit-learn 0.24 wheel for python 3.8 needs to be build with numpy 1.17.3 to make sure that users who have such old versions of numpy can install a binary package of scikit-learn that is binary compatible.

The wheel building workflow is here:

https://github.com/scikit-learn/scikit-learn/blob/master/.github/workflows/wheels.yml

@ogrisel yeah, I gathered as much as you explained. Not sure why scipy chose to not pin numpy the same way, but I'm fine with setting this to 1.19.3, and thanks for the pointer to the new workflow. cibuildwheels looks nice 🙂

@alfaro96 yeah I got that 😉 , but thanks for making it explicit

ogrisel · 2020-11-06T14:52:17Z

/cc @alfaro96 since it's related to the new wheel builder infrastructure developed in #17921.

alfaro96

Hey @terencehonles,

Thank you for your PR!

I have a few of requested changes:

I think that we may update the Python version used in circleci:

grep -rnw ".circleci" -e "python"
.circleci/config.yml:6:      - image: circleci/python:3.7.3-stretch
.circleci/config.yml:47:      - image: circleci/python:3.7.3-stretch
.circleci/config.yml:93:      - image: circleci/python:3.6
.circleci/config.yml:122:      - image: circleci/python:3.6

If we decide to update the NumPy version for Python 3.6 to 1.16.5, we should update the NUMPY_MIN_VERSION global variable in the sklearn/_min_dependencies.py file.

alfaro96 · 2020-11-06T17:02:02Z

.github/workflows/check-manifest.yml

@@ -11,7 +11,7 @@ jobs:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
        with:
-          python-version: '3.8'
+          python-version: '3.9'


By default, Python 3.9 is used in GitHub Actions.

Are you suggesting I remove this? I believe it might make sense to keep this explicit since this is a C extension that may need to be pinned to a different version than what is the default. If you'd like me to remove it I can.

Let us keep then.

alfaro96 · 2020-11-06T17:03:19Z

azure-pipelines.yml

@@ -15,7 +15,7 @@ jobs:
  steps:
    - task: UsePythonVersion@0
      inputs:
-        versionSpec: '3.8'
+        versionSpec: '3.9'


I think that the same applies for Azure Pipelines.

Same comment here, though I believe Azure Pipelines may not have switched the default (I know this was discussed earlier on). I also remember coming across microsoft/azure-pipelines-tasks#13708 but it looks like there has been no response there.

Okay, same here.

alfaro96 · 2020-11-06T17:07:41Z

build_tools/azure/install.sh

+    # NOTE(honles): scikit-image does not include pyproject.toml in its
+    #               current release and these are what it needs to build.
+    #               This can be removed when
+    #               https://github.com/scikit-image/scikit-image/pull/5016


I agree with @ogrisel.

pyproject.toml

terencehonles · 2020-11-07T02:01:04Z

I think that we may update the Python version used in circleci:

I'm not sure if we can update doc-min-dependencies I bumped it to use the Python 3.9 circle CI image, and I specified Python 3.9 as the Python version. It fails during conda install, but if I'm not mistaken that job looks like it's computing the min requirements and it makes sense to be running it with the lowest Python version. Does that seem correct?

ogrisel · 2020-11-09T15:09:46Z

It fails during conda install, but if I'm not mistaken that job looks like it's computing the min requirements and it makes sense to be running it with the lowest Python version. Does that seem correct?

Yes Python is considered a dependency of scikit-learn as numpy, scipy, etc. So the min_dependency build of circle CI should use the oldest supported version of Python which is Python 3.6 at the moment.

ogrisel · 2020-11-09T15:22:38Z

Not sure why scipy chose to not pin numpy the same way, but I'm fine with setting this to 1.19.3.

If scipy pins to an older version of numpy for Python 3.9, we can do so, but we need to wait for this Python 3.9 wheel for numpy to be uploaded to that version on PyPI, otherwise we won't be able to build our own wheels.

ogrisel · 2020-11-09T16:29:07Z

@terencehonles To keep this PR focused and merge it quickly to prepare the scikit-learn 0.24 release, could you please revert the changes made on the circle ci configuration? They are not required to support and test Python 3.9.

We can always bump up the python versions used in circle ci in another PR but this requires more in dept testing and now is not the time to risk making the doc building infrastructure unstable ;)

Same for PyPy which is hard to test at the moment because of #18624.

alfaro96 · 2020-11-09T16:44:20Z

@terencehonles Do you mind updating the pyproject.toml file according to #18782 (comment)?

terencehonles · 2020-11-09T17:25:23Z

Not sure why scipy chose to not pin numpy the same way, but I'm fine with setting this to 1.19.3.

If scipy pins to an older version of numpy for Python 3.9, we can do so, but we need to wait for this Python 3.9 wheel for numpy to be uploaded to that version on PyPI, otherwise we won't be able to build our own wheels.

@ogrisel I'm aware of that, I mean that they don't constrain future releases to the min version needed to build. It's not really important to discuss since it seems like there are more specific requirements for this package.

@terencehonles To keep this PR focused and merge it quickly to prepare the scikit-learn 0.24 release, could you please revert the changes made on the circle ci configuration? They are not required to support and test Python 3.9.

We can always bump up the python versions used in circle ci in another PR but this requires more in dept testing and now is not the time to risk making the doc building infrastructure unstable ;)

Same for PyPy which is hard to test at the moment because of #18624.

@ogrisel this PR really isn't that big and now two of you are asking for conflicting things. I changed it because @alfaro96 asked, but also because the change does need to be tested. The CI changes are not failing so I'm not sure what's your reason to not make the change now.

@terencehonles Do you mind updating the pyproject.toml file according to #18782 (comment)?

@alfaro96 done, I added some comments to explain when the constraints can be removed as Python versions are expired.

ogrisel · 2020-11-09T17:45:13Z

The CI changes are not failing so I'm not sure what's your reason to not make the change now.

The full doc build is very expensive and not triggered by your PR. The PyPy build is only triggered by a scheduled task it does not run on PRs.

See: https://scikit-learn.org/dev/developers/contributing.html#continuous-integration-ci

The following were used as references when updating pyproject.toml: - scipy/scipy#12940 - scikit-image/scikit-image#4920 - scikit-image/scikit-image#4920 (comment)

terencehonles · 2020-11-10T01:03:33Z

The CI changes are not failing so I'm not sure what's your reason to not make the change now.

The full doc build is very expensive and not triggered by your PR. The PyPy build is only triggered by a scheduled task it does not run on PRs.

See: https://scikit-learn.org/dev/developers/contributing.html#continuous-integration-ci

It really looks like that only applies to pypy which I had thought I hadn't included, but it looks like I had left it in. Since I was going to be pushing I reverted any changes to Circle CI at your request.

ogrisel

LGTM!

ogrisel · 2020-11-10T10:34:04Z

I just pushed a commit to check that the new pyproject.toml does not break the wheel builder workflow.

alfaro96

Thank you @terencehonles for your PR.

I would like to know if it would be possible to use oldest-supported-numpy to simplify the pyproject.toml file.

alfaro96 · 2020-11-10T14:52:41Z

.github/workflows/check-manifest.yml

@@ -11,7 +11,7 @@ jobs:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
        with:
-          python-version: '3.8'
+          python-version: '3.9'


Let us keep then.

alfaro96 · 2020-11-10T14:52:52Z

azure-pipelines.yml

@@ -15,7 +15,7 @@ jobs:
  steps:
    - task: UsePythonVersion@0
      inputs:
-        versionSpec: '3.8'
+        versionSpec: '3.9'


Okay, same here.

build_tools/azure/install.sh

alfaro96 · 2020-11-10T15:04:31Z

pyproject.toml

+
+    # PyPy needs numpy >= 1.14.0
+    # platform_python_implementation!='CPython' not needed >= Python 3.7 which is numpy 1.14.5
+    "numpy==1.13.3; python_version=='3.6' and platform_machine!='aarch64' and platform_system!='AIX' and platform_python_implementation=='CPython'",
+    "numpy==1.14.0; python_version=='3.6' and platform_machine!='aarch64' and platform_system!='AIX' and platform_python_implementation!='CPython'",
+
+    # AIX needs numpy >= 1.16.0
+    # platform_system!='AIX' not needed >= Python 3.8 which is numpy 1.17.3
+    "numpy==1.16.0; python_version=='3.6' and platform_machine!='aarch64' and platform_system=='AIX'",
+    "numpy==1.16.0; python_version=='3.7' and platform_machine!='aarch64' and platform_system=='AIX'",
+
+    # ARM needs numpy >= 1.19.0
+    # platform_machine!='aarch64' not needed >= Python 3.9 which is numpy 1.19.3
+    "numpy==1.19.0; python_version=='3.6' and platform_machine=='aarch64'",
+    "numpy==1.19.0; python_version=='3.7' and platform_machine=='aarch64'",
+    "numpy==1.19.0; python_version=='3.8' and platform_machine=='aarch64'",
+
+    # default numpy requirements
+    "numpy==1.14.5; python_version=='3.7' and platform_machine!='aarch64' and platform_system!='AIX'",
+    "numpy==1.17.3; python_version=='3.8' and platform_machine!='aarch64'",
+    "numpy==1.19.3; python_version=='3.9'",
+


What about using oldest-supported-numpy?

It would simplify the pyproject.toml file quite a lot.

WDYT @thomasjpfan and @ogrisel?

That seems like a pretty interesting idea to me, but we'd need to upstream all the changes that were made here (existing pypy constraints and added ARM constraints)

Let us accept this solution and keep us track of oldest-supported-numpy to see if we could use it in the future.

oldest-supported-numpy (implicitly by numpy developers) is not necessarily the same as oldest-supported-numpy by scikit-learn developers.

I think we are rather conservative, so the two might match, but we would have to discuss that with a dedicated issue.

Let's not block this PR further but feel free to open an issue on this.

Do you know where the pypy version constraints come from? Looking at PyPI I can see the ARM wheels so I can create a PR for that, but I don't know if I should upstream the PyPy requirement of 1.14.0

Not sure either.

See #11010 (comment). We had 7 failures with numpy==1.13.3 and none with numpy==1.14.0 for PyPy.

I do not think that we could upstream such dependencies since as @ogrisel pointed out: "oldest-supported-numpy is not necessarily the same as oldest-supported-numpy by scikit-learn developers."

Sorry for the noise!

Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>

alfaro96

LGTM.

alfaro96 · 2020-11-10T16:00:30Z

pyproject.toml

+
+    # PyPy needs numpy >= 1.14.0
+    # platform_python_implementation!='CPython' not needed >= Python 3.7 which is numpy 1.14.5
+    "numpy==1.13.3; python_version=='3.6' and platform_machine!='aarch64' and platform_system!='AIX' and platform_python_implementation=='CPython'",
+    "numpy==1.14.0; python_version=='3.6' and platform_machine!='aarch64' and platform_system!='AIX' and platform_python_implementation!='CPython'",
+
+    # AIX needs numpy >= 1.16.0
+    # platform_system!='AIX' not needed >= Python 3.8 which is numpy 1.17.3
+    "numpy==1.16.0; python_version=='3.6' and platform_machine!='aarch64' and platform_system=='AIX'",
+    "numpy==1.16.0; python_version=='3.7' and platform_machine!='aarch64' and platform_system=='AIX'",
+
+    # ARM needs numpy >= 1.19.0
+    # platform_machine!='aarch64' not needed >= Python 3.9 which is numpy 1.19.3
+    "numpy==1.19.0; python_version=='3.6' and platform_machine=='aarch64'",
+    "numpy==1.19.0; python_version=='3.7' and platform_machine=='aarch64'",
+    "numpy==1.19.0; python_version=='3.8' and platform_machine=='aarch64'",
+
+    # default numpy requirements
+    "numpy==1.14.5; python_version=='3.7' and platform_machine!='aarch64' and platform_system!='AIX'",
+    "numpy==1.17.3; python_version=='3.8' and platform_machine!='aarch64'",
+    "numpy==1.19.3; python_version=='3.9'",
+


Let us accept this solution and keep us track of oldest-supported-numpy to see if we could use it in the future.

Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>

adrinjalali

LGTM, and pretty small at this point.

terencehonles · 2020-11-10T16:34:21Z

Let us accept this solution and keep us track of oldest-supported-numpy to see if we could use it in the future.

I've opened a PR to upstream the aarch constraints scipy/oldest-supported-numpy#13, but if the PyPy change doesn't get upstreamed you can probably use the oldest-supported-numpy package when dropping Python 3.6

ogrisel · 2020-11-10T16:49:45Z

Thanks all!

terencehonles force-pushed the support-python3.9 branch 2 times, most recently from 6dc223a to 5449fd5 Compare November 5, 2020 06:54

gsheni reviewed Nov 5, 2020

View reviewed changes

ogrisel added this to the 0.24 milestone Nov 6, 2020

ogrisel requested changes Nov 6, 2020

View reviewed changes

ogrisel added the Blocker label Nov 6, 2020

ogrisel mentioned this pull request Nov 6, 2020

Updade the maintainers' guide to use the new Wheel Builder github actions tool #18776

Closed

alfaro96 suggested changes Nov 6, 2020

View reviewed changes

terencehonles force-pushed the support-python3.9 branch 4 times, most recently from 35e7cde to 0afb549 Compare November 7, 2020 01:58

ogrisel mentioned this pull request Nov 9, 2020

Use pyproject.toml for build dependencies MacPython/scikit-learn-wheels#57

Closed

terencehonles force-pushed the support-python3.9 branch from 0afb549 to 249ebc3 Compare November 9, 2020 17:22

MNT Support and test Python 3.9

eafd63a

The following were used as references when updating pyproject.toml: - scipy/scipy#12940 - scikit-image/scikit-image#4920 - scikit-image/scikit-image#4920 (comment)

terencehonles force-pushed the support-python3.9 branch from 249ebc3 to eafd63a Compare November 10, 2020 01:02

ogrisel approved these changes Nov 10, 2020

View reviewed changes

Trigger wheel builder [cd build]

e6c07ad

ogrisel requested review from alfaro96 and thomasjpfan November 10, 2020 11:30

ogrisel added Waiting for Reviewer Build / CI labels Nov 10, 2020

alfaro96 reviewed Nov 10, 2020

View reviewed changes

alfaro96 mentioned this pull request Nov 10, 2020

CI Build wheels for the ARM64 architecture #18782

Merged

Fix typo when optionally installing scikit-image

9e8314c

Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>

alfaro96 approved these changes Nov 10, 2020

View reviewed changes

Update build_tools/azure/install.sh [ci skip]

eda15d4

Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>

adrinjalali approved these changes Nov 10, 2020

View reviewed changes

adrinjalali merged commit 8af5f61 into scikit-learn:master Nov 10, 2020

terencehonles mentioned this pull request Nov 10, 2020

Add constraints for aarch64: numpy==1.19.0 for Python<=3.8 scipy/oldest-supported-numpy#13

Merged

terencehonles deleted the support-python3.9 branch November 10, 2020 16:34

terencehonles mentioned this pull request Nov 23, 2020

MNT simplify pyproject.toml by using oldest-supported-numpy #18900

Merged

lesteve mentioned this pull request Feb 21, 2022

CI: remove special case for scikit-image install #22564

Closed

MNT Support and test Python 3.9 #18761

MNT Support and test Python 3.9 #18761

Conversation

terencehonles commented Nov 5, 2020

What does this implement/fix? Explain your changes.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel commented Nov 6, 2020

alfaro96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles commented Nov 7, 2020

ogrisel commented Nov 9, 2020

ogrisel commented Nov 9, 2020

ogrisel commented Nov 9, 2020

alfaro96 commented Nov 9, 2020 • edited Loading

terencehonles commented Nov 9, 2020

ogrisel commented Nov 9, 2020

terencehonles commented Nov 10, 2020

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel commented Nov 10, 2020

alfaro96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfaro96 Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfaro96 left a comment

Choose a reason for hiding this comment

alfaro96 Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

terencehonles commented Nov 10, 2020

ogrisel commented Nov 10, 2020

alfaro96 commented Nov 9, 2020 •

edited

Loading

alfaro96 Nov 10, 2020 •

edited

Loading

alfaro96 Nov 10, 2020 •

edited

Loading