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

Fix attribute error in setup.py #576

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sveinung-r
Copy link
Contributor

This line was causing a error due to an update to scikit-build. The issue was that the setuptools.command.test module is not put into the symbol table by the setuptools import, but it was put there during the skbuild import causing it to be available. Due to changes in scikit-build this is no longer the case and the line gives an AttributError.

The rationale for this line was that scikit-builds test command implied develop (this was obnoxious), something that is no longer true. There is thus no longer any reason to keep this line, so we can fix this issue by simply removing it.

This line was causing a  error due to an update to scikit-build.  The
issue was that the setuptools.command.test module is not put into the
symbol table by the setuptools import, but it was put there during the
skbuild import causing it to be available. Due to changes in
scikit-build this is no longer the case and the line gives an
AttributError.

The rationale for this line was that scikit-builds test command implied
develop (this was obnoxious), something that is no longer true. There is
thus no longer any reason to keep this line, so we can fix this issue by
simply removing it.
Parsing segyio.BinField type as int in PyArg_ParseTuple is no longer
possible.
From Numpy 2.0 adding a numpy.float32 and a Python numeric type returns
a numy.float32 when it previously returned a numpy.float64. This changes
the behavior when using the Python builtin sum function on a
numpy.float32 array as the internal computations now will be performed
as numpy.float32 additions when it used to be numpy.float64.

Passing a numpy.double(0) as a start value to the innermost sum forces
the old behavior and provides consistent results for Numpy 1 and 2.
MacOS xlarge runners are no longer required for building on Apple arm64
architecture.
@sveinung-r sveinung-r force-pushed the master branch 4 times, most recently from 22fecb5 to 879dd76 Compare July 31, 2024 10:41
The latest runner for MacOS is running on arm64 architecture. The
cibuildwheel cross compiling for x86 fails, so we build these wheels on
the latest MacOS runner that runs on x86 (macos-13).
@sveinung-r sveinung-r force-pushed the master branch 4 times, most recently from 44708ed to fb5fe15 Compare August 2, 2024 12:26
@sveinung-r sveinung-r force-pushed the master branch 2 times, most recently from e66781e to 46e56da Compare August 23, 2024 11:37
Python 3.13 is still in pre-release, but cibuildwheels has already
included it in the matrix. Some tests are failing on 3.13, but let's
wait to see if this is resolved by the official release before spending
time on debugging.
@tjni
Copy link

tjni commented Oct 21, 2024

@sveinung-r Hello! I'm trying to fix the build for this package in nixpkgs, which is broken due to this error.

  1. I notice quite a few commits. Are they unrelated to fix CI issues, or are they related to this change?
  2. What's the status of this patch being merged?

Thank you for creating this patch and doing the leg work of figuring out what is going on.

@ajaust
Copy link
Contributor

ajaust commented Oct 21, 2024

Hei @tjni,

Awesome that you package segyio for Nix. :)

I cannot comment on the first point, but I think it is still the plan to merge the PR.
The PR was blocked due to the failing test on aarch64. The issue was tracked down to be an issue with np.unique. Afaik that was fixed, but we were still waiting for a new release of NumPy for an older Python release that we support (I think 3.9). I see that there are several new NumPy releases. We will check if can merge this PR soon.

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.

3 participants