upgrade cvxopt to version 1.1.8 #19687

dimpase · 2015-12-09T21:52:21Z

this is the current stable version

The tarball is here: http://users.ox.ac.uk/~coml0531/sage/cvxopt-1.1.8.tar.gz

I removed html docs from the source (https://github.com/cvxopt/cvxopt/archive/1.1.8.tar.gz) to make it smaller.

Upstream: Fixed upstream, but not in a stable release.

CC: @jdemeyer @nathanncohen

Component: packages: standard

Author: Dima Pasechnik

Branch/Commit: 79784f3

Reviewer: François Bissey

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

The text was updated successfully, but these errors were encountered:

dimpase · 2015-12-09T22:24:01Z

Branch: u/dimpase/19687

dimpase · 2015-12-09T22:24:01Z

New commits:

`c040534`	`update to 1.1.8`

dimpase · 2015-12-09T22:24:01Z

Commit: c040534

kiwifb · 2015-12-11T20:38:37Z

Reviewer: François Bissey

kiwifb · 2015-12-11T20:38:37Z

comment:2

Looks good to me. No doctest broken on linux x86_64.

vbraun · 2015-12-21T19:02:05Z

comment:3

On older linux (Ubuntu 12.04) I get

ImportError: /mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/cvxopt/amd.so: undefined symbol: clock_gettime

according to man page, needs to "link with -lrt (only for glibc versions before 2.17)" but does not.

kiwifb · 2015-12-21T19:12:05Z

comment:5

Upstream probably doesn't build on older machines. Should be trivial to fix.

dimpase · 2015-12-21T20:50:56Z

comment:6

Replying to @vbraun:

On older linux (Ubuntu 12.04) I get
ImportError: /mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/cvxopt/amd.so: undefined symbol: clock_gettime
according to man page, needs to "link with -lrt (only for glibc versions before 2.17)" but does not.

It does not seem that testing glibc version tells you the right thing. Indeed, on Ubuntu 14.04, where this all works, I see

>>> import platform
>>> platform.libc_ver() 
('glibc', '2.4')

And I don't even have librt. I will just add it unconditionally, for every Linux...

sagetrac-git · 2015-12-21T20:55:41Z

Changed commit from c040534 to 6390e86

sagetrac-git · 2015-12-21T20:55:41Z

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

`8c1e90d`	`update to 1.1.8`
`6390e86`	`add 'rt' to libs on linux`

dimpase · 2015-12-21T20:57:11Z

Upstream: Not yet reported upstream; Will do shortly.

kiwifb · 2015-12-21T21:02:28Z

comment:9

Wrong solution

if SUITESPARSE_EXT_LIB:
    amd = Extension('amd',
        libraries = ['amd','suitesparseconfig'],
        include_dirs = [SUITESPARSE_INC_DIR],
        library_dirs = [SUITESPARSE_LIB_DIR],
        sources = ['src/C/amd.c'])
else:
    amd = Extension('amd', 
        include_dirs = [ 'src/C/SuiteSparse/AMD/Include', 
            'src/C/SuiteSparse/SuiteSparse_config' ],
        define_macros = MACROS,
        sources = [ 'src/C/amd.c', 'src/C/SuiteSparse/SuiteSparse_config/SuiteSparse_config.c'] +
        glob('src/C/SuiteSparse/AMD/Source/*.c') )

The amd extension doesn't use libraries so I would be surprised if it worked by adding it to BLAS_LIBS.

sagetrac-git · 2015-12-21T21:57:04Z

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

`fdaeefa`	`add libs='rt' to amd ext.`

sagetrac-git · 2015-12-21T21:57:04Z

Changed commit from 6390e86 to fdaeefa

dimpase · 2015-12-21T22:00:54Z

comment:11

right, it should have gone to the amd Extension chunk, sorry.
The patch is longer, as I fixed all the fizz, by cloning https://github.com/cvxopt/cvxopt
and making a new patch using the old one.
Then I just added libraries=['rt'] unconditionally there -- should this be done for Linux only?

kiwifb · 2015-12-21T22:03:36Z

comment:12

No librt on OS X as far as I can tell, I don't know about cygwin. I would restrict it.

kiwifb · 2015-12-21T22:14:11Z

comment:13

clock_gettime is not in the AMD code it is in the SuiteSparse_config I am checking the implications.

sagetrac-git · 2015-12-21T22:23:54Z

Changed commit from fdaeefa to 4b91b98

sagetrac-git · 2015-12-21T22:23:54Z

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

`4b91b98`	`add libs='rt' to amd ext (on linux only)`

dimpase · 2015-12-21T22:33:50Z

comment:15

Replying to @kiwifb:

clock_gettime is not in the AMD code it is in the SuiteSparse_config I am checking the implications.

well, the import error is in amd.so, and src/C/SuiteSparse/SuiteSparse_config/SuiteSparse_config.c
gets complied into the amd extension. So this looks right what I do now...

sagetrac-git · 2015-12-21T22:37:23Z

Changed commit from 4b91b98 to e8c1e0a

sagetrac-git · 2015-12-21T22:37:23Z

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

`e8c1e0a`	`add libs='rt' to amd ext (on linux only)`

kiwifb · 2015-12-21T22:48:18Z

comment:17

Volker reported one failure, there may be other hidden behind the first one. That file is also included in cholmod and umfpack so they are potentially affected too. However the code should only be enabled if SUITESPARSE_TIMER_ENABLED is defined, in turn it depends on whether NTIMER is defined (which it is for cholmod and umfpack so in the end they won't be affected). A better solution in my opinion is to add [('NTIMER', '1')] to the line define_macros for the amd section.

New commits:

`e8c1e0a`	`add libs='rt' to amd ext (on linux only)`

dimpase · 2015-12-21T23:03:14Z

comment:18

adding things like [('NTIMER', '1')] looks like going too far, as you're changing the behaviour of upstream code this way...

kiwifb · 2015-12-21T23:12:33Z

comment:19

I don't think cvxopt cares about timing of the code, in fact SuiteSparse_{tic,toc,time} are used nowhere in AMD's code. It is only enabled as part of SuiteSparse_config.c by accident I would argue. In contrast cholmod and umfpack have code that can use SuiteSparse_time and it is disabled on purpose in those. Those function are only used to measure performance, I don't think cvxopt would be doing it that way, if it was interested in measuring the performance of the SuiteSparse code.

kiwifb · 2015-12-21T23:14:16Z

comment:20

I would further argue that we should point to upstream that unless they want to use those timing functions in the future, NTIMER should be in the default macros variable that is currently empty.

dimpase · 2015-12-21T23:30:02Z

comment:21

I just asked upstream here on this.

dimpase · 2015-12-21T23:30:02Z

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

kiwifb · 2015-12-21T23:36:29Z

comment:22

OK, I'll add an argument:
SuiteSparse_config.c is not part of amd's sources. If you were using an external suitesparse install, you would have a suitesparse_config library and amd would get the functions from this library as needed. But here we have a custom system that makes SuiteSparse_config.c a part of the used amd source code and including all the function of SuiteSparse_config.c instead of just the one needed by amd. That include the timing functions.

sagetrac-git · 2015-12-22T19:38:37Z

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

`79784f3`	`fix building amd ext`

sagetrac-git · 2015-12-22T19:38:37Z

Changed commit from e8c1e0a to 79784f3

dimpase · 2015-12-22T19:39:22Z

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

dimpase · 2015-12-22T19:39:22Z

comment:24

indeed, that was the right suggestion for the fix. The upstream fixed this in dev here 3 weeks ago...

please see the latest commit update.

New commits:

`79784f3`	`fix building amd ext`

kiwifb · 2015-12-22T19:41:50Z

comment:25

I am happy with that resolution. Back to positive review.

vbraun · 2015-12-23T11:06:35Z

Changed branch from u/dimpase/19687 to 79784f3

dimpase added this to the sage-6.10 milestone Dec 9, 2015

dimpase added c: packages: standard labels Dec 9, 2015

This comment has been minimized.

Sign in to view

dimpase added the s: needs review label Dec 9, 2015

kiwifb added s: positive review and removed s: needs review labels Dec 11, 2015

vbraun added s: needs work and removed s: positive review labels Dec 21, 2015

dimpase modified the milestones: sage-6.10, sage-7.0 Dec 21, 2015

dimpase added s: needs review and removed s: needs work labels Dec 21, 2015

kiwifb added s: positive review and removed s: needs review labels Dec 22, 2015

vbraun removed the s: positive review label Dec 23, 2015

vbraun closed this as completed in 31096e1 Dec 23, 2015

dimpase mentioned this issue Dec 2, 2019

Upgrade to cvxopt 1.2.x #22380

Closed

upgrade cvxopt to version 1.1.8 #19687

upgrade cvxopt to version 1.1.8 #19687

Comments

dimpase commented Dec 9, 2015

This comment has been minimized.

dimpase commented Dec 9, 2015

dimpase commented Dec 9, 2015

dimpase commented Dec 9, 2015

kiwifb commented Dec 11, 2015

kiwifb commented Dec 11, 2015

vbraun commented Dec 21, 2015

kiwifb commented Dec 21, 2015

dimpase commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

dimpase commented Dec 21, 2015

kiwifb commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

dimpase commented Dec 21, 2015

kiwifb commented Dec 21, 2015

kiwifb commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

dimpase commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

sagetrac-git mannequin commented Dec 21, 2015

kiwifb commented Dec 21, 2015

dimpase commented Dec 21, 2015

kiwifb commented Dec 21, 2015

kiwifb commented Dec 21, 2015

dimpase commented Dec 21, 2015

dimpase commented Dec 21, 2015

kiwifb commented Dec 21, 2015

sagetrac-git mannequin commented Dec 22, 2015

sagetrac-git mannequin commented Dec 22, 2015

dimpase commented Dec 22, 2015

dimpase commented Dec 22, 2015

kiwifb commented Dec 22, 2015

vbraun commented Dec 23, 2015