Move CHOLMOD.common_struct from Vector{UInt8} to an actual struct

[ettersi]

Related: JuliaLang/LinearAlgebra.jl#649

[musm]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

[andreasnoack]

While there is no doubt that it is nicer to work with a Julia struct like this, the potential issue here is that the common struct changes over time, see https://github.com/DrTimothyAldenDavis/SuiteSparse/blame/master/CHOLMOD/Include/cholmod_core.h. With a shim, we don't have to do anything but compile the shim for each SuiteSparse release but if we layout the struct manually in Julia then we'd have to keep it in sync with the C header file. I'm quite confident that we'll fail to do that.

[ettersi]

I guess we could write a test which checks that the C and Julia structs are indeed equivalent. That's probably a good idea anyway, because it is quite easy to make a mistake when translating all the many many fields of cholmod_common.

Is it possible to get Make to compile a library which is used only for testing? Moving jl_cholmod_common_offset into such a library and expanding it to test all fields would probably be the cleanest way to implement this.

[ViralBShah]

Yes, we can add a check at build time.

I guess we could also use Clang.jl to auto-generate the struct definition. Perhaps best done whenever we are able to move the sparse stuff into a separate repo.

[ettersi]

I'm currently busy teaching, but I'd be happy to pick this up again once the semester is over (around mid-April) if there is any chance that this PR could be accepted. If not (Viral's comment sounds like you might want to wait for a more major overhaul), then I'd greatly appreciate if you let me know.

[ViralBShah]

@andreasnoack seems to be not in favour of this for maintainability reasons, which I agree with as well. So the only way to address that is auto-generation with Clang.jl or something - in which case it would have to wait for the separation of repos.

[ettersi]

All right, thanks for letting me know!

One minor remark: I interpreted @andreasnoack 's remark over in Issue JuliaLang/LinearAlgebra.jl#649 as encouragement for this PR, so I can't deny that I am a little disappointed that apparently the proposed improvement was actually doomed from the beginning. I'm sure this was just miscommunication and quite possibly I am the one to blame here, but it takes two to miscommunicate so I thought I'd share my feelings in case it could be helpful in the future.

[ViralBShah]

Sorry, my intention was not to disappoint.

We really do want to get this PR in - but just with a guard against changes to the struct upstream. BTW, building a separate test library for this as you suggest ought to be something that will be acceptable here, since that will provide a guard against struct changes.

[ViralBShah]

I'm changing my mind on this. This will let us get rid of the SuiteSparse wrapper - gets us really close to getting rid of the wrapper file. @ettersi Can you help us get this over the finish line? It would be good to add some comments in the code that point to where the struct is (perhaps a github link to Tim Davis' code?)

Can we add some checks to the code so that we can do a CHOLMOD version check so that if SuiteSparse is bumped without checking the code, a warning is given - so that whoever bumps will take a look at the warning? I think that would be sufficient to get this done.

[ettersi]

I'd love to help, but I won't be able to do so before May. Any chance you guys can wait until then?

[ViralBShah]

Yes, there's no urgency. We have a working system right now. So we just get it done when its ready.

[ettersi]

Okay, I believe I addressed your concerns. Specifically, I added tests to CHOLMOD.__init__() to check that the size and offsets of the C cholmod_common and Julia Common structs match. I also added some comments to point out the dependencies between the various pieces of code. Let me know if you have any further concerns.

[ViralBShah]

Is it possible to get rid of the C wrapper code completely?

[ViralBShah]

We should also hard-code the version of cholmod for which this struct is coded up, so that if cholmod is bumped in the distribution, one has to bump the version required in cholmod.jl as well - to avoid accidental breakage when the struct goes out of sync when upstream libraries are bumped.

[ettersi]

I have added tests in CHOLMOD.__init__() which check that the Julia and C struct definitions are compatible (have same size and offsets). That should address the issue of accidental breaks, no?

[ettersi]

Is it possible to get rid of the C wrapper code completely?

Don't think so. I don't see how we could replicate any of the functions in
https://github.com/JuliaLang/julia/blob/cebee5ab93410f0b1784dfba7d35486a78659f86/deps/SuiteSparse_wrapper.c
in Julia.

[ViralBShah]

If the sizes and offsets are the same in the C and Julia codes, can we not just get rid of the wrapper?

[ettersi]

