Add support for cython 3 #1143

tommy-waltmann · 2023-08-28T21:52:40Z

This PR enables builds of freud with cython>=3.0.2

Description

Small changes were made to accommodate changes to cython in their 3.0 release.

Motivation and Context

Cython has made a major release, which we need to support so we can continue to be compatible with new python versions as they are released.
Resolves: #1130

How Has This Been Tested?

We need to bump the newest supported version of cython in our latest testing configuration. Older testing configurations will continue to test against cython<3.0.0

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds or improves functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
I have updated the documentation (if relevant).
I have added tests that cover my changes (if relevant).
All new and existing tests passed.
I have updated the credits.
I have updated the Changelog.

bdice · 2023-08-29T19:00:25Z

freud/box.pyx

+        # NOTE: cython<3.0 treats __mul__ and __rmul__ as one operation, so
+        # type checks are needed. For cython>=3.0, __rmul__ is implemented


Do you need to support both Cython < 3 and Cython >= 3? I would strongly recommend requiring Cython 3, which would mean this note isn't needed.

Why require Cython >= 3? Won't people want to be able to build freud with older cython versions too?

I don't think this will be a problem. I think you can require Cython 3 with no harm done. Most users will only need prebuilt packages, and those who do wish to build freud from source can do so with build isolation (on by default), which will install Cython into a temporary environment. Most Cython-using packages I know are just updating the requirement to 3.0.

This was marked as resolved but does not appear to be resolved. I expected the comment would be deleted, and this would not require a utility method _mul_impl. Try just implementing __mul__(self, scale) without the argument inspection (assume the second argument is a scale) and then call __mul__ in __rmul__.

def __rmul__(self, scale): return self * scale

I bumped the requirements to all have cython>=3.0.2, but forgot to simplify the implementation of mul and rmul. Should be good now.

freud/locality.pyx

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

DomFijan · 2023-08-30T03:44:32Z

I've stumbled into some chatter about performance regression re cython 3.0 ( scikit-learn/scikit-learn#27086 ) . Would it make sense to benchmark before / after to see if we would be affected? Maybe not everything but just they key components that heavily rely on cython (msd?).

bdice · 2023-08-30T15:35:02Z

Most of freud isn't doing heavy lifting in Cython -- it all happens in the C++ layer. I don't expect this to be a problem for freud which just uses Cython for bindings to C++. Always good to benchmark though. :)

edit: even MSD is mostly calling out to other libraries in C++, if I remember correctly.

tommy-waltmann · 2023-08-30T19:23:27Z

I've run the MSD benchmark with cython 0.x and 3.x and there appears to be a very small performance slowdown. Given that all other benchmarks use less cython than MSD, I don't think we need to worry about performance regressions.

Running with cython 0.x:

tomwalt@killian ~/code/freud $ python benchmarks/benchmark_msd_MSD.py
freud.msd.MSD:
        L = 10, mode = window
       100:    0.156 ms | 1557.096 ns per item
       500:    0.380 ms |  760.656 ns per item
      1000:    0.679 ms |  679.278 ns per item
      5000:    5.391 ms | 1078.285 ns per item
