python3Packages.numpy: fix failing tests and clang 16#244250
python3Packages.numpy: fix failing tests and clang 16#244250vcunat merged 3 commits intoNixOS:staging-nextfrom
Conversation
tjni
left a comment
There was a problem hiding this comment.
Apart from one comment, LGTM.
I think this should target staging-next (I think numpy will start to be broken there).
There was a problem hiding this comment.
I think we should use the commit link in case the PR has to be amended:
https://github.com/numpy/numpy/commit/609fee4324f3521d81a3454f5fcc33abb0d3761e.patch
There was a problem hiding this comment.
I switched it to use the commit link.
I also retargeted it to staging-next. |
There was a problem hiding this comment.
Please make this unconditional, none of the core python maintainers uses a Mac regularly to test numpy updates
There was a problem hiding this comment.
I can move the conditional to the Python-side, but it seems like the test should be run on platforms where it is expected to work. The patch itself just uses pytest.mark.skipif to skip the complex256 test on x86_64-darwin.
There was a problem hiding this comment.
Well, I see the complication that we already have lots of binaries on other platforms and this would make them rebuild again, slowing everything down.
There was a problem hiding this comment.
Then we can send a patch to staging afterwards making this unconditional
There was a problem hiding this comment.
Yes. It's slightly more complicated to do, but otherwise sounds like a very good solution.
There was a problem hiding this comment.
Won’t the other patches cause rebuilds regardless? Am I making all of the changes conditional?
There was a problem hiding this comment.
We should be able to sleep name this unconditional
There was a problem hiding this comment.
I can make this patch apply unconditionally. It can be dropped with the derivation is updated to numpy 1.25.
There was a problem hiding this comment.
We probably don't want to include the other patches based on distutils
There was a problem hiding this comment.
That was already in the derivation. I assumed it was necessary until numpy is updated and switched to meson.
There was a problem hiding this comment.
I was more hinting that we need to close the list here before adding the other patches.
There was a problem hiding this comment.
Oh, you’re right. I completely missed the conditional. I understand now and will push a fix for that piece.
There was a problem hiding this comment.
Yes, I meant talking about the whole bunch, as they don't seem useful/needed for our gcc linux builds.
There was a problem hiding this comment.
I made each of the patches conditional.
There was a problem hiding this comment.
This got uglier than I expected. Cleanup PR: #244461
6422c40 to
5c7d9aa
Compare
|
Latest push reflects the requested changes. |
* Fix an invalid function pointer conversion; and * Disable strict overflow hardening due to an unused argument warning that is promoted to -Werror.
The `atanhl` function is broken under Rosetta 2 with 80-bit long
doubles, which numpy uses to implement long double complex numbers. This
results in a test failure. Attempts were made to change the
implementation of things, but that just changed the breakage.
The following Swift program demonstrates the problem.
import Foundation
import Numerics
let x = Float80(1.00000000e-20)
let z = Complex(x)
print("X: \(x), Z: \(z)")
let x_atanh = Float80.atanh(x)
let z_atanh = Complex.atanh(z)
print("atanh:")
print("X: \(x_atanh), Z: \(z_atanh)")
let d = abs(x_atanh / z_atanh.real - 1)
print("d: \(d)")
On x86_64-darwin hardware, it prints the following:
X: 1e-20, Z: (1e-20, 0.0)
atanh:
X: 1e-20, Z: (1e-20, 0.0)
d: 0.0
On aarch64-darwin under Rosetta 2, it prints the following:
X: 1e-20, Z: (1e-20, 0.0)
atanh:
X: 1e-20, Z: (-1.0237493319595677839e-40, 0.0)
d: 9.7680161420558978584e+19
The latter is obviously incorrect. FB12656897 was submitted to Apple,
but even if this is fixed eventually, this derivation needs to build for
users (and Hydra) who aren’t on the latest version.
Description of changes
This PR fixes the failing tests on Darwin. The clang 16 fix was submitted upstream.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)