Opened 5 years ago

Closed 5 years ago

#18142 closed enhancement (fixed)

Numpy: fix dependency checking of headers for Sage library code

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.6
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: d9bbc1d (Commits) Commit: d9bbc1d673590fcd18db88120ac95c68a25f281a
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

  1. In src/setup.py, use numpy.get_include() to determine the location of the numpy includes.
  2. When installing numpy, touch the includes to fix dependency checking.
  3. Remove all the manual numpy header and dependency stuff from src/module_list.py.

The removal of the manual numpy dependencies only removed the numpy dependency of src/sage/finance/fractal.pyx (which doesn't use numpy, so the change is correct).

Change History (18)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:3 follow-up: Changed 5 years ago by fbissey

It always felt strange that there isn't a way to get to the numpy include location from numpy itself. Is it just because we are ignorant? If not it seems to be a case of something missing to me.

Of course with sage you can afford the short-cut to local/include I cannot when I can numpy installed for various versions of python. I'd like to see if numpy provides a mechanism first.

comment:5 in reply to: ↑ 3 Changed 5 years ago by jdemeyer

Replying to fbissey:

Of course with sage you can afford the short-cut to local/include I cannot when I can numpy installed for various versions of python. I'd like to see if numpy provides a mechanism first.

François, I don't quite understand what you're trying to say. Are you saying that it's a bad idea to symlink numpy's headers in $SAGE_LOCAL/include/numpy?

comment:6 follow-up: Changed 5 years ago by fbissey

Well for sage alone you don't really care. But from my sage-on-distro point of view I can have numpy for python 2.7.9 and python 3.4.3 installed. And I will have to choose which one to link under /usr/include. But I think the annoyance we have in setup.py (and currently module_list.py), is because we never bothered to look if numpy itself could just tell us where its headers are. And as a matter of fact it can.

comment:7 in reply to: ↑ 6 Changed 5 years ago by jdemeyer

Replying to fbissey:

Well for sage alone you don't really care. But from my sage-on-distro point of view I can have numpy for python 2.7.9 and python 3.4.3 installed.

Does the Python version even matter for the C includes of numpy?

comment:8 follow-up: Changed 5 years ago by jdemeyer

If you prefer numpy.get_include(), that could be arranged. But I still find it strange that every single package installs its includes in $SAGE_LOCAL/includes except for numpy.

comment:9 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Numpy: install includes in $SAGE_LOCAL/include to Numpy: fix dependency checking of headers

comment:10 in reply to: ↑ 8 Changed 5 years ago by fbissey

Replying to jdemeyer:

If you prefer numpy.get_include(), that could be arranged. But I still find it strange that every single package installs its includes in $SAGE_LOCAL/includes except for numpy.

The only python packages that I know have headers and install under include are

  • python itself under python$version
  • pynac (python 2.7 only)
  • polybori (python 2.7 only)

Apart from numpy and cython I cannot think of any other python packages that install headers and those two definitely don't do it under $prefix/include. Because some of the headers in these can depend on the version of python I don't think it will happen anytime soon.

comment:11 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/18142

comment:12 Changed 5 years ago by jdemeyer

  • Commit set to d9bbc1d673590fcd18db88120ac95c68a25f281a
  • Status changed from new to needs_review

New commits:

d9bbc1dSimplify building Cython code which uses numpy

comment:13 follow-up: Changed 5 years ago by fbissey

After inspection of my system I got one header that is shared between version of python under /usr/include: pygobject/pygobject.h. There is only one switch inside that depends on the version of python used if I read the thing correctly. So it can be done.

Anyway the patch looks very good to me and it get the right head counts of numpy_depends (26) in module_list.py.

The only thing you forgot is that bit of code in sage/misc/cython.py

include_dirs = [os.path.join(SAGE_LOCAL,'include','csage'),
                os.path.join(SAGE_LOCAL,'include'), \
                os.path.join(SAGE_LOCAL,'include','python'+platform.python_version().rsplit('.', 1)[0]), \
                os.path.join(SAGE_LOCAL,'lib','python','site-packages','numpy','core','include'), \
                os.path.join(SAGE_SRC,'sage','ext'), \
                os.path.join(SAGE_SRC), \
                os.path.join(SAGE_SRC,'sage','gsl')]

comment:14 in reply to: ↑ 13 Changed 5 years ago by jdemeyer

Replying to fbissey:

The only thing you forgot is that bit of code in sage/misc/cython.py

Please no! That's outside the scope of this ticket. Fixing the big mess which is sage/misc/cython.py is for later.

comment:15 Changed 5 years ago by jdemeyer

  • Summary changed from Numpy: fix dependency checking of headers to Numpy: fix dependency checking of headers for Sage library code

comment:16 follow-up: Changed 5 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Fair enough cython.py deserve its own ticket.

comment:17 in reply to: ↑ 16 Changed 5 years ago by jdemeyer

Replying to fbissey:

Fair enough cython.py deserve its own ticket.

But later. There are way too many Cython-related tickets that it's becoming hard to do anything which doesn't conflict with another ticket.

comment:18 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/18142 to d9bbc1d673590fcd18db88120ac95c68a25f281a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.