Threads          100           |          500           |          1000          |          5000          |
      1    0.155 ms      1.00x |    0.378 ms      1.00x |    0.664 ms      1.00x |    5.462 ms      1.00x |
      2    0.156 ms      1.00x |    0.376 ms      1.01x |    0.660 ms      1.01x |    5.518 ms      0.99x |
      3    0.154 ms      1.01x |    0.381 ms      0.99x |    0.676 ms      0.98x |    5.281 ms      1.03x |
      4    0.154 ms      1.01x |    0.380 ms      1.00x |    0.698 ms      0.95x |    5.293 ms      1.03x |
      5    0.155 ms      1.00x |    0.371 ms      1.02x |    0.673 ms      0.99x |    5.420 ms      1.01x |
      6    0.154 ms      1.01x |    0.374 ms      1.01x |    0.662 ms      1.00x |    5.315 ms      1.03x |
      7    0.156 ms      0.99x |    0.377 ms      1.01x |    0.676 ms      0.98x |    5.317 ms      1.03x |
      8    0.153 ms      1.01x |    0.374 ms      1.01x |    0.669 ms      0.99x |    5.234 ms      1.04x |
      9    0.154 ms      1.01x |    0.372 ms      1.02x |    0.679 ms      0.98x |    5.580 ms      0.98x |
     10    0.153 ms      1.01x |    0.379 ms      1.00x |    0.659 ms      1.01x |    5.249 ms      1.04x |
     11    0.169 ms      0.92x |    0.374 ms      1.01x |    0.674 ms      0.99x |    5.353 ms      1.02x |
     12    0.154 ms      1.01x |    0.374 ms      1.01x |    0.650 ms      1.02x |    5.442 ms      1.00x |
     13    0.155 ms      1.00x |    0.402 ms      0.94x |    0.655 ms      1.01x |    5.364 ms      1.02x |
     14    0.154 ms      1.01x |    0.372 ms      1.02x |    0.690 ms      0.96x |    5.287 ms      1.03x |
     15    0.155 ms      1.00x |    0.374 ms      1.01x |    0.659 ms      1.01x |    5.592 ms      0.98x |
     16    0.154 ms      1.01x |    0.385 ms      0.98x |    0.669 ms      0.99x |    5.223 ms      1.05x |

Running with cython 3.x:

freud.msd.MSD:
        L = 10, mode = window
       100:    0.154 ms | 1541.408 ns per item
       500:    0.386 ms |  771.820 ns per item
      1000:    0.688 ms |  688.213 ns per item
      5000:    5.640 ms | 1127.987 ns per item
Threads          100           |          500           |          1000          |          5000          |
      1    0.161 ms      1.00x |    0.397 ms      1.00x |    0.691 ms      1.00x |    5.520 ms      1.00x |
      2    0.156 ms      1.03x |    0.383 ms      1.04x |    0.669 ms      1.03x |    5.500 ms      1.00x |
      3    0.157 ms      1.03x |    0.399 ms      0.99x |    0.692 ms      1.00x |    5.357 ms      1.03x |
      4    0.159 ms      1.01x |    0.377 ms      1.05x |    0.657 ms      1.05x |    5.832 ms      0.95x |
      5    0.151 ms      1.06x |    0.375 ms      1.06x |    0.663 ms      1.04x |    5.434 ms      1.02x |
      6    0.152 ms      1.06x |    0.375 ms      1.06x |    0.665 ms      1.04x |    5.580 ms      0.99x |
      7    0.150 ms      1.07x |    0.373 ms      1.06x |    0.669 ms      1.03x |    5.488 ms      1.01x |
      8    0.151 ms      1.07x |    0.383 ms      1.04x |    0.653 ms      1.06x |    5.405 ms      1.02x |
      9    0.150 ms      1.07x |    0.367 ms      1.08x |    0.669 ms      1.03x |    5.174 ms      1.07x |
     10    0.151 ms      1.06x |    0.373 ms      1.06x |    0.672 ms      1.03x |    5.462 ms      1.01x |
     11    0.150 ms      1.07x |    0.377 ms      1.05x |    0.667 ms      1.04x |    5.223 ms      1.06x |
     12    0.150 ms      1.07x |    0.378 ms      1.05x |    0.666 ms      1.04x |    5.441 ms      1.01x |
     13    0.157 ms      1.02x |    0.397 ms      1.00x |    0.681 ms      1.02x |    5.291 ms      1.04x |
     14    0.153 ms      1.05x |    0.377 ms      1.05x |    0.701 ms      0.99x |    5.354 ms      1.03x |
     15    0.154 ms      1.05x |    0.373 ms      1.06x |    0.664 ms      1.04x |    5.601 ms      0.99x |
     16    0.153 ms      1.05x |    0.385 ms      1.03x |    0.678 ms      1.02x |    5.418 ms      1.02x |

