Skip to content
This repository has been archived by the owner on Feb 15, 2023. It is now read-only.

MAINT: mac and win py3.10 wheels #132

Merged
merged 7 commits into from
Oct 23, 2021

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Oct 3, 2021

Wheels for macOS and Windows for Python 3.10.

Wait until NumPy's wheel are in:

Following what NumPy did MacPython/numpy-wheels#135 (comment):

  • Microsoft will be dropping 32 bit Python 3.10 and it is not available on azure-pipelines
  • The Mac version is only available as a universal2 wheel. That is due to multibuild requiring osx 11+ for the build.

@tupui
Copy link
Contributor Author

tupui commented Oct 18, 2021

Looks like we have some timeout, but the wheels are built for linux and macOS. For windows, there is this

pypocketfft.cxx
C:\Python310-x64\Include\pybind11/numpy.h(36): error C2065: 'ssize_t': undeclared identifier
C:\Python310-x64\Include\pybind11/numpy.h(36): error C2338: ssize_t != Py_intptr_t

@charris would you know? Is this linked to numpy/numpy#18890?

@mattip
Copy link
Contributor

mattip commented Oct 18, 2021

That was a cleanup of external headers done for 3.10. It is described in numpy/numpy#18888 and was fixed for numpy in numpy/numpy#18890 by ssize_t -> Py_ssize_t

@tupui
Copy link
Contributor Author

tupui commented Oct 18, 2021

That was a cleanup of external headers done for 3.10. It is described in numpy/numpy#18888 and was fixed for numpy in numpy/numpy#18890 by ssize_t -> Py_ssize_t

Thanks @mattip. But then does it mean https://github.com/pybind/pybind11/blob/master/include/pybind11/numpy.h should be adapted??

@mattip
Copy link
Contributor

mattip commented Oct 18, 2021

Are you using that version? I think you are using an older pybind11 somehow that has the previous

static_assert(sizeof(ssize_t) == sizeof(Py_intptr_t), "ssize_t != Py_intptr_t");

rather than the current HEAD's

static_assert(sizeof(::pybind11::ssize_t) == sizeof(Py_intptr_t), "ssize_t != Py_intptr_t");

@tupui
Copy link
Contributor Author

tupui commented Oct 18, 2021

Are you using that version? I think you are using an older pybind11 somehow that has the previous

Oh thanks, let's try that! Sorry I don't know c++

Seems like this change was introduced in pybind11 2.6.

@@ -11,7 +11,7 @@ env:
- PLAT=x86_64
- CYTHON_BUILD_DEP="Cython==0.29.24"
- PYTHRAN_BUILD_DEP="pythran"
- PYBIND11_BUILD_DEP="pybind11==2.4.3"
- PYBIND11_BUILD_DEP="pybind11>=2.4.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be determined by the build process itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean we should not pin anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not here, the pinning and pip installing should all be from the repo's files. Maybe that is material for a future PR.

@tupui
Copy link
Contributor Author

tupui commented Oct 18, 2021

Failures from travis got introduced with scipy/scipy#13096. I asked if we can simply bump the tolerances in this case.

@tupui
Copy link
Contributor Author

tupui commented Oct 18, 2021

Not sure if we can do anything with the timeouts right now as it takes almost an hour to build the wheels. Either we don't test, we mark some tests as slow or we increase the timeoutInMinutes limit to 90 minutes for instance.

@tupui
Copy link
Contributor Author

tupui commented Oct 18, 2021

Now we have some failures with cdist on windows with py 3.10.

self.Y = spatial.distance.cdist(x_min, X_min, 'euclidean')
        X_min      = array([[0.5    ],
       [0.65625]])
        self       = <scipy.optimize._shgo.SHGO object at 0x000001D2EF63CD60>
        x_min      = array([[0.30817338]])
C:\Python310-x64\lib\site-packages\scipy\spatial\distance.py:3043: in cdist
    return cdist_fn(XA, XB, out=out, **kwargs)
