Only warn in case of CHOLMOD major version mismatch #13000

nalimilan · 2015-09-07T16:07:48Z

The check introduced in #10362 is actually too strict,
as ABI break only happen with mismatches in the major
version. Warning on minor version differences means
distributions cannot update SuiteSparse without
rebuiling Julia (#12841).

Also fix the incorrect wording introuced in 25eb444:
this message corresponds to the case where the version is
different, not necessarily older.

CC: @andreasnoack

nalimilan · 2015-09-07T16:10:28Z

base/sparse/cholmod.jl

BTW, I don't understand why this line is here. version_array is declared const above, and already set during compilation using the very same call. Since it's supposed to hold the version used when building Julia, resetting it on init doesn't make any sense to me. Should I remove it?

version_array ends up with the linked version of CHOLMOD if possible and tmp has the version at build time. Therefore, I think we could just have

tmp = copy(version_array)

instead of

tmp = Array(Cint, 3) ccall((:jl_cholmod_version, :libsuitesparse_wrapper), Cint, (Ptr{Cint},), tmp)

but I think there is a problem with version which should be updated at run time.

I think we need these definitions at compile time, but I'm not completely sure, so it might be that they can just be defined in the __init__ function.

nalimilan · 2015-09-08T07:58:23Z

(Moving from adding comments inline as they get hidden after a new commit is added.)

More precisions. I was actually able to get the tests to pass with 4.1.0 (it was related to a build issue). But with 4.0.0 they don't, and I get this error, which seems more serious:

     * sparse               ERROR: LoadError: LoadError("/home/milan/Dev/julia/usr/share/julia/test/sparse.jl",6,LoadError("/home/milan/Dev/julia/usr/share/julia/test/sparsedir/cholmod.jl",106,ErrorException("test failed: 1 == 0\n in expression: unsafe_load(chma.p).is_ll == 0")))

So we should rely on either 4.1.0 (libcholmod 2.1.0) or 4.2.0 (libcholmod 2.1.1) as a minimum. Since only the latter allows getting the version number, I'm inclined to require it. Both versions were released in the same month 2013. That's what this PR currently does.

I'll also write to Tim Davis to ensure we never get ABI breaks inside a minor release in the future.

Finally, I forgot to say that currently, even with this PR, sparse support doesn't work at all when the warning message is printed: since StdErr is not defined, it triggers an error, and the initialization phase does not complete. So we also need to fix #12841 separately.

tkelman · 2015-09-08T12:57:17Z

base/sparse/cholmod.jl

Could we just say "binaries from www.julialang.org" without mentioning the OS here? MSYS2 may eventually have a Julia package where this could also apply.

Sure, will fix when the PR is ready.

Fixed in the new version.

JeffBezanson · 2015-09-09T03:30:19Z

@andreasnoack @ViralBShah can somebody comment on the current state of this?

nalimilan · 2015-09-09T08:20:53Z

I've changed it to use Libdl.dlsym_e.

There's still something I don't understand in the logic of the code: why do we call cholmod_version at the top level, i.e. at build time? If we only care about the version used during the build, we could call jl_cholmod_version unconditionally there. At the moment all that is quite confusing, since the global version_array and version can get out of sync, since the former is used to store the current CHOLMOD version in __init__. Do we even need these globals? Wouldn't calling jl_cholmod_version in __init__be enough to find out the build time version of CHOLMOD?

andreasnoack · 2015-09-09T08:24:09Z

There used to be function definitions the depended on the version and therefore the constants were convenient. However, these conditions have been removed. It is likely that we don't need these constants at compile time anymore so feel free to try to remove them.

nalimilan · 2015-09-09T09:24:07Z

@andreasnoack OK, I've added a commit simplifying the logic.

nalimilan · 2015-09-09T09:37:39Z

With the last commit, the fields of the C_Factor struct are determined based on the CHOLMOD version the SuiteSparse wrapper was compiled with. This means that merely recompiling the Julia image without recompiling this wrapper won't make it take into account a change in the CHOLMOD version. I considered that's not an issue since an ABI break without rebuilding would generate problems with the wrapper anyway, right?

andreasnoack · 2015-09-09T14:01:40Z

I think your reasoning here makes good sense. Package maintainers like you are also the main audience for these checks so if they make sense from a packaging perspective, we should be good. Users who downloads the generic builds or compile from source won't have these problems.

I don't think I have any further comment. Is the pr ready now?

The check introduced in #10362 is actually too strict, as ABI break only happen with mismatches in the major version. Warning on minor version differences means distributions cannot update SuiteSparse without rebuiling Julia (#12841). We also require at least libcholmod 2.1.1. Call dlsym_e instead of dlsym, which raises an error instead of returning C_NULL. Also fix the incorrect wording introuced in 25eb444: this message corresponds to the case where the version is different, not necessarily older.

Use jl_cholmod_version only once to get the version used during the build, and do not modify this constant in __init__(). Rename variables to make them more explicit. Also only register gc tracked allocator if current version (and not build version) is recent enough.

nalimilan · 2015-09-09T14:17:19Z

That's ready from my side. I've squashed the two first commits, as they are tightly related and overlap a lot when moving the messages around.

ViralBShah · 2015-09-09T14:45:50Z

This looks ok to me too. Can merge once CI completes.

mschauer · 2015-09-09T15:13:04Z

Congrats, the last issue with milestone 0.4.0. Nice that it falls on number 13000.

nalimilan · 2015-09-09T15:19:29Z

Cool, I hadn't noticed that detail! So I have the privilege of hitting the green button. Thanks everybody for the review!

Improve CHOLMOD version mismatch detection

tkelman · 2015-09-09T17:24:57Z

base/sparse/cholmod.jl

these should be spaces, not tabs

Ah, I hate editors when they don't respect the existing style of a file. I've pushed a fix on master, should I also push it on release-0.4 to ease future backports?

just did d523dde

This was referenced Sep 7, 2015

CHOLMOD version warning is invisible due to undefined STDERR #12841

Closed

Check CHOLMOD version at runtime #10362

Merged

nalimilan reviewed Sep 7, 2015
View reviewed changes

kshyatt added the sparse Sparse arrays label Sep 7, 2015

tkelman reviewed Sep 8, 2015
View reviewed changes

JeffBezanson added this to the 0.4.0 milestone Sep 8, 2015

nalimilan force-pushed the nl/choldmod branch from 9eadb0e to 817f5c2 Compare September 8, 2015 20:24

nalimilan force-pushed the nl/choldmod branch from 817f5c2 to 76d7619 Compare September 9, 2015 08:20

nalimilan force-pushed the nl/choldmod branch from 778cede to 553a0ef Compare September 9, 2015 09:29

nalimilan added 2 commits September 9, 2015 16:13

nalimilan force-pushed the nl/choldmod branch from 553a0ef to 4c25109 Compare September 9, 2015 14:16

nalimilan added a commit that referenced this pull request Sep 9, 2015

Merge pull request #13000 from JuliaLang/nl/choldmod

e9f4e20

Improve CHOLMOD version mismatch detection

nalimilan merged commit e9f4e20 into master Sep 9, 2015

nalimilan deleted the nl/choldmod branch September 9, 2015 15:20

tkelman reviewed Sep 9, 2015
View reviewed changes

Uh oh!

Only warn in case of CHOLMOD major version mismatch #13000

Only warn in case of CHOLMOD major version mismatch #13000

Uh oh!

Conversation

nalimilan commented Sep 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalimilan commented Sep 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Sep 9, 2015

Uh oh!

nalimilan commented Sep 9, 2015

Uh oh!

andreasnoack commented Sep 9, 2015

Uh oh!

nalimilan commented Sep 9, 2015

Uh oh!

nalimilan commented Sep 9, 2015

Uh oh!

andreasnoack commented Sep 9, 2015

Uh oh!

nalimilan commented Sep 9, 2015

Uh oh!

ViralBShah commented Sep 9, 2015

Uh oh!

mschauer commented Sep 9, 2015

Uh oh!

nalimilan commented Sep 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!