Wrong result from np.linalg.cholesky on 32-bit win32 in numpy 1.13.3 #9795

pv · 2017-09-30T14:00:07Z

The following program produces wrong results for Numpy 1.13.3 for the 32-bit win32 wheel numpy-1.13.3-cp36-none-win32.whl:

import numpy as np
m = np.array([[1, 0.3], [0.3, 1]])
c = np.linalg.cholesky(m)
print(np.__version__, "\n", c)
assert np.allclose(c @ c.T, m)

it prints

1.13.3 
 [[ -2.00000000e+00   0.00000000e+00]
 [ -1.50000000e-01  -9.55902467e-54]]
Traceback (most recent call last):
  File "test.py", line 5, in <module>
    assert np.allclose(c @ c.T, m)
AssertionError

The 64-bit windows wheels appear to be OK.

The result for Numpy 1.13.1 (numpy-1.13.1-cp36-none-win32.whl) is also OK:

1.13.1 
 [[ 1.         0.       ]
 [ 0.3        0.9539392]]

The text was updated successfully, but these errors were encountered:

pv · 2017-09-30T14:03:28Z

I can't think of other causes than a broken LAPACK/BLAS linalg library. Was the build changed between 1.13.1 and 1.13.3 (it appears yes, atlas was swapped for openblas)? Also, doesn't numpy have tests for cholesky that would have caught this?

pv · 2017-09-30T14:59:01Z

This issue probably can be handled just with the wheel build tags...

matthew-brett · 2017-09-30T15:44:30Z

No better using mingwpy build of later OpenBLAS commit : https://ci.appveyor.com/project/matthew-brett/numpy-wheels/build/job/uf1r7hsa6rmxe962

This is OpenBLAS commit 5f998ef

How easy would it be to make a minimal test case for the OpenBLAS guys?

pv · 2017-09-30T16:55:21Z

Fairly easy, OpenMathLib/OpenBLAS#1318

charris · 2017-09-30T16:58:56Z

Doesn't look to be an easy way to choose between ATLAS and OpenBLAS for building Windows wheels. The quick fix would be to use ATLAS for the four 32 bit windows wheels, then use OpenBLAS again when it is fixed.

charris · 2017-09-30T17:39:35Z

So, just to clarify the procedure for bad wheels, as opposed to bad source,

Remove the bad wheels from PyPI, should fall back to last good release.
Upload good wheels, when available, using a build tag.

@rgommers Thoughts? The wheel names are already so arcane that I think adding a build tag is not much of a problem.

See: numpy/numpy#9795 (comment)

matthew-brett · 2017-09-30T18:48:29Z

@charris - I've built the 32-bit ATLAS wheels : https://ci.appveyor.com/project/matthew-brett/numpy-wheels/build/1.0.75 - just a hack for now, using an older numpy-wheels commit. I've downloaded them, got the build tags done, can upload on your say-so.

charris · 2017-09-30T18:57:11Z

@matthew-brett Let's give it a shot. I'll download them once they are up in order to redo the hashes for Github.

charris · 2017-09-30T19:00:39Z

Oh, and which cythonized version did you use? Just want to make sure it was 0.26.1.

matthew-brett · 2017-09-30T19:03:07Z

Oops - no - it was Cython 0.27. Silly me. Build tag 2 I guess.

matthew-brett · 2017-09-30T19:31:08Z

Done rebuild with Cython 0.26.1 : https://ci.appveyor.com/project/matthew-brett/numpy-wheels/build/1.0.76

Checked they are using ATLAS, and are not the same as previous wheels built with Cython 0.27.

Added build tag. Ready to upload. This will be build tag 2...

charris · 2017-09-30T20:02:33Z

Sounds good.

matthew-brett · 2017-09-30T20:05:37Z

Done.

charris · 2017-09-30T20:16:16Z

Great, thanks. I'll leave the other files in for a bit to see if PyPI works correctly with them there.

pv · 2017-09-30T20:46:23Z

pip install numpy still gives the broken wheel (with no build tags).
Maybe pip just doesn't actually have support for build tags?

pv · 2017-09-30T20:58:21Z

Ok, seems to work now (after charris deleted the old wheels --- or due to something else).

rgommers · 2017-09-30T20:59:08Z

Maybe pip just doesn't actually have support for build tags?

My internet connection is too slow to search for it now, but I'd like to point out again that we tried using .post0 build tags before, they didn't work as expected, and we decided to not do that again.

So, just to clarify the procedure for bad wheels, as opposed to bad source,

Remove the bad wheels from PyPI, should fall back to last good release.
Upload good wheels, when available, using a build tag.

@rgommers Thoughts? The wheel names are already so arcane that I think adding a build tag is not much of a problem.

Unless we experiment with this thoroughly (not in the main numpy account) and prove that everything works as expected, I'd say that either broken wheels or broken source means we pull the whole release if the breakage is bad enough to warrant that.

pv · 2017-09-30T21:02:27Z

