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

Refactor cibuildwheel setup #276

Merged
merged 13 commits into from
Aug 24, 2023

Conversation

mwtoews
Copy link
Member

@mwtoews mwtoews commented Aug 18, 2023

There are a few aims in this PR:

  • Move cibuildwheel data from GitHub Actions variables to pyproject.toml
  • Build each platform, but allow to work with any Python 3 version, since this is a Pure Python project
  • Use tox to run pytest for all Python versions
  • For Linux, remove duplicated rtree/lib; close Linux binary wheels have two copies of libspatialindex #270
  • Modify finder to look for auditwheel-built shared library (using importlib.metadata.files)

The repair_wheel.py script was modified from cmake-python-distributions. Admittedly, there are several things that I don't understand, but have left alone. It uses wheel unpack and wheel pack to modify the binary wheels.

@mwtoews mwtoews force-pushed the mv-cibuildwheel-pyproject branch 2 times, most recently from 2cf3f52 to fec75fb Compare August 18, 2023 12:03
@mwtoews mwtoews force-pushed the mv-cibuildwheel-pyproject branch from fec75fb to cb719ba Compare August 18, 2023 12:05
@adamjstewart
Copy link
Collaborator

After this is merged we should consider cutting a 1.0.2 release so that we'll have wheels that can be installed on Python 3.12.

@mwtoews
Copy link
Member Author

mwtoews commented Aug 18, 2023

The failures with manylinux2014_i686.manylinux_2_17_i686 are puzzling, since there were no issues with the most-recent build. And for some reason the platform tags (separated by .) are flipped from manylinux_2_17_i686.manylinux2014_i686. Does the order of these matter? I'm able to reproduce these locally too, so they can be investigated further.

The macos wheels are also a bit odd. The previous build had two platform tags macosx_10_9_x86_64 and macosx_11_0_arm64 while the new has only one macosx_12_0_arm64.macosx_12_0_universal2.macosx_12_0_x86_64. Does this one wheel work for older platforms? I'm unable to do much testing with these.

@mwtoews
Copy link
Member Author

mwtoews commented Aug 18, 2023

Oh wait, the previous builds worked for manylinux*i686 since these tests were skipped with 6763788 . The skip could be re-instated, but I don't understand the root cause of the failures yet, they could be a 32-bit/64-bit thing.

Things are running oddly smooth for Windows, however! Additional platforms could be considered.

@mwtoews mwtoews force-pushed the mv-cibuildwheel-pyproject branch from 67d2b55 to 9041825 Compare August 20, 2023 10:25
@mwtoews
Copy link
Member Author

mwtoews commented Aug 20, 2023

I've enabled cp312 testing to see what happens. Currently testing is only possible on Linux, but all of the platforms show a consistent error. See #277 for this issue.

