Opened 4 years ago

Closed 4 years ago

#18777 closed enhancement (fixed)

Remove unneeded BLAS linking for Cython modules

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.8
Component: build Keywords:
Cc: jason, fbissey, jpflori Merged in:
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: be582ec (Commits) Commit: be582ec8355415737606b208b102f895a9cfde57
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Various Cython modules implementing matrices and vectors list

libraries = [BLAS, BLAS2]

in src/module_list.py.

However, nothing in Sage directly uses BLAS, it is only used through external libraries (linbox, numpy, ...). So it makes no sense to link to BLAS and nothing else, which means that this is a mistake.


I checked the commit history and these extensions listing only BLAS, BLAS2 as libraries go back to #3498 and #4206:

commit a8b66cc30d4aca925c1e3a1c0bb69d0f4f62fbb1
Author: Jason Grout <jason-sage@creativetrax.com>
Date:   Wed Jul 9 19:54:35 2008 -0500

    Switch the RDF and CDF matrices to a numpy 1.2 backend; factor out common functionality to matrix_double_dense.pyx.

and

commit b07600d93461c717fc6702e99afd0f2cf2eeb1df
Author: Jason Grout <jason-sage@creativetrax.com>
Date:   Tue Nov 11 03:30:38 2008 -0600

    Change the RDF/CDF vector backend to numpy, and refactor the code to a common parent vector_double_dense.  Also change names of the classes to vector_real_double_dense and vector_complex_double_dense.

It seems that the backends were changed from GSL to Numpy and I guess that the BLAS libraries (needed for GSL) were simply kept.

There might also be the mistaken assumption that BLAS was needed for numpy. It's true that numpy uses BLAS, but we use numpy using the standard Python import mechanism, it's not the usual dynamic linking (there is no -lnumpy argument needed either).

Change History (6)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/remove_unneeded_blas_linking_for_cython_modules

comment:3 Changed 4 years ago by jdemeyer

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

New commits:

be582ecRemove unneeded BLAS linking for Cython modules

comment:4 Changed 4 years ago by fbissey

OK I will check all of these with readelf in sage-on-gentoo. Since It is all compiled with -as-needed we'll definitely see if anything needs cblas directly. If the dependency is indirect will it break on cygwin?

comment:5 Changed 4 years ago by fbissey

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

All these only link to libc and libpython which I could have seen straight away from module_list.py. We definitely want that simplification. I cannot see how we would introduce side effects by removing these linkings.

comment:6 Changed 4 years ago by vbraun

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