No, because we need the wrapper to check that the offsets are indeed the same (unless there's some trick to get around this that I am not aware of?). Also, SuiteSparse defines a SuiteSparse_long type that we need to extract.

[ViralBShah]

I am imagining that we could write a little C program that calculates all these things and writes them out into a .jl file that can be part of our Julia wrappers for SuiteSparse. An attempt to do was made by @simonbyrne in JuliaPackaging/Yggdrasil#505

There's an overall issues to get rid of the wrapper: https://github.com/JuliaLang/julia/issues/34725

[ViralBShah]

This PR will also fix #20985

[ettersi]

I am imagining that we could write a little C program that calculates all these things and writes them out into a .jl file that can be part of our Julia wrappers for SuiteSparse.

I guess in this case we might as well auto-generate the struct definitions using Clang.jl. I'd be happy to have a go at this, but if I understand correctly then simonbyrne and Gnimuc already tried and got stuck so I doubt I'd get very far without some very detailed explanations.

[Gnimuc]

I believe it's possible to do cross-compilation with pure Clang.jl, build flags(include dirs, defines, targets) and corresponding artifacts(system headers).

[ettersi]

I believe the failed tests are because the SuiteSparse_wrapper.c file in the Julia repo is now out of sync with the one in Yggdrasil.
This hypothesis is based on the observation that the first error message in the failed tests is

��� Error: Error during initialization of module CHOLMOD
���   exception =
���    could not load symbol "jl_cholmod_method_offsets":
���    /buildworker/worker/tester_linux64/build/bin/../lib/julia/libsuitesparse_wrapper.so: undefined symbol: jl_cholmod_method_offsets

I know only little about how Yggdrasil works and how it interacts with base Julia, but Yggdrasil being out of sync with Julia is the only reason I can think of why some tests run into the above while others don't.

What is the procedure for resolving this issue? Should I create another PR in Yggdrasil with the required changes, and then whoever merges has to make sure they merge both at the same time? Or is there a safer way to do this?

---

Also, I've moved the offset checks from CHOLMOD.__init__() to the cholmod test file. Placing these checks in __init__() was the result of some misconceptions I had about how linking works, and I now believe that it makes no sense to run these checks every time someone loads the CHOLMOD package.

On a related topic, it seems that CHOLMOD occasionally rearranges the order of the cholmod_common fields without bumping the major version number (at least that's the impression I got looking at the blame history). So maybe this would require us to be a bit more strict when comparing version numbers on this line?

Long story short, it would probably be good if someone with a better understanding of versioning and linking would have another look at this to make sure I did not mess things up.

[Gnimuc]

@Gnimuc's idea to use Clang.jl (JuliaPackaging/Yggdrasil#505 (comment)) looks like it works, but would require someone to integrate it with BinaryBuilder.jl (JuliaPackaging/BinaryBuilder.jl#673)

@simonbyrne I just wrote a concrete example using Clang.jl's new generator, indeed, with some hacks: https://github.com/Gnimuc/SuiteSparseGenerator.

I now get stuck with the sysroot flag, even I passed it to libclang via --sysroot=..., it just failed to recognize it.

[ViralBShah]

@ettersi The new SuiteSparse_wrapper.c for now should just be a PR to the Yggdrasil repo - specifically, replace this file with the new one: https://github.com/JuliaPackaging/Yggdrasil/tree/master/S/SuiteSparse/SuiteSparse/bundled/SuiteSparse_wrapper

The PR will kick off the build process. Separately, we should tighten the version checking to closely follow CHOLMOD down to the patch level for now. Both the Yggdrasil PRs and this one should be merged in close succession - with Yggdrasil first. I'm happy to help with the merging.

[ettersi]

All right, I tightened the version number check and created the Yggdrasil pull request (JuliaPackaging/Yggdrasil#2848). I believe this should be good to go now?

[ViralBShah]

I've merged the Yggdrasil PR, which should propagate into the relevant places in the next hour or so. We will need all green here based on that PR and then merge this one.

[ViralBShah]

I merged JuliaRegistries/General#34441 by hand.

[ViralBShah]

This PR needs to update https://github.com/JuliaLang/julia/blob/master/stdlib/SuiteSparse_jll/Project.toml. The version needs to be changed to 5.8.1+2 so that it picks up the new SuiteSparse binaries.

[ettersi]

Last remaining error:

make: *** No rule to make target 'win-extras'.  Stop.

Unrelated? 🤞

[ViralBShah]

We should rebase to master, since this PR is against a very old master. Most likely that should fix the test failures on windows.

[ettersi]

Did the rebase. The tests which are failing now were added in #39301, and it seems there are some issues with these tests, see the recent discussion in #40504. I guess we wait until these issues are resolved and try again.

[ettersi]

All green! 🎉🎉🎉

[ettersi]

Awesome, thanks everyone for all your help! As you could probably tell, this was my first (significant) contribution to Julia, and thanks to all of you this has been a very enjoyable and rewarding experience!

[ViralBShah]

Thanks @ettersi for the contribution and staying with the process - which can be a bit long at times. But as a result now we have one more person who understands our sparse solver infrastructure :-)