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

Rtree Wheel Infrastructure #163

Merged
merged 134 commits into from
Dec 16, 2020
Merged

Rtree Wheel Infrastructure #163

merged 134 commits into from
Dec 16, 2020

Conversation

mikedh
Copy link
Contributor

@mikedh mikedh commented Aug 10, 2020

Hey, thanks for the great project! I made a pass at building wheels that bundle libspatialindex as it seems like something that trips people up. Happy to close this if it isn't desirable, and thought I'd open it for feedback before I put more time into it.

Changes

  • Breaks out library finding logic into separate rtree/finder submodule so that setup.py can reuse it.
  • Refactors or replaces wheel building logic with cibuildwheel. I think it's a great project, and I've been able to get it to build every flavor of wheel for other projects.

Status

  • without tests enabled this is building wheels successfully on every flavor of Python on Linux.
  • I think I'm running into actual issues on some variants of Python that I don't fully understand. The nice thing about cibuildwheel is it runs the tests using the version of the package from the actual wheel and these may be actual issues.

Todo

  • Fix tests on Linux platforms.
  • Better finder logic for "module path" (it's currently a cheap hack that only works on Linux for testing).
  • Get wheels building on MacOS and Windows.

Copy link
Member

@hobu hobu left a comment

Choose a reason for hiding this comment

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

This is awesome. 🎊

  • There are flake8 failures now that must be cleaned up.
  • Is there a build/test loop of the wheels for each platform? That's what I was trying to do with my scripts and not having much success with. Its great that the wheels are built, but we have no way to know they haven't rotted without a test loop.

@mikedh mikedh marked this pull request as draft August 10, 2020 16:36
@mikedh
Copy link
Contributor Author

mikedh commented Aug 10, 2020

This is awesome. 🎊

Great! I'll try to get this over the finish line then haha. It might be a few weeks, there is potentially a lot of surface area here 😁.

There are flake8 failures now that must be cleaned up.

Absolutely, I'll do a formatting and docstring pass before removing the [WIP] and draft-pr tags and make sure linters pass.

Is there a build/test loop of the wheels for each platform? That's what I was trying to do with my scripts and not having much success with. Its great that the wheels are built, but we have no way to know they haven't rotted without a test loop.

Yeah that is my favorite part: https://cibuildwheel.readthedocs.io/en/stable/options/#test-command
The only thing I'm not totally certain of is how to make sure that it's using the libspatialindex from the wheel, although I guess we could not do a make install.

Any ideas about that cp27m failure? Looks like a dtype thing:

x = array([ 2, 92, 51, 55, 26, 95,  7, 81, 38, 22, 58, 89, 91, 83, 98, 37, 70,
   ...6, 64, 79, 94, 40, 32, 46, 47, 15, 68, 10,  0, 80, 56,
       50], dtype=int64)
y = array([ 2., 92., 51., 55., 26., 95.,  7., 81., 38., 22., 58., 89., 91.,
      ..., 79., 94., 40., 32., 46., 47., 15., 68., 10.,
        0., 80., 56., 50., 30.])
atol = 1e-08, rtol = 1e-05

    def within_tol(x, y, atol, rtol):
        with errstate(invalid='ignore'):
>           return less_equal(abs(x-y), atol + rtol * abs(y))
E           ValueError: operands could not be broadcast together with shapes (69,) (70,)

@hobu
Copy link
Member

hobu commented Aug 10, 2020

Any ideas about that cp27m failure? Looks like a dtype thing:

No. We were planning to ditch 2.7 soon anyway.

@hobu
Copy link
Member

hobu commented Dec 14, 2020

Making progress?

@mikedh
Copy link
Contributor Author

mikedh commented Dec 14, 2020

Yeah I think this is pretty much ready to go! I've done some manual verification of these wheels here and there, and they seem to work fine. Although there are approximately a bajillion of them, I haven't manually verified all of them.

  1. Wheels on Linux/Mac/Windows are building properly and running tests using the wheel against the whole matrix. The shared library search location repair is now done in the CI build scripts, and then we delete build directories and skip make install to make sure tests are being run with the wheel copy of the shared library (system-wide libraries were causing false success at certain points.)
  2. I changed finder logic to work with libspatialindex-dev installed system-wide but as a last resort on posix.
  3. Conda build looks like it was fixed by adding search locations, but I haven’t tested this manually.
  4. To get this to upload wheels to pypi you’ll have to add PYPI_USERNAME and PYPI_PASSWORD through the repo’s secrets interface and remove that if false

Here's the final tally:

Rtree-0.9.5-cp27-cp27m-linux_i686.whl
Rtree-0.9.5-cp27-cp27m-linux_x86_64.whl
Rtree-0.9.5-cp27-cp27m-macosx_10_15_x86_64.whl
Rtree-0.9.5-cp27-cp27mu-linux_i686.whl
Rtree-0.9.5-cp27-cp27mu-linux_x86_64.whl
Rtree-0.9.5-cp27-cp27m-win_amd64.whl
Rtree-0.9.5-cp35-cp35m-linux_i686.whl
Rtree-0.9.5-cp35-cp35m-linux_x86_64.whl
Rtree-0.9.5-cp35-cp35m-macosx_10_15_x86_64.whl
Rtree-0.9.5-cp35-cp35m-win_amd64.whl
Rtree-0.9.5-cp36-cp36m-linux_i686.whl
Rtree-0.9.5-cp36-cp36m-linux_x86_64.whl
Rtree-0.9.5-cp36-cp36m-macosx_10_15_x86_64.whl
Rtree-0.9.5-cp36-cp36m-win_amd64.whl
Rtree-0.9.5-cp37-cp37m-linux_i686.whl
Rtree-0.9.5-cp37-cp37m-linux_x86_64.whl
Rtree-0.9.5-cp37-cp37m-macosx_10_15_x86_64.whl
Rtree-0.9.5-cp37-cp37m-win_amd64.whl
Rtree-0.9.5-cp38-cp38-linux_i686.whl
Rtree-0.9.5-cp38-cp38-linux_x86_64.whl
Rtree-0.9.5-cp38-cp38-macosx_10_15_x86_64.whl
Rtree-0.9.5-cp38-cp38-win_amd64.whl
Rtree-0.9.5-cp39-cp39-linux_i686.whl
Rtree-0.9.5-cp39-cp39-linux_x86_64.whl
Rtree-0.9.5-cp39-cp39-macosx_10_15_x86_64.whl
Rtree-0.9.5-cp39-cp39-win_amd64.whl

@mikedh mikedh marked this pull request as ready for review December 14, 2020 22:37
@mikedh mikedh changed the title [WIP] Rtree Wheel Infrastructure Rtree Wheel Infrastructure Dec 14, 2020
@mikedh
Copy link
Contributor Author

mikedh commented Dec 15, 2020

OK, manually verified the wheels from here:
https://github.com/mikedh/rtree/actions/runs/422194320

I made sure there was a fresh install (also check location of rtree.core.rt) and ran a simple 2D index check on the following:

  • MacOS Python 3.9
  • Windows x86_64 Python 3.8
  • Linux x86_64 Python 3.8

This is probably fine to release, although happy to take additional feedback. Also anyone who wants to test the wheels at that link feel free, and if you could post your platform/version that would be great.

@hobu
Copy link
Member

hobu commented Dec 15, 2020

This is probably fine to release, although happy to take additional feedback.

Awesome, I'll push it out today or tomorrow

@hobu hobu merged commit 14fb3f0 into Toblerity:master Dec 16, 2020
@hobu
Copy link
Member

hobu commented Dec 18, 2020

Released. https://pypi.org/project/Rtree/

The GHAs need to be updated to capture the wheels as artifacts and the Publish GHA needs to be updated to push them to PyPI upon release.

@hobu
Copy link
Member

hobu commented Dec 18, 2020

An unfortunate failure on Windows Python 3.6 when the Conda package builds. Unclear why it is any different than 3.7 or 3.8, however. https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=253701&view=logs&j=171a126d-c574-5c8c-1269-ff3b989e923d&t=1183ba29-a0b5-5324-8463-2a49ace9e213&l=393

@henryiii
Copy link

The GHAs need to be updated to capture the wheels as artifacts and the Publish GHA needs to be updated to push them to PyPI upon release.

This might be helpful?
https://scikit-hep.org/developer/gha_wheels

@mikedh
Copy link
Contributor Author

mikedh commented Dec 22, 2020

Thanks for merging! This will probably generate a whole bunch of issues like #175 just given the volume of platforms haha. I'll try to help address the higher priority/segfault/not-functioning ones when I get a chance.

@hobu
Copy link
Member

hobu commented Dec 23, 2020

Do you happen to remember why you were copying in your own CMakeLists.txt file? What is missing in libspatialindex's?

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.

4 participants