If we want openblas for the windows numpy wheels, gh-9645 may be a better way to go, because that relies on vanilla mingw-w64 rather than on mingwpy.

charris · 2017-09-30T21:09:45Z

@rgommers, If this works I'm inclined to leave it be. That said, IIRC, the reason for not allowing reuse of file names was that synchronizing different servers could take time. I don't know that build tags solve that problem. I think we need more feedback from the PyPA people.

pv · 2017-09-30T22:36:15Z

Before closing this, consider fixing the numpy-wheels repo configuration.

ghost · 2017-09-30T23:03:57Z

I thought we already had found that mingwpy caused problems. I would recommend using the same procedure in the source repository as the release repository.

matthew-brett · 2017-09-30T23:11:49Z

@xoviat - yes - that's a good idea. Do you have any interest in porting your stuff over to the numpy-wheels repo?

ghost · 2017-09-30T23:14:03Z

Yes but I think the pull request on this repository needs to be merged first (#9645).

matthew-brett · 2017-09-30T23:14:48Z

For the build tags - I'm pretty confident that they work - I've used them before a few times, and we're testing it again now, with this fix. I have always deleted the previous versions after uploading the new build tag, but I suspect that's not necessary.

charris · 2017-10-01T00:03:38Z

@xoviat #9645 is in.

ghost · 2017-10-01T00:08:45Z

I'll look at the wheels repository sometime soon.

ghost · 2017-10-01T01:49:19Z

@matthew-brett See MacPython/numpy-wheels#3.

rgommers · 2017-10-01T19:29:49Z

@rgommers, If this works I'm inclined to leave it be. That said, IIRC, the reason for not allowing reuse of file names was that synchronizing different servers could take time. I don't know that build tags solve that problem. I think we need more feedback from the PyPA people.

Now that I've looked at it in a little more detail: what you currently did should be robust. It's not a build tag like .post0, which is a version modifier (that is the thing that should be avoided). By deleting the old wheel and putting in a new one with an extra tag -1 in this case, it should still find this wheel for the relevant OS/Python combo. It could be any tag, doesn't have to be -1, -2 etc. The only requirement is that it's a valid tag format.

matthew-brett · 2017-10-01T19:39:39Z

I believe the build tag has to start with a digit - see: https://github.com/pypa/pip/blob/master/src/pip/_internal/wheel.py#L488

rgommers · 2017-10-01T19:46:01Z

@matthew-brett you're right. And it's not just an implementation detail, there's an actual spec: https://www.python.org/dev/peps/pep-0491/#file-name-convention

I thought there were also arbitrary tags, but doesn't look like that's right. The .post0 business has been removed.

njsmith · 2017-10-01T20:34:10Z

.post0 is still a thing; in the file name layout in PEP 491 that you linked, .post0 would be part of the version. The .post nonsense is described in PEP 440 (though even it suggests that you avoid using it).

The key distinction here is that .post0 is part of the actual version number, so as far as the tools are concerned 1.13.1.post0 is a whole new release that comes after 1.13.1. It's basically a more confusing way of spelling 1.13.1.1. OTOH a wheel build tag indicates a new build of the same release.

charris · 2017-10-01T20:57:11Z

@njsmith Would be good if build tags could also be used with sources, although the naming scheme might be too flexible for that. I suspect the possiblity of different source builds was overlooked.

njsmith · 2017-10-01T21:05:52Z

@charris I believe (for better or for worse) the concept is exactly that a given release has 1 source and possibly lots of builds, and if you want to roll a new sdist then you should bump the actual version. I guess this makes some sense; if you have multiple source builds then how do you know which ones were used for which binary builds?

ghost · 2017-10-01T21:36:59Z

I think the general idea is that a source distribution is as close to a source checkout as possible (specifically, there should be no cythonized files in the source distribution). If that's not the case, then pypi won't play well with what you're trying to do.

njsmith · 2017-10-01T22:22:50Z

Cythonized files in the source distribution work fine, but yeah, if you want to regenerate the cython files it means you have to bump the version number.

charris · 2017-12-17T04:57:10Z

I'm going to close this. The NumPy problem seems to have been solved by moving to vanilla mingw-w64.

pv mentioned this issue Sep 30, 2017

Test failures on 32-bit win32 with Numpy 1.13.3 scipy/scipy#7955

Closed

This was referenced Sep 30, 2017

ENH: Basin-hopping changes scipy/scipy#7954

Merged

TST: linalg: add basic smoketest for cholesky #9796

Merged

matthew-brett added a commit to MacPython/numpy-wheels that referenced this issue Sep 30, 2017

Build 32-bit wheels with ATLAS

65d5b94

See: numpy/numpy#9795 (comment)

charris mentioned this issue Nov 26, 2017

TST: linalg: add basic smoketest for cholesky #10085

Merged

charris closed this as completed Dec 17, 2017

Uh oh!

Wrong result from np.linalg.cholesky on 32-bit win32 in numpy 1.13.3 #9795

Wrong result from np.linalg.cholesky on 32-bit win32 in numpy 1.13.3 #9795

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!