Output of error
  platform linux -- Python 3.12.0rc1, pytest-7.4.0, pluggy-1.2.0
  cachedir: .tox/py312/.pytest_cache
  rootdir: /project
  collected 38 items
  tests/test_index.py ........x............F...............                [ 97%]
  tests/test_tpr.py .                                                      [100%]
  =================================== FAILURES ===================================
  _____________________ IndexSerialization.test_interleaving _____________________
  self = <tests.test_index.IndexSerialization testMethod=test_interleaving>
      def test_interleaving(self) -> None:
          """Streaming against a persisted index without interleaving"""
      
          def data_gen(
              interleaved: bool = True,
          ) -> Iterator[Tuple[int, Tuple[float, float, float, float], int]]:
              for i, (minx, miny, maxx, maxy) in enumerate(self.boxes15):
                  if interleaved:
                      yield (i, (minx, miny, maxx, maxy), 42)
                  else:
                      yield (i, (minx, maxx, miny, maxy), 42)
      
          p = index.Property()
          tname = tempfile.mktemp()
          idx = index.Index(
              tname, data_gen(interleaved=False), properties=p, interleaved=False
          )
          hits1 = sorted(list(idx.intersection((0, 60, 0, 60))))
          self.assertTrue(len(hits1), 10)
          self.assertEqual(hits1, [0, 4, 16, 27, 35, 40, 47, 50, 76, 80])
      
  >       leaves = idx.leaves()
  /project/tests/test_index.py:475: 
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  self = rtree.index.Index(bounds=[-186.673789279, 184.761387556, -96.7177218184, 96.6043699778], size=100)
      def leaves(self):
          leaf_node_count = ctypes.c_uint32()
          p_leafsizes = ctypes.pointer(ctypes.c_uint32())
          p_leafids = ctypes.pointer(ctypes.c_int64())
          pp_childids = ctypes.pointer(ctypes.pointer(ctypes.c_int64()))
      
          pp_mins = ctypes.pointer(ctypes.pointer(ctypes.c_double()))
          pp_maxs = ctypes.pointer(ctypes.pointer(ctypes.c_double()))
          dimension = ctypes.c_uint32(0)
      
          core.rt.Index_GetLeaves(
              self.handle,
              ctypes.byref(leaf_node_count),
              ctypes.byref(p_leafsizes),
              ctypes.byref(p_leafids),
              ctypes.byref(pp_childids),
              ctypes.byref(pp_mins),
              ctypes.byref(pp_maxs),
              ctypes.byref(dimension),
          )
      
          output = []
      
          count = leaf_node_count.value
          sizes = ctypes.cast(p_leafsizes, ctypes.POINTER(ctypes.c_uint32 * count))
          ids = ctypes.cast(p_leafids, ctypes.POINTER(ctypes.c_int64 * count))
          child = ctypes.cast(
              pp_childids, ctypes.POINTER(ctypes.POINTER(ctypes.c_int64) * count)
          )
          mins = ctypes.cast(
              pp_mins, ctypes.POINTER(ctypes.POINTER(ctypes.c_double) * count)
          )
          maxs = ctypes.cast(
              pp_maxs, ctypes.POINTER(ctypes.POINTER(ctypes.c_double) * count)
          )
          for i in range(count):
              p_child_ids = child.contents[i]
      
              id = ids.contents[i]
              size = sizes.contents[i]
              child_ids_array = ctypes.cast(
                  p_child_ids, ctypes.POINTER(ctypes.c_int64 * size)
              )
      
              child_ids = []
              for j in range(size):
  >               child_ids.append(child_ids_array.contents[j])
  E               ValueError: NULL pointer access
  /project/rtree/index.py:1311: ValueError
  =============================== warnings summary ===============================
  tests/test_index.py::IndexSerialization::test_interleaving
    /project/rtree/index.py:283: DeprecationWarning: index.get_size() is deprecated, use len(index) instead
      warnings.warn(
  -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
  =========================== short test summary info ============================
  FAILED tests/test_index.py::IndexSerialization::test_interleaving - ValueErro...
  ============== 1 failed, 36 passed, 1 xfailed, 1 warning in 0.62s ==============

@mwtoews mwtoews marked this pull request as ready for review August 20, 2023 22:50
@mwtoews
Copy link
Member Author

mwtoews commented Aug 20, 2023

Finally ready for review. A few notes:

  • Firstly, I'm newish to cibuildwheel, so review by anyone with more experience would be appreciated. Would appreciate @EwoutH to take a look too.
  • There are many fewer built distribution files, e.g. linux went from 24 files 28.7 MB to 4 files 4.24 MB. This is a good thing.
  • macOS now has a platform tag: macosx_10_9_universal2.macosx_10_9_x86_64.macosx_11_0_arm64.macosx_11_0_universal2 which is better? Anyone with M1/M2 chip want to test this one?
  • There is still an issue with cp312, these errors are shown in the workflow, but the tox config ignore_outcome = True allows the wheel builds to succeed
  • This is still an issue with the i686 tests, but these don't seem to be major. The tests could be re-written to be more robust for 32-bit and 64-bits

@adamjstewart
Copy link
Collaborator

Anyone with M1/M2 chip want to test this one?

Me! Where can I download the wheel?

@adamjstewart
Copy link
Collaborator

Would it be possible to have separate wheels for x86_64 and arm64 on macOS? We might see better performance if binaries aren't running through Rosetta.

@mwtoews
Copy link
Member Author

mwtoews commented Aug 22, 2023

Go to the "Summary" of any build (e.g. here), scroll to the bottom and find the "Artifacts", e.g. macos-latest-whl.

As for separate macos builds, sure I can take another look at the repair wheel script. As mentioned previously, I don't really understand much about the macos stuff, so advice would be appreciated.

@adamjstewart
Copy link
Collaborator

I was able to import rtree. Anything else I should test?

I don't know much about cibuildwheel but I know some stuff about macOS. I'm going to guess that the macOS platform tag args are an artifact of when macOS arm64 first came out and no one had runners to properly build it so they instead built a single universal wheel. Are there any other repos that do something similar to this (single wheel for all Python versions) without universal wheels? I feel like there should be an easier way to do this than having a repair script... Maybe reach out to the cibuildwheel devs?

@pradyunsg @rgommers may also have ideas on how to create platform-specific Python-version-independent wheels with cibuildwheel.

@mwtoews mwtoews force-pushed the mv-cibuildwheel-pyproject branch from 2fae967 to 90e4990 Compare August 23, 2023 11:19
* Move before-all to before-build
* Remove delocate-wheel --require-archs (for now)
@mwtoews
Copy link
Member Author

mwtoews commented Aug 23, 2023

Note the previous macos artifacts have macosx_10_9_x86_64 and macosx_11_0_arm64 platform tags, but they are actually both x86_64 binaries. Testing again with CMAKE_OSX_ARCHITECTURES from ARCHFLAGS variable...

@mwtoews
Copy link
Member Author

mwtoews commented Aug 23, 2023

@adamjstewart mind testing the latest artifacts again? There are now two macOS wheels, each with only one architecture. Importing rtree is the first smoke test. But I suppose a performance benchmark would be more useful. There is no easy benchmark, unfortunately. The closest is benchmarks.py.off by @hobu

@adamjstewart
Copy link
Collaborator

New artifacts work great:

Stream load:
14350.47 usec/pass

One-at-a-time load:
58568.11 usec/pass


30000 points
Query box:  (1240000, 1010000, 1400000, 1390000)


Brute Force:
53 hits
1657.80 usec/pass

Memory-based Rtree Intersection:
53 hits
620.89 usec/pass

Disk-based Rtree Intersection:
53 hits
608.25 usec/pass

Disk-based Rtree Intersection without Item() wrapper (objects='raw'):
53 raw hits
47.25 usec/pass

Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Once this is merged we should think about cutting a 1.0.2 release so we have wheels for Python 3.12

@mwtoews mwtoews merged commit f41494c into Toblerity:master Aug 24, 2023
@mwtoews mwtoews deleted the mv-cibuildwheel-pyproject branch August 25, 2023 01:00
@mwtoews
Copy link
Member Author

mwtoews commented Aug 25, 2023

Thanks all for testing and feedback! Last few things to mention.

  1. The magic that rewrites the wheels is wheel tags, using --python-tag=py3 --abi-tag=none options. Additionally the unpack and pack steps are not really needed for Windows and macOS, but was there as a workaround for a (now closed) issue wheel tags adds directories to RECORD causing issues with pip uninstall pypa/wheel#558
  2. The new wheels with "none" ABI tag work with PyPy, which supports ctypes. I could have added pp{37,38,39} to tox.ini, perhaps later.
  3. Tests against each Python version are automated using tox. Tests are only run if a binary version of numpy is available, otherwise a long wait is needed to build numpy from source. This is done via tox option:
    install_command = python -I -m pip install --only-binary=:all: {opts} {packages}
  4. Tox tests always pass due to options ignore_outcome = True (and possibly ignore_errors = True). This means that manual inspection may be required, rather than just looking at a green checkmark.

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.

Linux binary wheels have two copies of libspatialindex
2 participants