Support various array types in extensions

[gdalle]

Fix #766 by adding an extension for StaticArrays.jl and a custom dispatch for ktypeof(::StaticVector).

The extension is loaded conditionally with Requires.jl for Julia < 1.9.
This means that Requires comes as a new dependency.

EDIT: also fix #769

[gdalle]

@abelsiqueira is right, this is what I'm doing in the PR. Using the built in extension mechanism introduced in 1.9, and Requires for previous versions. See https://pkgdocs.julialang.org/v1/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions)

[amontoison]

I'm not at my computer right now so I can't test what S.name.name does for a StaticVector, but it sounds like a working solution. The only trouble is that we would have to list each subtype of StaticVector manually, and that this particular line of code needs to change again for each new array package.

S.name.name returns :SArray. I don't understand why do your have to list each subtype of a StaticVector.
If you replace

if S.name.name == :Zeros || S.name.name == :Ones

by

if S.name.name == :Zeros || S.name.name == :Ones || S.name.name == :SArray || S.name.name || :MArray

it should work.

You don't have to check for VERSION < 1.9, although you need to check for :get_extensions and use Requires.jl then - which is what is being done here.
Is that your opposition? Using the conditional to determine whether to use Requires.jl? Are you concerned with something in particular?

I just find my solution simple and type-stable.
Why do we need to modify 58 lines, add a dependency and an extension if just one modification is enough and is doing exactly the same job?

If we add one extension for StaticArrays.jl, we should also do the same thing for CUDA.jl, AMDGPU.jl, OneAPI.jl, etc...
I prefer to do that when Julia 1.10 will be released such that I don't need to update again these files.
I will add the extensions because it's easy to test the associated code and this is my main motivation for adding them in the future.

I don't have any opposition again package extensions otherwise. 😃

[abelsiqueira]

It's good that the current solution is type stable. @gdalle, can you comment if that works for your current use case? It looks like it can be tested using the branch from #768.

@amontoison, your code is simple, but also it is obfuscated. S.name.name and the symbols (:Zeros, etc.) are not really descriptive. Having an extension and using dispatch makes it clear from which package it comes and which function made that happen.

If we add one extension for StaticArrays.jl, we should also do the same thing for CUDA.jl, AMDGPU.jl, OneAPI.jl, etc...

That is the idea (if necessary). Extensions are supposed to be free and efficient.

I prefer to do that when Julia 1.10 will be released such that I don't need to update again these files.

The code won't break when 1.10 is released, and if we don't immediately drop 1.6, it might remain in use. Furthermore, I hope @gdalle will still be around to help update the code if needed.

All that being said, my main concern is the cost increase of adding Requires as a dependency. I wonder if either of you has some benchmarks on 1.6 and 1.9?

[gdalle]

Indeed the idea is that extensions are basically free, and packages can support as many as they want. I encourage you to watch Kristoffer's JuliaCon 2023 talk about them.

It takes a bit more code to set them up, but I have done this before and I can help. Once it's done, they are much more readable and maintainable than what's currently in place.

Requires is only necessary for < 1.9, it's not even loaded (just downloaded) in the latest version. I'll run some benchmarks for you in the evening for download, precompilation and loading time

[gdalle]

I'll test the #768 branch later tonight, thanks for the quick fix! That PR will probably solve my immediate issue, so I'd be happy if it gets merged quickly. And in the meantime I could turn the current PR into a more generic move from name.name to extensions for all array backends that are known to cause problems?

[gdalle]

I did the same thing for FillArrays, which allowed me to remove some lines in krylov_utils.jl. I also moved from Requires to PackageExtensionTools, which further cuts down the additional code.

If you disregard the new tests (new tests are never a waste of LOCs), I think you'll find this PR hits a very reasonable tradeoff in complexity :)

[gdalle]

Here are some benchmarks, obtained with the following commands:

shell> rm -rf ~/.julia/compiled; rm -rf ~/.julia/packages;

julia> ENV["JULIA_PKG_PRECOMPILE_AUTO"]=0;

julia> using Pkg

julia> Pkg.activate(temp=true)

julia> Pkg.add(url="https://github.com/gdalle/Krylov.jl", rev="staticarrays_extension")
# julia> Pkg.add(url="https://github.com/JuliaSmoothOptimizers/Krylov.jl", rev="main")

