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

remove obsolete test skip #235

Merged
merged 1 commit into from
Jun 28, 2021
Merged

remove obsolete test skip #235

merged 1 commit into from
Jun 28, 2021

Conversation

h-vetinari
Copy link
Member

Builds on #234

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

@rgommers @charris @r-devulap @seiko2plus
Just FYI, I tried building 1.21.0rc1 based on the current conda-forge recipe (well, including #234, but those changes don't affect the build in any way), and this seems to have a new error on some linux builds (cpython 3.7 & 3.9, but not 3.8 or pypy), about not raising an error for something - so relatively low impact.

Not sure why this is not consistent across python versions, but the git blame seems to point to numpy/numpy#18920, and following that trail of crumbs, to numpy/numpy#18932 & numpy/numpy#18933 (though the error still appears despite the latter PR being included in rc1).

Test suite error log
=================================== FAILURES ===================================
______________________ TestSpecialFloats.test_exp_values _______________________

self = <numpy.core.tests.test_umath.TestSpecialFloats object at 0x7f1a528bb820>

    def test_exp_values(self):
        x = [np.nan,  np.nan, np.inf, 0.]
        y = [np.nan, -np.nan, np.inf, -np.inf]
        for dt in ['f', 'd', 'g']:
            xf = np.array(x, dtype=dt)
            yf = np.array(y, dtype=dt)
            assert_equal(np.exp(yf), xf)
    
        with np.errstate(over='raise'):
            assert_raises(FloatingPointError, np.exp, np.float32(100.))
            assert_raises(FloatingPointError, np.exp, np.float32(1E19))
            assert_raises(FloatingPointError, np.exp, np.float64(800.))
            assert_raises(FloatingPointError, np.exp, np.float64(1E19))
    
        with np.errstate(under='raise'):
            assert_raises(FloatingPointError, np.exp, np.float32(-1000.))
            assert_raises(FloatingPointError, np.exp, np.float32(-1E19))
>           assert_raises(FloatingPointError, np.exp, np.float64(-1000.))

../[...]/lib/python3.9/site-packages/numpy/core/tests/test_umath.py:998: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../[...]/lib/python3.9/unittest/case.py:733: in assertRaises
    return context.handle('assertRaises', args, kwargs)
../[...]/lib/python3.9/unittest/case.py:201: in handle
    callable_obj(*args, **kwargs)
../[...]/lib/python3.9/unittest/case.py:223: in __exit__
    self._raiseFailure("{} not raised by {}".format(exc_name,
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <unittest.case._AssertRaisesContext object at 0x7f1a528bba90>
standardMsg = 'FloatingPointError not raised by exp'

    def _raiseFailure(self, standardMsg):
        msg = self.test_case._formatMessage(self.msg, standardMsg)
E       AssertionError: FloatingPointError not raised by exp

../[...]/lib/python3.9/unittest/case.py:163: AssertionError
=============================== warnings summary ===============================

@r-devulap
Copy link

@h-vetinari Can you confirm if numpy/numpy#18933 is part of the build? I would think it should be ..

@h-vetinari
Copy link
Member Author

Hey @r-devulap, thanks for stopping by!

As I wrote above, I checked that numpy/numpy#18933 was part of 1.21.0rc1, as can be seen from numpy/numpy@cd73ab7 and https://github.com/numpy/numpy/commits/v1.21.0rc1/numpy/core/src/umath/loops_exponent_log.dispatch.c.src.

Since we're verifying the hash of the source tar for 1.21.0rc1, I'm 99.9% sure that it is included alreaady.

@h-vetinari
Copy link
Member Author

Actually, I just noticed that the release artefact changed (since the hashes don't match anymore). Let's try again if it works now.

@h-vetinari
Copy link
Member Author

The error persists on py37 & py39; To triple-check I also downloaded https://github.com/numpy/numpy/archive/refs/tags/v1.21.0rc1.tar.gz and verified that numpy/numpy#18933 is contained in the release.

@seiko2plus
Copy link

Could you execute the following code before running the tests so we can determine what kind of CPU features we're dealing with and also to check the current NumPy tag?

python -c "import numpy; numpy._pytesttester._show_numpy_info()"

@h-vetinari
Copy link
Member Author

So, on python 3.7, the addition of the line requested by @seiko2plus made the error go away (not sure if this run got a differently-specced CI agent or if this is a heisenbug), here's the output:

+ python -c 'import numpy; numpy._pytesttester._show_numpy_info()'
NumPy version 1.21.0rc1
NumPy relaxed strides checking option: True
NumPy CPU features:  SSE SSE2 SSE3 SSSE3* SSE41* POPCNT* SSE42* AVX* F16C* FMA3* AVX2* AVX512F* AVX512CD* AVX512_KNL? AVX512_KNM? AVX512_SKX* AVX512_CLX? AVX512_CNL? AVX512_ICL?

On python 3.9, the failure remained (with the following):

+ python -c 'import numpy; numpy._pytesttester._show_numpy_info()'
NumPy version 1.21.0rc1
NumPy relaxed strides checking option: True
NumPy CPU features:  SSE SSE2 SSE3 SSSE3* SSE41* POPCNT* SSE42* AVX* F16C* FMA3* AVX2* AVX512F? AVX512CD? AVX512_KNL? AVX512_KNM? AVX512_SKX? AVX512_CLX? AVX512_CNL? AVX512_ICL?

The difference seems to be AVX512F* AVX512CD* for the passing run, and AVX512F? AVX512CD? for the failing one.

@seiko2plus
Copy link

It makes sense now the error disappears when the machine has AVX512 support, it seems we are dealing with compiler/libc that doesn't respect underflow error since the current exp/f64 SIMD kernel only optimizes CPUs with AVX512 Skylake features.

I'm not sure what exactly you doing here since I see you requesting the compiler to tune the whole lib for Haswell CPUs through env var CFLAGS.

CFLAGS="-mtune=haswell"

However, Could you try the following patch?

index 41e0bf37b..89798f74a 100644
--- a/numpy/core/src/umath/loops_exponent_log.dispatch.c.src
+++ b/numpy/core/src/umath/loops_exponent_log.dispatch.c.src
@@ -1248,6 +1248,7 @@ NPY_NO_EXPORT void NPY_CPU_DISPATCH_CURFX(FLOAT_@func@)
 /**begin repeat
  * #func = exp, log#
  * #scalar = npy_exp, npy_log#
+ * #underflow = 1, 0#
  */
 NPY_NO_EXPORT void NPY_CPU_DISPATCH_CURFX(DOUBLE_@func@)
 (char **args, npy_intp const *dimensions, npy_intp const *steps, void *NPY_UNUSED(data))
@@ -1260,6 +1261,13 @@ NPY_NO_EXPORT void NPY_CPU_DISPATCH_CURFX(DOUBLE_@func@)
 #endif
     UNARY_LOOP {
         const npy_double in1 = *(npy_double *)ip1;
+    #if @underflow@
+        if (NPY_UNLIKELY(!npy_isnan(in1) && in1 < -0x1.74910d52d3053p+9)) {
+            *(npy_double *)op1 = 0;
+            npy_set_floatstatus_underflow();
+            continue;
+        }
+    #endif
         *(npy_double *)op1 = @scalar@(in1);
     }
 }

@seiko2plus
Copy link

if the patch passes the test, then we will need to determine which versions of glibc are affected by this bug.

@h-vetinari
Copy link
Member Author

h-vetinari commented May 27, 2021

I'm not sure what exactly you doing here since I see you requesting the compiler to tune the whole lib for Haswell CPUs through env var CFLAGS.

CFLAGS="-mtune=haswell"

I don't know where this is coming from, the build.sh doesn't have any mention of that.

if the patch passes the test, then we will need to determine which versions of glibc are affected by this bug.

Could this be a similar issue like numpy/numpy#15179? This test is currently being skipped here (after I verified it still fails), but does pass with the following patch (suggested by @r-devulap):

diff --git a/numpy/core/tests/test_umath.py b/numpy/core/tests/test_umath.py
index 9d1b13b53..faa2ca8f0 100644
--- a/numpy/core/tests/test_umath.py
+++ b/numpy/core/tests/test_umath.py
@@ -1188,8 +1188,6 @@ def test_sincos_float32(self):
         M = np.int_(N/20)
         index = np.random.randint(low=0, high=N, size=M)
         x_f32 = np.float32(np.random.uniform(low=-100.,high=100.,size=N))
-        # test coverage for elements > 117435.992f for which glibc is used
-        x_f32[index] = np.float32(10E+10*np.random.rand(M))
         x_f64 = np.float64(x_f32)
         assert_array_max_ulp(np.sin(x_f32), np.float32(np.sin(x_f64)), maxulp=2)
         assert_array_max_ulp(np.cos(x_f32), np.float32(np.cos(x_f64)), maxulp=2

The comment also mentions glibc, which is why I'm asking...

@h-vetinari
Copy link
Member Author

@seiko2plus: if the patch passes the test, then we will need to determine which versions of glibc are affected by this bug.

With the patch, it passed.

@h-vetinari
Copy link
Member Author

@seiko2plus

if the patch passes the test, then we will need to determine which versions of glibc are affected by this bug.

Any follow-up you'd like me to do? Open an issue in numpy?

@h-vetinari
Copy link
Member Author

Ping @seiko2plus @r-devulap

For 1.21.0 (once it arrives), should we skip the test? Carry your patch? Something else?

@h-vetinari
Copy link
Member Author

if the patch passes the test, then we will need to determine which versions of glibc are affected by this bug.

Opened an issue: numpy/numpy#19192

@h-vetinari h-vetinari changed the title try building 1.21.0rc1 try building 1.21.0rc's Jun 8, 2021
@h-vetinari
Copy link
Member Author

Note, the last commits were to test whether numpy/numpy#19192 disappears in the face of a newer glibc (2.17 instead of 2.12), and indeed, this is the case.

Interestingly, there seems to be a non-timeout failure on pypy3.7+aarch, not sure what caused it.

@h-vetinari
Copy link
Member Author

@conda-forge/numpy @conda-forge/core

In the upstream issue uncovered by testing the rc, the point came up if this recipe could move to CentOS 7 already. Any thoughts on that?

@h-vetinari: Yeah, this is a similar situation as with multilinux - conda-forge tries to build for the lowest common denominator, which currently is CentOS 6, but there slowly are some packages requiring CentOS 7.
....
However, I expect the bump on conda-forge side to 2.17 to not be merged, because that would force all packages that depend on numpy to also require 2.17.

@rgommers: CentOS 6 is >6 months past EOL. Official EOL really should be the last time to drop a constraint. The patch for working around this problem seems fine to accept, but exercises like these are a massive waste of time. For manylinux we're also fine to move to glibc 2.17 (= manylinux2014), for the same reason. What's the problem here exactly?

@xhochy
Copy link
Member

xhochy commented Jun 9, 2021

I think we should in general move conda-forge to cos7 but here is probably not the right place to discuss this. Probably we already have an issue for that.

@h-vetinari
Copy link
Member Author

I think we should in general move conda-forge to cos7 but here is probably not the right place to discuss this. Probably we already have an issue for that.

Found it: conda-forge/conda-forge.github.io#1436

@h-vetinari h-vetinari changed the title try building 1.21.0rc's remove obsolete test skip Jun 28, 2021
@h-vetinari
Copy link
Member Author

This was mostly superseded by #236, but for reasons of skip-hygiene, we should still remove the skip that has become obsolete in 1.21.0 (because numpy is currently xfailing it itself).

@h-vetinari h-vetinari marked this pull request as ready for review June 28, 2021 11:43
Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

for reasons of skip-hygiene, we should still remove the skip that has become obsolete in 1.21.0 (because numpy is currently xfailing it itself).

Agreed.

All jobs are green except for one timeout, so in it goes. Thanks @h-vetinari

@rgommers rgommers merged commit 7d35403 into conda-forge:master Jun 28, 2021
@h-vetinari h-vetinari deleted the rc branch June 30, 2021 07:24
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.

6 participants