Drop support for versions of Julia lower than v1.6 #985
Conversation
- I actually don't understand this one: it was previously bounded to _below_ v1.5 - but that would indicate we assume allocations on later versions?
|
Incidentally, this also includes the same fix as in #970 (i.e. bump docs versioning to 1.6). |
test/ambiguities.jl
Outdated
| # TODO: Revisit and fix. See | ||
| # https://github.com/JuliaLang/julia/pull/36962 | ||
| # https://github.com/JuliaLang/julia/issues/36951 | ||
| @test_broken length(detect_ambiguities(#=LinearAlgebra, =#StaticArrays)) <= allowable_ambiguities |
There was a problem hiding this comment.
Seems a pity to have this turned off entirely, is there some reasonable nonzero value we can still set this to?
Upstream did seemingly break this test without obvious recourse, so if ignoring this is the best we can do, so be it.
There was a problem hiding this comment.
Currently we have:
length(detect_ambiguities(StaticArrays)) # = 49 on v1.6.5 and 48 on v1.7.1
length(detect_ambiguities(LinearAlgebra, StaticArrays)) # = 49 on v1.6.5 and 48 on v1.7.1I'm not sure why the LinearAlgebra thing was commented out.
48 is of course a lot higher than the small numbers that were given for earlier versions before - but I don't understand what made this code and branching on versions necessary in the first place; presumably one of those PRs linked above.
If we want something we could set it to e.g. 55 - then we'd at least be told if things suddenly increase a lot. We could also set it lower/higher, depending on tastes?
There was a problem hiding this comment.
Heh... I rebased this PR just now and that increased the number of ambiguities from 49 to 61 (on v1.6)...
So, yeah, there's some downside to not tracking it I guess.
EDIT: Seems the real explanation actually is that I'm seeing 49 ambiguities when I run:
using Test, StaticArrays
detect_ambiguities(StaticArrays)But there's 61 ambiguities when running the test suite: I wonder how that happens; somehow, something in the tests introduce additional ambiguities.
There was a problem hiding this comment.
A follow-up on the difference of ambiguity counts. Turns out there's two earlier @testsets which define new structs that add to the ambiguity count. Specifically, this happens here:
- 5 ambiguities:
StaticArrays.jl/test/FieldVector.jl
Lines 86 to 90 in d336cfb
- 7 ambiguities:
StaticArrays.jl/test/FieldMatrix.jl
Lines 94 to 98 in d336cfb
I.e., the test suite itself adds extra ambiguities: this is probably undesirable, but also an issue that adding certain subtypes of FieldVector and FieldMatrix introduces new ambiguities.
There was a problem hiding this comment.
For this PR, I just updated the numbers to 61 (v1.6) and 60 (v1.7). I also made the change that the ambiguity check should be ensure unchanged number ambiguities. If a change decreases the number of ambiguities, it should decrease the values in /test/ambiguities.jl accordingly.
Probably, it would be good with some ambiguity hunting at some point...
|
Good point, I turned those off for now. |
- require unchanged ambiguity counts or an updated value of `allowable_ambiguities`
|
@andyferris @mateuszbaran — unless you foresee any big problems with this, I think we should merge it soon. I've a small worry that it could bifurcate the ecosystem a bit with respect to requiring Julia 1.6, due to dependees inadvertently requiring the latest version of |
|
One thing I wanted to ask about: CI got stuck on the benchmarks which ran forever - is that something to be worried about? |
That does seem a bit problematic. Does it run fine for you locally? |
Tried just now: |
|
I've kicked off the benchmarks again in CI — I guess we'll see whether it's a transient failure. |
I think merging this is fine, though I'm not certain if it's worth it. For me continuing support for Julia 1.0 didn't introduce any significant issues but after this change patching the Julia 1.0-compatible version will be more convoluted. Older versions of Julia sometimes had random CI failures (IIRC mostly 1.3) so we could just drop support for Julia 1.1 to 1.5 with nearly no impact on the ecosystem and cleaner CI. Anyway, these are not big problems and I'm fine with dropping support for Julia 1.0. |
|
The benchmark CI timed out again. Ideas? |
|
I'm not sure what's going on with the benchmarks — will need some experimentation. They seem to start fine, then get stuck in It seems that there's a way to interactively debug github actions by getting an interactive shell on the code runner, so I'd suggest trying that out: https://github.com/marketplace/actions/debugging-with-tmate |
|
I have just triggered the Benchmark CI again, and it now passed.
|
|
Thanks for picking this up and checking CI again @hyrodium. Looking at the logs, and as also highlighted by @c42f, the issue is that Concretely, running the below snippet in a fresh session to get (compilation) timings for N = 10; SA = @SMatrix rand(N,N); @time exp(SA)I get:
With † indicating that I did not try to get the numbers. Coincidentally, I think this is a another reason to drop <v1.6: benchmarking on v1.0 has effectively hidden this issue for the Julia versions that most users will care about. It could even be an argument to benchmark on whatever is the most recent release of Julia. It is probably a bad idea to be benchmarking I suggest we change change StaticArrays.jl/benchmark/bench_matrix_ops.jl Lines 9 to 13 in 4225f1b Btw., I previously wrote:
But that's nonsense: doing |
|
On a technical level, I think this is ready now (e.g. the benchmarks should run in finite time again). The remaining question is whether there's consensus around the decision to drop support for pre-LTS versions? |
I think there is as much consensus as there can be for such change. Ultimately the only way to figure out whether this is acceptable is tagging a release that drops support for pre-LTS versions of Julia and seeing who complains 🙂 . We can always undo this change. |
I think that makes sense |
|
Stacktrace: |
I haven't seen that. Some recent compiler changes might have caused that and it may be worth reporting. |
Personally, I'm happy with this decision - this package always evolves with the compiler and standard library. If people have code using Julia 1.0-1.5 they probably don't want StaticArrays APIs to be evolving either! |
Same error happend on Linux on master now. |
|
Thanks @thchr for debugging and pushing through with this! |
We discussed this briefly on Slack: now that the new Julia LTS is v1.6, it would make sense to drop support for earlier versions. There's at least two good reasons for doing so:
VERSIONchecks in StaticArrays currently (in source files and in tests).I'm putting this up mainly for discussion - but the relevant bits to actually do this should also be here and are split across independent commits.
I also bumped the minor version of StaticArrays to v1.4: quoting @c42f from Slack:
One obvious counter-point is that this could cause a lot of downstream "noise" for packages that try to support pre-LTS versions of Julia. A similar fear could be that this could cause some fracturing of the compat versions of StaticArrays in the ecosystem, which could lead to compat conflicts.
Conversely, since StaticArrays is so widely used, this could also be seen as good thing, in that it would help speed up a broader transition to move on to v1.6+.