julia> Pkg.precompile("Krylov", timing=true)
# julia> @time Pkg.precompile("Krylov") # on < 1.9

julia> @time_imports using Krylov
# julia> @time using Krylov # on < 1.8

shell> rm -rf ~/.julia/compiled; rm -rf ~/.julia/packages;

julia> @show VERSION

Julia 1.9

This branch:

julia> Pkg.precompile("Krylov", timing=true)
Precompiling Krylov
    900.1 ms  ✓ PackageExtensionCompat
   7800.2 ms  ✓ Krylov
  2 dependencies successfully precompiled in 9 seconds

julia> @time_imports using Krylov
      0.9 ms  PackageExtensionCompat
    246.2 ms  Krylov

Main branch:

julia> Pkg.precompile("Krylov", timing=true)
Precompiling Krylov
   7111.4 ms  ✓ Krylov
  1 dependency successfully precompiled in 7 seconds

julia> @time_imports using Krylov
    224.4 ms  Krylov

Julia 1.8

This branch:

julia> @time Pkg.precompile("Krylov") # on < 1.9
Precompiling project...
  3 dependencies successfully precompiled in 9 seconds
  8.552460 seconds (98.20 k allocations: 5.775 MiB, 1.46% compilation time)

julia> @time_imports using Krylov
      0.5 ms  Requires
      0.2 ms  PackageExtensionCompat
     84.1 ms  Krylov 32.45% compilation time (8% recompilation)

Main branch:

julia> @time Pkg.precompile("Krylov") # on < 1.9
Precompiling project...
  1 dependency successfully precompiled in 7 seconds
  7.477803 seconds (67.81 k allocations: 3.921 MiB, 1.42% gc time, 2.57% compilation time)

julia> @time_imports using Krylov
     45.2 ms  Krylov

So essentially the performance hit seems negligible, except on Julia < 1.9 for the load time. And I still believe it is worth exploring this as a general paradigm, because that's how it will work in the future of Julia.

[gdalle]

@amontoison I think this is ready, if you want to approve the workflows

[gdalle]

Found this old PR while cleaning things up, is it still relevant @amontoison ?

[amontoison]

Hi @gdalle!
Do you have some array types that are not yet supported?
Otherwise, I propose to wait the next LTS version of Julia.

[oscardssmith]

I really think this approach is the right way to go. If we want to avoid the overhead for Julia <1.9, we could continue to use the .name.name approach and not load the extension with Requires at all for older julia versions. Especially since #768 didn't actually fix ComponentArrays (see #769 (comment)) I think this PR is the better way forward.

[amontoison]

@oscardssmith Is your use case with ComponentArrays?
I wanted to wait the next LTS release and add extensions for all relevant arrays types.

[oscardssmith]

Yes.
Why do you want to wait? I can understand not wanting to support them on versions <1.9, but there's no rule that says that all features need to be supported on all julia versions. Surely supporting ComponentArrays on Julia 1.9+ now is better than not supporting them on any julia version.

[amontoison]

Yes. Why do you want to wait? I can understand not wanting to support them on versions <1.9, but there's no rule that says that all features need to be supported on all julia versions. Surely supporting ComponentArrays on Julia 1.9+ now is better than not supporting them on any julia version.

I wanted to keep support for the LTS version but not using Requires, only package extensions.

[oscardssmith]

What's wrong with using Requires on LTS?

[amontoison]

@oscardssmith I opened #859 to fix the issue with ComponentArrays.

[oscardssmith]

Thanks! I do really think that pkg extension/requires is a better fix, but #859 is definitely better than status quo.

[gdalle]

You can also have package extensions that are just not loaded on <1.9, no need for Requires in that case

[amontoison]

Thanks! I do really think that pkg extension/requires is a better fix, but #859 is definitely better than status quo.

I agree with both of you Oscar and Guillaume.

You can also have package extensions that are just not loaded on <1.9, no need for Requires in that case

Oh, good to know!

I plan to do a release 1.0.0 of Krylov.jl this year, let's drop Julia 1.6 and requires at least Julia 1.9 and add package extensions at this moment. Is it fine for you?
The new LTS might not happen for a while.

[gdalle]

Now that 1.10 is the LTS, it might be time to revisit this