E   RuntimeError: Arrays must be aligned
        XA         = array([[0.30817338]])
        XB         = array([[0.5    ],
       [0.65625]])
        cdist_fn   = <built-in method cdist_euclidean of PyCapsule object at 0x000001D2DCC84B40>
        kwargs     = {}

This is triggered from here https://github.com/scipy/scipy/blob/ca5e4f012f2ff847ff64310d81a29d2d06e353f3/scipy/spatial/src/distance_pybind.cpp#L228-L230.
Would you know @peterbell10 @perimosocordiae? An ordering issue?

@tupui
Copy link
Contributor Author

tupui commented Oct 19, 2021

Failures from travis got introduced with scipy/scipy#13096. I asked if we can simply bump the tolerances in this case.

Fixed in scipy/scipy#14878

There are 2 remaining issue. Timeouts and cdist arrays' alignment on windows.

@peterbell10
Copy link

@tupui I don't see the cdist error in any of the build logs. Can you link it for me?

@tupui
Copy link
Contributor Author

tupui commented Oct 19, 2021

@tupui I don't see the cdist error in any of the build logs. Can you link it for me?

Sure it's there at the end of the log https://ci.appveyor.com/project/scipy/scipy-wheels/builds/41187597/job/yvt3dq1kgbxcb3pe

@tupui tupui force-pushed the python_3_10_mac_win branch from 700fd5d to 578f5b7 Compare October 19, 2021 18:42
@tupui
Copy link
Contributor Author

tupui commented Oct 19, 2021

@peterbell10 Looks like now the issue reads RuntimeError: Arrays must be aligned to element size, but found stride of 9223372036854775807 bytes for elements of size 8. Hope this helps 😅

@peterbell10
Copy link

Can you try pinning pybind11 to 2.7 and see if there is any difference?

@tupui
Copy link
Contributor Author

tupui commented Oct 20, 2021

@peterbell10 the pinning did not resolve the issue. But maybe it's linked, now we don't have timeouts anymore.

@tupui tupui force-pushed the python_3_10_mac_win branch from ad2cc1d to 11cca3b Compare October 20, 2021 19:14
@tupui
Copy link
Contributor Author

tupui commented Oct 21, 2021

Everything is ok now 😃 The failure with Travis is just a tolerance and should be fixed soon. This PR is ready on my side.

@tupui tupui requested a review from mattip October 21, 2021 09:12
@tupui
Copy link
Contributor Author

tupui commented Oct 21, 2021

Since NumPy does not build 32bit builds on Linux, shall we do this here too?

Copy link
Collaborator

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Sounds like the remaining ARM64 test failures will be handled in the main repo. Some of the changes here I may need to be cautious with when backporting in wheels repo, like NUMPY_TEST_DEP version for Appveyor (that said, it looks like each matrix entry sets their own value anyway..).

I think it makes sense to merge this now and if we want to strip out remaining 32-bit stuff later we can (and separating the PRs that make this change makes it easier for me to backport support for Python 3.10 without removing 32-bit support in a given release minor version series).

@tylerjereddy tylerjereddy merged commit 92e0b31 into MacPython:master Oct 23, 2021
@tylerjereddy
Copy link
Collaborator

Thanks @tupui et al.

@tupui tupui deleted the python_3_10_mac_win branch October 24, 2021 08:11
@EwoutH
Copy link

EwoutH commented Nov 4, 2021

Could this PR be cherry-picked to the v1.7.x branch, and Python 3.10 wheels be built and uploaded to Pypi for scipy 1.7.1?

@tylerjereddy
Copy link
Collaborator

@EwoutH If you want to know what the hold up is, the later parts of the discussion here explain a bit: scipy/scipy#14567

Mostly we'll start moving forward again when the next NumPy release comes out with thin x86_64 MacOS Python 3.10 wheels, because there's a problem with double/quad precision mixup with the current NumPy Python 3.10 universal2 wheel.

@EwoutH
Copy link

EwoutH commented Nov 4, 2021

Thanks for the quick rely, link and explanation!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants