Skip to content

Compile on numpy2#2964

Merged
benjeffery merged 1 commit intotskit-dev:mainfrom
jeromekelleher:compile-numpy2
Jun 27, 2024
Merged

Compile on numpy2#2964
benjeffery merged 1 commit intotskit-dev:mainfrom
jeromekelleher:compile-numpy2

Conversation

@jeromekelleher
Copy link
Member

Part of supporting numpy >= 2. Following the ABI advice from numpy, we should be able to compile against numpy version >= 2, and also have a runtime dependency < 2. Hopefully this is all we need to do initially to ship compatible wheels and conda packages (which are also broken)

@codecov
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.62%. Comparing base (f467db6) to head (bb137ff).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2964   +/-   ##
=======================================
  Coverage   89.62%   89.62%           
=======================================
  Files          29       29           
  Lines       30170    30170           
  Branches     5867     5867           
=======================================
  Hits        27041    27041           
  Misses       1790     1790           
  Partials     1339     1339           
Flag Coverage Δ
c-tests 86.20% <ø> (ø)
lwt-tests 80.78% <ø> (ø)
python-c-tests 88.72% <ø> (ø)
python-tests 99.01% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
python/tskit/formats.py 99.46% <100.00%> (ø)

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

Looks good, to test wheel building, push to origin/test

fail-fast: false
matrix:
python: [ 3.8, 3.9, "3.11" ]
python: [ 3.9, 3.11, 3.12 ]
Copy link
Member

Choose a reason for hiding this comment

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

If test versions are changing then mergify rules need updating.

@jeromekelleher
Copy link
Member Author

Looks like this is mostly working now. Outstanding issues:

  • Some tests are failing on most recent numpy 1.x version and also on numpy 2.x.
  • We need numpy 2.x compatible kastore distributions, so parking this until that's ready.

@jeromekelleher
Copy link
Member Author

Shall I pick this up or is it simpler to start again @benjeffery? The kastore conda-forge packages should be built and available in the next few hours, so that should unblock us here.

@benjeffery
Copy link
Member

Shall I pick this up

I'm working on this now!

Copy link
Member Author

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Actual code changes look small, great!

python/setup.py Outdated

# Obscure magic required to allow numpy be used as a 'setup_requires'.
# Based on https://stackoverflow.com/questions/19919905
class local_build_ext(build_ext):
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need this at all any more?

Copy link
Member

Choose a reason for hiding this comment

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

Good spot, removed in 03e4091

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, removing this worked locally, but not on CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Kastore works without it, check how we do it there

w_aB = state[2, k]
w_ab = n - (w_AB + w_Ab + w_aB)
with suppress_division_by_zero_warning():
with np.errstate(over="ignore", divide="ignore", invalid="ignore"):
Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't it be better to update the original function here rather than duplicating?

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in a8cb721

@benjeffery
Copy link
Member

All seems to be working - I'll squash and merge.

@benjeffery benjeffery force-pushed the compile-numpy2 branch 2 times, most recently from ffead55 to b1cd1bb Compare June 27, 2024 11:03
@benjeffery
Copy link
Member

Realised we were doing wheels for 3.12, but not testing on it. Fixing that now

Copy link
Member Author

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM! What's the strategy for pushing an updated release - cherry pick this back onto last release tag?

@benjeffery
Copy link
Member

LGTM! What's the strategy for pushing an updated release - cherry pick this back onto last release tag?

Seems the simplest approach.

@benjeffery benjeffery merged commit beafeba into tskit-dev:main Jun 27, 2024
@jeromekelleher jeromekelleher deleted the compile-numpy2 branch June 27, 2024 12:45
This was referenced Sep 23, 2024
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.

2 participants