Opened 6 years ago

Closed 6 years ago

#19687 closed enhancement (fixed)

upgrade cvxopt to version 1.1.8

Reported by: dimpase Owned by:
Priority: major Milestone: sage-7.0
Component: packages: standard Keywords:
Cc: jdemeyer, ncohen Merged in:
Authors: Dima Pasechnik Reviewers: François Bissey
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 79784f3 (Commits, GitHub, GitLab) Commit: 79784f35dbb0752f160492dd574813e7e527f155
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

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.

Change History (26)

comment:1 Changed 6 years ago by dimpase

  • Branch set to u/dimpase/19687
  • Cc jdemeyer ncohen added
  • Commit set to c0405346c6d4d57f0c57e9822f20198fc2ccbfc5
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

c040534update to 1.1.8

comment:2 Changed 6 years ago by fbissey

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

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

comment:3 follow-up: Changed 6 years ago by 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.

comment:4 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:5 Changed 6 years ago by fbissey

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

comment:6 in reply to: ↑ 3 Changed 6 years ago by dimpase

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

comment:7 Changed 6 years ago by git

  • Commit changed from c0405346c6d4d57f0c57e9822f20198fc2ccbfc5 to 6390e86c33b9fdd7c47ee15592277ac7432926bb

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

8c1e90dupdate to 1.1.8
6390e86add 'rt' to libs on linux

comment:8 Changed 6 years ago by dimpase

  • Milestone changed from sage-6.10 to sage-7.0
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
  • Status changed from needs_work to needs_review

comment:9 Changed 6 years ago by fbissey

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.

comment:10 Changed 6 years ago by git

  • Commit changed from 6390e86c33b9fdd7c47ee15592277ac7432926bb to fdaeefa041f7c55ab914afc13ce9dcbe2859c2b2

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

fdaeefaadd libs='rt' to amd ext.

comment:11 Changed 6 years ago by dimpase

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?

comment:12 Changed 6 years ago by fbissey

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

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

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

comment:14 Changed 6 years ago by git

  • Commit changed from fdaeefa041f7c55ab914afc13ce9dcbe2859c2b2 to 4b91b98e94b44d0559c97500eff70747cdf15271

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

4b91b98add libs='rt' to amd ext (on linux only)

comment:15 in reply to: ↑ 13 Changed 6 years ago by dimpase

Replying to fbissey:

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

comment:16 Changed 6 years ago by git

  • Commit changed from 4b91b98e94b44d0559c97500eff70747cdf15271 to e8c1e0a82f06364975b9b9f91c69de2443c9ad10

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

e8c1e0aadd libs='rt' to amd ext (on linux only)

comment:17 Changed 6 years ago by fbissey

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:

e8c1e0aadd libs='rt' to amd ext (on linux only)

comment:18 Changed 6 years ago by dimpase

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

comment:19 Changed 6 years ago by fbissey

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.

comment:20 Changed 6 years ago by fbissey

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.

comment:21 Changed 6 years ago by dimpase

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

I just asked upstream here on this.

comment:22 Changed 6 years ago by fbissey

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.

Last edited 6 years ago by fbissey (previous) (diff)

comment:23 Changed 6 years ago by git

  • Commit changed from e8c1e0a82f06364975b9b9f91c69de2443c9ad10 to 79784f35dbb0752f160492dd574813e7e527f155

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

79784f3fix building amd ext

comment:24 Changed 6 years ago by dimpase

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

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:

79784f3fix building amd ext

comment:25 Changed 6 years ago by fbissey

  • Status changed from needs_review to positive_review

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

comment:26 Changed 6 years ago by vbraun

  • Branch changed from u/dimpase/19687 to 79784f35dbb0752f160492dd574813e7e527f155
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.