Skip to content

Conversation

@thalassemia
Copy link
Contributor

@thalassemia thalassemia commented Dec 5, 2024

This removes the deprecated distutils in favor of setuptools and replaces the setup.py CLI with pip and build.

Here is a table of environments where python stochastic_arrow/test/test_arrow.py ran without issue after installing either an sdist (platform-agnostic) or wheel (platform-specific) that I built on macOS 15.1.1, Python 3.12, Cython 3.0.11, and Numpy 2.1.3 with python -m build --sdist/wheel:

Operating System Python version Numpy version Build type
Docker debian:latest 2.7 1.14.0 sdist
Docker debian:latest 2.7 1.16.0 sdist
macOS 15.1.1 3.12 1.26.4 sdist
macOS 15.1.1 3.12 1.26.4 wheel

It's pretty cool that this can still be installed on Python 2.7 with just a few tweaks. The last row suggests that building with Numpy 2.1.3 does not make the wheel incompatible with older versions of Numpy.

Also, I added a cibuildwheel GitHub Action that should automatically build and publish wheels for all common platforms and recent Python versions. It should run pytest on every wheel it builds so we won't accidentally upload a non-functional wheel.

I got the wheel working on the new free-threaded release of Python 3.13. Unfortunately, it was almost universally slower than the normal Python 3.13 both to build and to run pytest.

@thalassemia thalassemia force-pushed the numpy2 branch 2 times, most recently from 0d64146 to 79aebce Compare December 5, 2024 12:01
@thalassemia thalassemia force-pushed the numpy2 branch 3 times, most recently from 6144a57 to 92b6482 Compare December 5, 2024 12:11
@thalassemia thalassemia force-pushed the numpy2 branch 4 times, most recently from cbd1543 to 9bf5d57 Compare December 5, 2024 13:05
@thalassemia thalassemia force-pushed the numpy2 branch 2 times, most recently from 7b3d070 to ea4baf6 Compare December 5, 2024 21:53
@thalassemia
Copy link
Contributor Author

In the process of getting pytest to work with cibuildwheel, I realized that our current directory structure can lead to import issues when trying to test a wheel that you built and installed. Specifically, calling pytest inside the root directory will import the stochastic_arrow source files instead of the installed wheel.

To fix this, I decided to follow PyPA's recommendation to have all package source files inside a src directory. I could then move the test directory out to the top level and move the data directory into test. From there, I just had to modify a few tests to load data from the right place. Now, pytest works seamlessly regardless of whether it is using an editable install ("Pytest" GitHub Action) or an installed wheel ("Build and upload to PyPI" GitHub Action).

Also, I added a temporary job to release.yml to upload the built wheels to TestPyPI, a mock PyPI. You can view the results of that here and try installing using pip install -i https://test.pypi.org/simple/ stochastic-arrow.

* Fix the clunky CLI test runner.
  * Update the README about it.
  * The `--plot` test requires installing matplotlib, in particular, one that's compatible with the installed numpy.
  * Moved the `analysis/` directory into the `test/` directory for the tests to import.
  * TODO: Fix the `--memory` test, which fails `assert memory_increases <= 10` on macOS.
  * TODO: Fix the `--plot` test, which raises `ValueError: x and y must have same first dimension, but have shapes (115,) and (114, 2469)` from matplotlib.
* Delete `release.sh` which would be risky to run now and has been supplanted by GitHub Actions. Actually my process was to copy/paste from this script rather than run it as is.

Q. Do *.pxd header files need `freethreading_compatible = True` or is it sufficient in the .pyx module?
@1fish2 1fish2 self-assigned this Dec 7, 2024
Copy link
Contributor

@1fish2 1fish2 left a comment

Choose a reason for hiding this comment

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

This looks great!

I fixed the CLI test runner although bugs remain in the --plot and --memory tests. See the commit message.

@thalassemia thalassemia mentioned this pull request Dec 8, 2024
3 tasks
@thalassemia
Copy link
Contributor Author

Thanks for the review and edits @1fish2! I noticed that the memory test in its current form fails on MacOS and changed it to assert memory_increases <= 10 instead of <= 1. Not sure why though, or if it's even worth digging into. The plot test is not included in our pytest test suite so I'm fine with that one failing.

Copy link
Member

@prismofeverything prismofeverything left a comment

Choose a reason for hiding this comment

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

Huge improvement all around, thanks for bringing arrow into the modern age (!) This pattern is very useful - if it were worthwhile this could be the basis of a cython template that could be reused in the future (just had to go through a similar thing recently).

@prismofeverything
Copy link
Member

I granted trusted publisher access to this repository for the package on pypi in the release.yml workflow, let me know if additional configuration is required there.

@thalassemia thalassemia marked this pull request as ready for review December 12, 2024 00:56
@thalassemia thalassemia merged commit d93454f into master Dec 12, 2024
35 checks passed
@thalassemia thalassemia deleted the numpy2 branch December 12, 2024 01:03
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