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

Update setup.py / pyproject.toml to follow latest numpy and cython docs #271

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Nov 17, 2023

Remove all the magic for allowing compilation of bundled c files without Cython, this was obsolete since we python Cython into the build requirements in pyproject.toml. Supporting the case where people don't have cython doesn't make sense, since pip ensures it.

Numpy 1.25 made oldest-supported-numpy obsolete, we can chose the lowest version of numpy we want to support with a definition as long as we compile using at least numpy 1.25.

orelgueta
orelgueta previously approved these changes Nov 17, 2023
Copy link
Contributor

@orelgueta orelgueta left a comment

Choose a reason for hiding this comment

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

I had to read a bit to figure out why in pyproject.toml you require numpy>=1.25 and in setup.py you require numpy>=1.21 and NPY_1_21_API_VERSION, but I think I understood and it's not a bug, so I approve (the one comment I left is not critical).

setup.py Outdated Show resolved Hide resolved
@maxnoe
Copy link
Member Author

maxnoe commented Nov 20, 2023

Dropping python 3.8 required for the numpy >= 1.25 requirement. But according to the NEP 29 schedule, this was overdue anyway and e.g. ctapipe has already done it.

@maxnoe
Copy link
Member Author

maxnoe commented Nov 20, 2023

I had to read a bit to figure out why in pyproject.toml you require numpy>=1.25 and in setup.py you require numpy>=1.21 and NPY_1_21_API_VERSION, but I think I understood and it's not a bug, so I approve (the one comment I left is not critical).

Yes, this is a bit confusing, but it boils down to:

  • numpy 1.25 introduced a new feature of stable C-API / ABI with a selectable lower version of numpy you want to support
  • We need to compile the extension against at least 1.25 to make use of this feature, but the resulting compiled extension will be compatible with all numpy versions larger than the one we defined.

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (72c9715) 86.58% compared to head (ef8d1dd) 86.58%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #271   +/-   ##
=======================================
  Coverage   86.58%   86.58%           
=======================================
  Files          24       24           
  Lines        2222     2222           
=======================================
  Hits         1924     1924           
  Misses        298      298           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@orelgueta orelgueta left a comment

Choose a reason for hiding this comment

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

All good now!

@maxnoe maxnoe merged commit feb2031 into main Nov 20, 2023
6 checks passed
@maxnoe maxnoe deleted the update_build_system branch November 20, 2023 13:30
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