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

numpy incompatibility when using system python and numpy 1.22 is installed in the system #33473

Closed
tornaria opened this issue Mar 7, 2022 · 29 comments

Comments

@tornaria
Copy link
Contributor

tornaria commented Mar 7, 2022

This retroactively affects sage-9.5, meaning sage-9.5 used to build ok in my system but now that system numpy has been upgraded to 1.22 it breaks. This is using the standard tarball, unpatched, as long as system python is used and numpy 1.22 happens to be installed in the system when sage is build.

NOTE: this is NOT trying to use system numpy. Sage builds and installs its own numpy (1.21) but headers for the system numpy (1.22) get incorrectly mixed up when building sagelib.

The symptom is that any sage module that uses cimport numpy will fail to load, making sage almost unusable, e.g.:

sage: import sage.rings.polynomial.real_roots
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: numpy.ndarray size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

Background:

  • numpy 1.21 and numpy 1.22 are ABI incompatible (ndarray changed size)
  • building sagelib needs to know location of include files:
    • for python: since I'm using system python, this is a system include dir, here /usr/include/python3.10
    • for numpy: since numpy is bundled with sage, this is a dir in the venv, here /home/tornaria/src/sage/sage-9.5/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/numpy/core/include
  • my system numpy (1.22) has its own headers installed in /usr/lib/python3.10/site-packages/numpy/core/include but there's also a symlink /usr/include/python3.10/numpy -> ../../lib/python3.10/site-packages/numpy/core/include/numpy.
  • in the CC command line the python include location (in the system) is before the numpy include location (in the venv)
  • since the system python contains the system numpy, all numpy headers will be included from system and not from the venv, triggering the incompatibility.

NOTE: the symlink /usr/include/python3.10/numpy is not shipped by numpy itself, but it seems some distros add it. I'm using void linux, which has it, but at least debian also has it (see https://packages.debian.org/bullseye/amd64/python3-numpy/filelist)

The solution should be changing the order of the include locations so that the numpy location comes before the python location.

CC: @vbraun

Component: build

Author: Gonzalo Tornaría

Branch: f50e4cb

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33473

@tornaria tornaria added this to the sage-9.6 milestone Mar 7, 2022
@tornaria
Copy link
Contributor Author

tornaria commented Mar 7, 2022

comment:1

It seems scipy may also be affected by this:

$ grep -l 'usr.*numpy.*\.h' logs/pkgs/*
logs/pkgs/sagelib-9.6.beta3.log
logs/pkgs/scipy-1.7.3.log

Indeed

$ grep 'usr.*numpy.*\.h' logs/pkgs/scipy-1.7.3.log 
  In file included from /usr/include/python3.10/numpy/ndarraytypes.h:1960,
                   from /usr/include/python3.10/numpy/ndarrayobject.h:12,
  /usr/include/python3.10/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]
  In file included from /usr/include/python3.10/numpy/ndarraytypes.h:1960,
                   from /usr/include/python3.10/numpy/ndarrayobject.h:12,
                   from /usr/include/python3.10/numpy/arrayobject.h:5,
  /usr/include/python3.10/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]

we see that scipy also uses some incorrect headers (the warning is unrelated).

@tornaria
Copy link
Contributor Author

tornaria commented Mar 7, 2022

comment:2

For sagelib, the includes are determined by

sage: sage.env.sage_include_directories()
['/home/tornaria/src/sage/sage-9.6.beta3/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages',
 '/usr/include/python3.10',
 '/home/tornaria/src/sage/sage-9.6.beta3/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/numpy/core/include']

I think modifying this function so that the numpy include location comes before the system python include location should fix the issue.

As for scipy, it seems the issue happens in module scipy.spatial as in:

sage: import scipy.spatial
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: numpy.ndarray size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

comment:3

What does python3.10 -m sysconfig | grep 'C.*FLAGS' show?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

comment:4

Replying to @tornaria:

For sagelib, the includes are determined by

sage: sage.env.sage_include_directories()
['/home/tornaria/src/sage/sage-9.6.beta3/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages',
 '/usr/include/python3.10',
 '/home/tornaria/src/sage/sage-9.6.beta3/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/numpy/core/include']

I think modifying this function so that the numpy include location comes before the system python include location should fix the issue.

Yes, this sounds like a good fix

@tornaria
Copy link
Contributor Author

tornaria commented Mar 7, 2022

comment:5

Here's an attempt to a solution. I based it on 9.5 and I wonder if this justifies an eventual 9.5.1.


New commits:

f50e4cbnumpy: prefer venv headers before system headers

@tornaria
Copy link
Contributor Author

tornaria commented Mar 7, 2022

Commit: f50e4cb

@tornaria
Copy link
Contributor Author

tornaria commented Mar 7, 2022

Branch: u/tornaria/33473-numpy-includes

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

Author: Gonzalo Tornaría

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

comment:10

I've updated the description - Sage uses numpy 1.21, not 1.19

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

Replying to @tornaria:

NOTE: the symlink /usr/include/python3.10/numpy is not shipped by numpy itself, but it seems some distros add it. I'm using void linux, which has it, but at least debian also has it (see https://packages.debian.org/bullseye/amd64/python3-numpy/filelist)

Arguably, this is a distribution bug. But we definitely have to work around it, given that it is widespread in Debian. Right now it is not acute because no released version of Debian/Ubuntu ships numpy 1.22.x. But it will become a bigger problem when we upgrade to numpy 1.22 and then we face the issue on all versions of Debian/Ubuntu.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2022

comment:13

Replying to @tornaria:

Here's an attempt to a solution. I based it on 9.5 and I wonder if this justifies an eventual 9.5.1.

I don't think anyone is in the mood for making bugfix releases. But it looks like we might be ready for 9.6

@tornaria
Copy link
Contributor Author

tornaria commented Mar 9, 2022

comment:15

Thanks.

I've reported this as a possible bug in the numpy package in void linux (void-linux/void-packages#36062), to try to get an opinion on the distro side.

We could try to get the scipy patch upstreamed.

BTW, I think eventually distutils.sysconfig.get_python_inc() should be changed to sysconfig.get_config_var("INCLUDEPY") since distutils is deprecated but sysconfig doesn't have a function get_python_inc().

@tornaria

This comment has been minimized.

@tornaria

This comment has been minimized.

@tornaria
Copy link
Contributor Author

tornaria commented Mar 9, 2022

comment:16

I reverted the description by accident. Indeed sage 9.5 uses numpy 1.21 and the binary incompatibility arises in the upgrade to 1.22.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 9, 2022

comment:17

Replying to @tornaria:

We could try to get the scipy patch upstreamed.

+1

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 9, 2022

comment:18

Replying to @tornaria:

BTW, I think eventually distutils.sysconfig.get_python_inc() should be changed to sysconfig.get_config_var("INCLUDEPY") since distutils is deprecated but sysconfig doesn't have a function get_python_inc().

Is it even needed or does setuptools already put this include on the command line?

@tornaria
Copy link
Contributor Author

tornaria commented Mar 9, 2022

comment:19

Replying to @mkoeppe:

Replying to @tornaria:

BTW, I think eventually distutils.sysconfig.get_python_inc() should be changed to sysconfig.get_config_var("INCLUDEPY") since distutils is deprecated but sysconfig doesn't have a function get_python_inc().

Is it even needed or does setuptools already put this include on the command line?

No idea. If so, then why is sage.env.sage_include_directories() using distutils.sysconfig.get_python_inc at all?

Can we just

--- a/src/sage/env.py
+++ b/src/sage/env.py
@@ -377,8 +377,7 @@ def sage_include_directories(use_sources=False):
 
     TOP = SAGE_SRC if use_sources else SAGE_LIB
 
-    dirs = [TOP,
-            distutils.sysconfig.get_python_inc()]
+    dirs = [TOP]
     try:
         import numpy
         dirs.insert(1, numpy.get_include())

?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 9, 2022

comment:20

worth a try

@vbraun
Copy link
Member

vbraun commented Mar 9, 2022

Changed branch from u/tornaria/33473-numpy-includes to f50e4cb

@tornaria
Copy link
Contributor Author

Changed commit from f50e4cb to none

@tornaria
Copy link
Contributor Author

comment:22

For the record:

        # Put the Python "system" include dir at the end, so that
        # any local include dirs take precedence.

@tobiasdiez
Copy link
Contributor

comment:23

This now fails on the github action:

sage -t --random-seed=174296915894383156674390339433925467897 sage/env.py
**********************************************************************
File "sage/env.py", line 359, in sage.env.sage_include_directories
Failed example:
    sage.env.sage_include_directories()
Expected:
    ['.../site-packages',
     '.../site-packages/numpy/core/include',
     '.../include/python...']
Got:
    ['/__w/sagetrac-mirror/sagetrac-mirror/src',
     '/sage/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/numpy/core/include',
     '/usr/include/python3.8']

See https://github.com/sagemath/sage/runs/5522519846

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 22, 2022

comment:24

Already fixed in #33532

@tobiasdiez
Copy link
Contributor

comment:25

Replying to @mkoeppe:

Already fixed in #33532

Ah good!

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 2, 2022

comment:26

Replying to @tornaria:

Can we just

--- a/src/sage/env.py
+++ b/src/sage/env.py
@@ -377,8 +377,7 @@ def sage_include_directories(use_sources=False):
 
     TOP = SAGE_SRC if use_sources else SAGE_LIB
 
-    dirs = [TOP,
-            distutils.sysconfig.get_python_inc()]
+    dirs = [TOP]
     try:
         import numpy
         dirs.insert(1, numpy.get_include())

?

Let's do this in #33137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants