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

Remove ATLAS #30350

Closed
mkoeppe opened this issue Aug 13, 2020 · 32 comments
Closed

Remove ATLAS #30350

mkoeppe opened this issue Aug 13, 2020 · 32 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 13, 2020

(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.

CC: @jhpalmieri @dimpase @kiwifb @jpflori

Component: packages: standard

Author: John Palmieri

Branch/Commit: 272424a

Reviewer: Matthias Koeppe

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Aug 13, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@jhpalmieri
Copy link
Member

Branch: u/jhpalmieri/remove-atlas

@jhpalmieri
Copy link
Member

comment:4

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

@jhpalmieri
Copy link
Member

Commit: 3c3022c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 30, 2021

Changed commit from 3c3022c to 0afdab5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 30, 2021

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

0afdab5trac 30350: remove the ATLAS package

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 1, 2021

Author: John Palmieri

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 24, 2021

comment:7

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.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 24, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 21, 2021

comment:9

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2021

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

8ba1d79trac 30350: remove the ATLAS package

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2021

Changed commit from 0afdab5 to 8ba1d79

@jhpalmieri
Copy link
Member

comment:11

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:12

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:13

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.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:14

The changes to the documentation look fine.

@jhpalmieri
Copy link
Member

comment:15

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

diff --git a/build/pkgs/openblas/spkg-configure.m4 b/build/pkgs/openblas/spkg-configure.m4
index 177bbb1d4f..c81533ba71 100644
--- a/build/pkgs/openblas/spkg-configure.m4
+++ b/build/pkgs/openblas/spkg-configure.m4
@@ -110,26 +110,5 @@ SAGE_SPKG_CONFIGURE([openblas], [
   LIBS="$SAVE_LIBS"
   CFLAGS="$SAVE_CFLAGS"
  ])
- ], [
-  dnl REQUIRED-CHECK
-  AS_IF([test "x$with_blas" = xopenblas], [
-     sage_require_openblas=yes
-     sage_require_atlas=no])
-  ], [
-  dnl PRE
-  AC_MSG_CHECKING([BLAS library])
-  AC_ARG_WITH([blas],
-  [AS_HELP_STRING([--with-blas=openblas],
-    [use OpenBLAS as BLAS library (default)])]
-  [AS_HELP_STRING([--with-blas=atlas],
-    [use ATLAS as BLAS library])],,
-    [with_blas=openblas]  # default
-  )
-  AS_CASE(["$with_blas"],
-    [openblas], [],
-    [atlas],    [sage_spkg_install_openblas=no],
-                [AC_MSG_ERROR([allowed values for --with-blas are 'atlas' and 'openblas'])])
-  AC_MSG_RESULT([$with_blas])
-  AC_SUBST([SAGE_BLAS], [$with_blas])
-  ]
+ ]
 )

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:16

Yes, that looks right

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:17

After removing, this line:

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

needs changing

@jhpalmieri
Copy link
Member

comment:18

Can I just change it to BLAS = openblas?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:19

Yes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2021

Changed commit from 8ba1d79 to 69db839

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2021

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

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

@jhpalmieri
Copy link
Member

comment:21

Okay, here are those changes.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:22

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2021

Changed commit from 69db839 to 272424a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2021

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

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

@jhpalmieri
Copy link
Member

comment:24

Fixed, thanks.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2021

comment:27

This seems to work well.

@jhpalmieri
Copy link
Member

comment:28

Thank you!

@vbraun
Copy link
Member

vbraun commented Oct 9, 2021

Changed branch from u/jhpalmieri/remove-atlas to 272424a

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

3 participants