Fix CHOLMOD version check #37
Conversation
This check was supposed to ensure major versions match, as the error message indicates. Introduced by fcaae6f.
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
=======================================
Coverage 88.70% 88.70%
=======================================
Files 4 4
Lines 1310 1310
=======================================
Hits 1162 1162
Misses 148 148
Continue to review full report at Codecov.
|
|
IIRC, CHOLMOD was changing layouts of their internal structs in patch releases which was breaking Julia silently. I believe you have to match the exact patch versions for SuiteSparse, or you can end up with a surprise once in a while. @andreasnoack may remember more of the details. |
|
If that's the case then we should report this to SuiteSparse. That would make it completely impossible to package Julia in distributions without also bundling SuiteSparse, which goes against distribution policies. As long as the SONAME doesn't change, one should be able to switch versions without recompiling anything. |
|
@Wimmerer FYI. |
|
I will look into this, and ask Dr. Davis. |
|
I'm going to close this one - since it would be dangerous to do this - until cholmod does a major version bump and then follows semver better. |
|
@Wimmerer Any news from Dr Davis about this? |
|
I wonder if @DrTimothyAldenDavis will see this on github. |
|
Please don't delete this branch. |
|
What's the issue? |
|
@ViralBShah I don't actually see any changes to the CHOLMOD structs in SuiteSparse's recent history (at least the ones referenced in the referenced PRs above), (could totally be missing them). I wonder if this will be easily fixed once we switch to Clang.jl wrappers? I can wrap any recent version and handle this internally? |
|
@DrTimothyAldenDavis @Wimmerer I can't quite hunt up the relevant issues/PRs (maybe @andreasnoack can). Basically we noticed that some of the internal cholmod structs changed between minor releases. Since Julia mirrors the C structs (but did it with offsets), upgrading cholmod without upgrading the Julia side led to the code breaking. The solution we thus adopted was to hardcode the Cholmod version Julia expects and check that on startup. This makes things difficult for linux distributions, who would like to use any Cholmod 4.x with Julia, rather than specifically 4.3.1 (as an example). For example, this PR tries to relax that check. I suppose going forward, if SuiteSparse libraries can use semantic versioning to ensure that APIs only change across major releases (and remains fully compatible across minor and patch releases), it will become a lot easier. Hope I have accurately captured the issue, and that someone can correct me if I have got it wrong. |
|
I thought I had been using semantic versioning. Which CHOLMOD versions
have different internal structs?
…On Thu, Aug 19, 2021 at 2:40 PM Viral B. Shah ***@***.***> wrote:
@DrTimothyAldenDavis <https://github.com/DrTimothyAldenDavis> @Wimmerer
<https://github.com/Wimmerer> I can't quite hunt up the relevant
issues/PRs (maybe @andreasnoack <https://github.com/andreasnoack> can).
Basically we noticed that some of the internal cholmod structs changed
between minor releases. Since Julia mirrors the C structs (but did it with
offsets), upgrading cholmod without upgrading the Julia side led to the
code breaking.
The solution we thus adopted was to hardcode the Cholmod version Julia
expects and check that on startup. This makes things difficult for linux
distributions, who would like to use any Cholmod 4.x with Julia, rather
than specifically 4.3.1 (as an example). For example, this PR tries to
relax that check.
I suppose going forward, if SuiteSparse libraries can use semantic
versioning <https://semver.org> to ensure that APIs only change across
major releases (and remains fully compatible across minor and patch
releases), it will become a lot easier.
Hope I have accurately captured the issue, and that someone can correct me
if I have got it wrong.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#37 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYIIOOUPJWHRWRATASSRQTT5VM2LANCNFSM5ADNSCNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
@DilumAluthge see above from Viral |
|
Whoops thanks. Thank goodness for the "Restore branch" button. |
|
This is difficult for me to read but it looks like maybe something was changed in v2.1.1 and 4.3.0? This was probably the issue. |
|
I haven’t changed CHOLMOD for a very long time. I can’t recall which is why
I asked which versions you thought were different.
On Thu, Aug 19, 2021 at 7:16 PM Will Kimmerer ***@***.***> wrote:
At least with respect to the CHOLMOD structs I don't see any recent
changes in Tim's repo. Does that sound right @DrTimothyAldenDavis
<https://github.com/DrTimothyAldenDavis>?
*We* changed from manual offsets -> a Julia struct -> a Clang generated
wrapper struct as I understand it. Can we just run the test suite against
all the recent SuiteSparse releases?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#37 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYIIOO52AMLBVLQDRWRDZLT5WNGBANCNFSM5ADNSCNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
--
Sent from Gmail Mobile
|
|
This is difficult for me to read but it looks like maybe something was changed in v2.1.1 and 4.3.0? This was probably the issue. |
|
Note that we have v2.1.1 as the minimum, so the changes there aren't the issue. So I think this was about changes to |
|
No that's not it. Anyways we need to dig up the exact issue. Perhaps @andreasnoack may remember. |
|
The relevant issues are JuliaLang/julia#10362 and JuliaLang/julia#10342. It looks like the issue was caused by a change in the major CHOLMOD vesion, see JuliaLang/julia#10342 (comment), so the check I implemented seems to be too strict and that this PR is correct. |
|
@DrTimothyAldenDavis Sorry for the noise! Thanks @andreasnoack. |
|
@nalimilan We'll merge this, and presumably we want to bump the SuiteSparse version in julia base and also backport to 1.7. |
|
I still don’t understand the issue. Is it CHOLMOD vs SuiteSparse version?
They differ.
Both are semantic versioning. SuiteSparse is not a package but a meta
package and its version number comes from SuiteSparse_config. I’m following
instructions from Debian on this.
A package inside the SuiteSparse meta package can bump its major version
number with API breaking changes to , say, CHOLMOD or GraphBLAS. But the
SuiteSparse major number does not bump. At most the minor version number
needs to bump.
Are you trying to use the SuiteSparse major number to see if any packages
bump in their major number? That will break, per instructions from Debian
maintainers.
On Fri, Aug 20, 2021 at 7:51 AM Viral B. Shah ***@***.***> wrote:
@nalimilan <https://github.com/nalimilan> We'll merge this, and
presumably we want to bump the SuiteSparse version in julia base and also
backport to 1.7.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#37 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYIIOODRQLJCSNQPKEW6ELT5ZFW3ANCNFSM5ADNSCNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
--
Sent from Gmail Mobile
|
|
I believe in this case, we are following the CHOLMOD version. We saw breakage across major versions of CHOLMOD - which is expected as per semver. However, when we saw the breakage, we put in a change that checked the exact version of CHOLMOD that the Julia interface was designed for. That check turns out to be too severe. This PR will only check for compatibility between CHOLMOD and its Julia interface and enforce that the major version number matches on both sides. |
|
Glad we sorted this out! Thanks everyone! A backport to 1.7 would indeed be welcome. |
|
I still don't understand. Is the change to Julia a workaround for a bad
semver on my part in CHOLMOD and/or SuiteSparse?
or is this a fix because you were incorrectly using my valid semver in
CHOLMOD and/or SuiteSparse?
…On Fri, Aug 20, 2021 at 12:02 PM Milan Bouchet-Valat < ***@***.***> wrote:
Glad we sorted this out! Thanks everyone! A backport to 1.7 would indeed
be welcome.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#37 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYIIOJCDJ3UN7RENB4G7CTT52DD7ANCNFSM5ADNSCNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
It was a bug in the Julia part. All good on the CHOLMOD/SuiteSparse front. |
This check was supposed to ensure major versions match, as the error message indicates.
Introduced by fcaae6f at JuliaLang/julia#38919.
Without this it's very hard to build Julia packages in distributions, as patch version mismatches are common.
@ViralBShah At JuliaLang/julia#38919 (comment) you said
I don't understand why this was needed, given that making the check stricter merely triggers a warning, which doesn't fix anything in itself. Anyway, can it be removed now? Note that we print a warning too if the version is older than the expected one.