for more information, see https://pre-commit.ci

bdice · 2023-08-31T14:41:58Z

benchmarks/reports/benchmark.json

@@ -692,5 +692,6 @@
                ]
            ]
        }
-    ]
+    ],
+    "5ddf603fa828b2130199b90d39fe4cb02072007c": []


What’s this change?

The benchmarking script records the SHA of the commit it was run on and the results. I cut the benchmarking script off before it got all the way done so it couldn't record all the results at the end. I can manually revert this change if necessary.

tommy-waltmann added 4 commits August 28, 2023 17:44

tests pass in a cython 3.0.2 environment

5a32216

maintain backward compatibility with older cython versions

32d2773

fix compile warnings

e94d849

undo previous commit

6ece766

tommy-waltmann added this to the v2.13.1 milestone Aug 29, 2023

bumpy cython requirement

44242c1

tommy-waltmann added the build_wheels label Aug 29, 2023

update changelog and credits

f96a65f

tommy-waltmann marked this pull request as ready for review August 29, 2023 18:53

tommy-waltmann requested review from a team as code owners August 29, 2023 18:53

tommy-waltmann requested review from bdice and removed request for a team August 29, 2023 18:53

bdice reviewed Aug 29, 2023

View reviewed changes

tommy-waltmann and others added 2 commits August 29, 2023 16:30

Update freud/locality.pyx

5677689

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

remove extra line

5ddf603

tommy-waltmann added 2 commits August 30, 2023 15:42

require cython>=3.0.2

49bec57

small benchmarks fix

e1dfd93

tommy-waltmann requested a review from bdice August 30, 2023 19:43

pre-commit-ci bot and others added 3 commits August 30, 2023 19:44

[pre-commit.ci] auto fixes from pre-commit.com hooks

7de5078

for more information, see https://pre-commit.ci

mul and rmul assume cython 3

0771ad4

Merge branch 'cython-3' of github.com:glotzerlab/freud into cython-3

21b829c

bdice approved these changes Aug 31, 2023

View reviewed changes

tommy-waltmann merged commit afcde37 into master Sep 1, 2023
28 checks passed

tommy-waltmann deleted the cython-3 branch September 1, 2023 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cython 3 #1143

Add support for cython 3 #1143

tommy-waltmann commented Aug 28, 2023

bdice Aug 29, 2023

tommy-waltmann Aug 29, 2023

bdice Aug 29, 2023 •

edited

Loading

bdice Aug 31, 2023 •

edited

Loading

tommy-waltmann Aug 31, 2023 •

edited

Loading

DomFijan commented Aug 30, 2023 •

edited

Loading

bdice commented Aug 30, 2023 •

edited

Loading

tommy-waltmann commented Aug 30, 2023

bdice Aug 31, 2023

tommy-waltmann Aug 31, 2023

		# NOTE: cython<3.0 treats __mul__ and __rmul__ as one operation, so
		# type checks are needed. For cython>=3.0, __rmul__ is implemented

Add support for cython 3 #1143

Add support for cython 3 #1143

Conversation

tommy-waltmann commented Aug 28, 2023

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

bdice Aug 29, 2023

Choose a reason for hiding this comment

tommy-waltmann Aug 29, 2023

Choose a reason for hiding this comment

bdice Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

bdice Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

tommy-waltmann Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

DomFijan commented Aug 30, 2023 • edited Loading

bdice commented Aug 30, 2023 • edited Loading

tommy-waltmann commented Aug 30, 2023

bdice Aug 31, 2023

Choose a reason for hiding this comment

tommy-waltmann Aug 31, 2023

Choose a reason for hiding this comment

bdice Aug 29, 2023 •

edited

Loading

bdice Aug 31, 2023 •

edited

Loading

tommy-waltmann Aug 31, 2023 •

edited

Loading

DomFijan commented Aug 30, 2023 •

edited

Loading

bdice commented Aug 30, 2023 •

edited

Loading