Opened 14 months ago

Last modified 2 days ago

#30350 positive_review enhancement

Remove ATLAS

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: packages: standard Keywords:
Cc: jhpalmieri, dimpase, fbissey, jpflori Merged in:
Authors: John Palmieri Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: u/jhpalmieri/remove-atlas (Commits, GitHub, GitLab) Commit: 272424a33ba1a68b221b160ebbbc9c89852aa464
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

(from #19719)

Our version of ATLAS is outdated and has not been tested in a long time. Latest upstream version as of 2021-09-21 is 3.11.40, released 2018-10-02, see ​https://github.com/math-atlas/math-atlas/releases

We remove this package.

However, we keep the makefile variable BLAS (used in packages' dependencies files), rather than hardcoding openblas in dependencies. It will make it easier to plug in another BLAS implementation if one comes along in the future, and it is also needed for #29387.

Change History (28)

comment:1 Changed 14 months ago by mkoeppe

  • Cc fbissey jpflori added

comment:2 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 8 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/remove-atlas

comment:4 follow-ups: Changed 8 months ago by jhpalmieri

  • Commit set to 3c3022ce28961e3f75bb729525f2b1a05834807f

Here is an attempt. I have questions:

  • what to do with openblas/spkg-configure.m4? Any changes necessary?
  • what about the spkg-install.in scripts for scipy and numpy? They refer to an ATLAS environment variable. Leave as is or change?
  • I don't know anything about cygwin, so I don't know what to do about the installation instructions referring to ATLAS in src/doc/en/installation/source.rst.

Anyway, this is a start.


New commits:

3c3022ctrac 30350: remove the ATLAS package

comment:5 Changed 8 months ago by git

  • Commit changed from 3c3022ce28961e3f75bb729525f2b1a05834807f to 0afdab5ef5a560c75f737ca7274635d268600f89

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0afdab5trac 30350: remove the ATLAS package

comment:6 Changed 8 months ago by mkoeppe

  • Authors set to John Palmieri

comment:7 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

comment:8 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:9 Changed 2 days ago by mkoeppe

I support removing this package in 9.5. The branch needs rebasing

comment:10 Changed 2 days ago by git

  • Commit changed from 0afdab5ef5a560c75f737ca7274635d268600f89 to 8ba1d79f84e6030c01ede985e813e35a3d180feb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8ba1d79trac 30350: remove the ATLAS package

comment:11 Changed 2 days ago by jhpalmieri

Rebased. I deleted the reference to ATLAS in the cygwin instructions in src/doc/en/installation/source.rst, but the other questions in comment:4 remain.

comment:12 in reply to: ↑ 4 Changed 2 days ago by mkoeppe

Replying to jhpalmieri:

  • what to do with openblas/spkg-configure.m4? Any changes necessary?

We could either keep the --with-blas=... option but make it an error if anything other than openblas is passed, or remove it.

comment:13 in reply to: ↑ 4 Changed 2 days ago by mkoeppe

Replying to jhpalmieri:

  • what about the spkg-install.in scripts for scipy and numpy? They refer to an ATLAS environment variable. Leave as is or change?

No change needed. The scripts just set the environment variable to a safe value so the installation does not pick up random things that may be in the user's environment. (Not sure why the default is different for macOS and for other systems.)

comment:14 Changed 2 days ago by mkoeppe

The changes to the documentation look fine.

comment:15 Changed 2 days ago by jhpalmieri

Is this the way to remove the --with-blas option?

  • build/pkgs/openblas/spkg-configure.m4

    diff --git a/build/pkgs/openblas/spkg-configure.m4 b/build/pkgs/openblas/spkg-configure.m4
    index 177bbb1d4f..c81533ba71 100644
    a b SAGE_SPKG_CONFIGURE([openblas], [ 
    110110  LIBS="$SAVE_LIBS"
    111111  CFLAGS="$SAVE_CFLAGS"
    112112 ])
    113  ], [
    114   dnl REQUIRED-CHECK
    115   AS_IF([test "x$with_blas" = xopenblas], [
    116      sage_require_openblas=yes
    117      sage_require_atlas=no])
    118   ], [
    119   dnl PRE
    120   AC_MSG_CHECKING([BLAS library])
    121   AC_ARG_WITH([blas],
    122   [AS_HELP_STRING([--with-blas=openblas],
    123     [use OpenBLAS as BLAS library (default)])]
    124   [AS_HELP_STRING([--with-blas=atlas],
    125     [use ATLAS as BLAS library])],,
    126     [with_blas=openblas]  # default
    127   )
    128   AS_CASE(["$with_blas"],
    129     [openblas], [],
    130     [atlas],    [sage_spkg_install_openblas=no],
    131                 [AC_MSG_ERROR([allowed values for --with-blas are 'atlas' and 'openblas'])])
    132   AC_MSG_RESULT([$with_blas])
    133   AC_SUBST([SAGE_BLAS], [$with_blas])
    134   ]
     113 ]
    135114)

comment:16 Changed 2 days ago by mkoeppe

Yes, that looks right

comment:17 Changed 2 days ago by mkoeppe

After removing, this line:

build/make/Makefile.in:BLAS = @SAGE_BLAS@

needs changing

comment:18 Changed 2 days ago by jhpalmieri

Can I just change it to BLAS = openblas?

comment:19 Changed 2 days ago by mkoeppe

Yes

comment:20 Changed 2 days ago by git

  • Commit changed from 8ba1d79f84e6030c01ede985e813e35a3d180feb to 69db83912a25f1cfa4341df59b2a5a45bb27a3cb

Branch pushed to git repo; I updated commit sha1. New commits:

69db839trac 30350: remove "--with-blas=" option to ./configure

comment:21 Changed 2 days ago by jhpalmieri

Okay, here are those changes.

comment:22 Changed 2 days ago by mkoeppe

build/pkgs/atlas/distros/ still contains some stuff

comment:23 Changed 2 days ago by git

  • Commit changed from 69db83912a25f1cfa4341df59b2a5a45bb27a3cb to 272424a33ba1a68b221b160ebbbc9c89852aa464

Branch pushed to git repo; I updated commit sha1. New commits:

272424atrac 30350: remove remaining bits in build/pkgs/atlas/distros

comment:24 Changed 2 days ago by jhpalmieri

Fixed, thanks.

comment:25 Changed 2 days ago by mkoeppe

  • Description modified (diff)
  • Status changed from new to needs_review

comment:26 Changed 2 days ago by mkoeppe

  • Description modified (diff)

comment:27 Changed 2 days ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

This seems to work well.

comment:28 Changed 2 days ago by jhpalmieri

Thank you!

Note: See TracTickets for help on using tickets.