Introduce package extensions

[mateuszbaran]

Some initial work on package extensions. Some things I haven't figured out yet:

1. I don't know if extensions can add new functions to parent modules, so for now BVP extension only adds methods.
2. I'm not sure what's the suggested naming of extensions that depend on more than one package.

[sethaxen]

I don't know if extensions can add new functions to parent modules, so for now BVP extension only adds methods.

No, extensions are strictly additive. They can't add new user-focused functions or types, but they can overload methods.

I'm not sure what's the suggested naming of extensions that depend on more than one package.

Extensions usually add methods defined in PackageA to take types from PackageB (our package). So the name would be PackageAPackageBExt.

[mateuszbaran]

No, extensions are strictly additive. They can't add new user-focused functions or types, but they can overload methods.

OK, that makes sense when I think about it. It's a bit more restrictive than Requires.jl but that's fine.

I'm not sure what's the suggested naming of extensions that depend on more than one package.

Extensions usually add methods defined in PackageA to take types from PackageB (our package). So the name would be PackageAPackageBExt.

What I mean here is that currently recipes.jl depends on loading both RecipesBase and Colors. https://pkgdocs.julialang.org/v1.9/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions) says that an extension can depend on two other packages being loaded so I thought I'd use that. Would the module name be something like ManifoldsRecipesBaseColorsExt?

[sethaxen]

OK, that makes sense when I think about it. It's a bit more restrictive than Requires.jl but that's fine.

More restrictive but often leads to cleaner code. e.g. if some functionality depends on loading a package, it's better to define the types and methods needed in the main package code and then implement them in the extension. How AbstractDifferentiation does this for Zygote is a good example of this: https://github.com/JuliaDiff/AbstractDifferentiation.jl/blob/master/ext/AbstractDifferentiationZygoteExt.jl.

What I mean here is that currently recipes.jl depends on loading both RecipesBase and Colors. https://pkgdocs.julialang.org/v1.9/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions) says that an extension can depend on two other packages being loaded so I thought I'd use that. Would the module name be something like ManifoldsRecipesBaseColorsExt?

No, recipes.jl only overloads RecipesBase methods, so it only extends RecipesBase with Manifolds types, so it should be named ManifoldsRecipesBaseExt. If it also extended Colors, I would recommend making that its own extension.

[mateuszbaran]

Thank you for help! What would you suggest doing about ode_callback.jl?

[sethaxen]

Thank you for help!

No problem!

What would you suggest doing about ode_callback.jl?

I'm not familiar enough with that code to know what is intended to be user-facing. My suggestion is to define any types, structs, and methods a user might need to interact with in Manifolds proper and then just implement those in an extension that depends on OrdinaryDiffEq.

[mateuszbaran]

I'm not familiar enough with that code to know what is intended to be user-facing. My suggestion is to define any types, structs, and methods a user might need to interact with in Manifolds proper and then just implement those in an extension that depends on OrdinaryDiffEq.

The one problem there is that it depends on both OrdinaryDiffEq and DiffEqCallbacks. Do you think my solution from the last commit is fine?

BTW, after just these changes load time of Manifolds.jl on Julia 1.9.0 went down below 1s on my laptop, from about 3-4s on 1.8. I honestly didn't expect such a big improvement here.

[codecov[bot]]

Codecov Report

Merging #607 (fbcde0b) into master (978e017) will decrease coverage by 0.01%. The diff coverage is 98.05%.

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
- Coverage   99.01%   99.00%   -0.01%     
==========================================
  Files         102      104       +2     
  Lines       10034    10034              
==========================================
- Hits         9935     9934       -1     
- Misses         99      100       +1     

Impacted Files	Coverage Δ
ext/ManifoldsBoundaryValueDiffEqExt.jl	100.00% <ø> (ø)
ext/ManifoldsOrdinaryDiffEqExt.jl	100.00% <ø> (ø)
ext/ManifoldsRecipesBaseExt.jl	97.61% <ø> (ø)
ext/ManifoldsTestExt/tests_general.jl	95.51% <ø> (ø)
ext/ManifoldsTestExt/tests_group.jl	95.01% <ø> (ø)
src/differentiation/ode_callback.jl	100.00% <ø> (ø)
src/manifolds/ConnectionManifold.jl	100.00% <ø> (ø)
src/Manifolds.jl	86.66% <77.77%> (-13.34%)	:arrow_down:
ext/ManifoldsNLsolveExt.jl	100.00% <100.00%> (ø)
ext/ManifoldsOrdinaryDiffEqDiffEqCallbacksExt.jl	100.00% <100.00%> (ø)
... and 1 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sethaxen]

The one problem there is that it depends on both OrdinaryDiffEq and DiffEqCallbacks. Do you think my solution from the last commit is fine?

Ah, I see what you mean now. It makes for a pretty ugly but interpretable extension name, so I think this is fine.

BTW, after just these changes load time of Manifolds.jl on Julia 1.9.0 went down below 1s on my laptop, from about 3-4s on 1.8. I honestly didn't expect such a big improvement here.

Wow! for me it went from 2.3s to 1.1s!

[mateuszbaran]

Hm, I'm not sure what to do about these:

WARNING: Method definition solve_exp_ode(ManifoldsBase.AbstractManifold{𝔽} where 𝔽, Any, Any, Number) in module Manifolds at /home/mateusz/.julia/dev/Manifolds/src/manifolds/ConnectionManifold.jl:407 overwritten in module ManifoldsOrdinaryDiffEqExt at /home/mateusz/.julia/dev/Manifolds/ext/ManifoldsOrdinaryDiffEqExt.jl:31.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition kwcall(Any, typeof(Manifolds.solve_exp_ode), ManifoldsBase.AbstractManifold{𝔽} where 𝔽, Any, Any, Number) in module Manifolds at /home/mateusz/.julia/dev/Manifolds/src/manifolds/ConnectionManifold.jl:407 overwritten in module ManifoldsOrdinaryDiffEqExt at /home/mateusz/.julia/dev/Manifolds/ext/ManifoldsOrdinaryDiffEqExt.jl:31.
  ** incremental compilation may be fatally broken for this module **

[sethaxen]

One trick is to change the definition in ConnectionManifolds.jl to have no specialized types. Then when the overload in the extension is defined, it no longer gets hit for the right types, e.g.

function solve_exp_ode(M, p, X, t; kwargs...)
    throw(
        ErrorException(
            """
            solve_exp_ode not implemented on $(typeof(M)) for point $(typeof(p)), vector $(typeof(X)).
            For a suitable default, enter `using OrdinaryDiffEq`.
            """,
        ),
    )
end

An alternative is to delete this method entirely and register an error hint: https://docs.julialang.org/en/v1/base/base/#Base.Experimental.register_error_hint

[mateuszbaran]

These error hints look nice. @kellertuer which solutions would you prefer?

[kellertuer]

Nice, I am just not sure about the verb enter since the right recommendation would be to load the package maybe?

[mateuszbaran]

Yes, I've fixed the warnings. The main issue I have at the moment is errors in documentation. I will also check coverage.

[kellertuer]

I saw the issue on Documenter and can check later today as well. I thought just loading all (weakly dependent) packages in make.jl does the job? At least I was under that impression for Manopt.jl but will check again as well.

[mateuszbaran]

Yes, it's weird because in REPL extensions work but Documenter seems to ignore them. I will try to make an MWE today.

[kellertuer]

I indeed have the same problem with the init_caches doctoring fro LRU Caches which does not show up, but that is not so important for now in Manopt I think – for best of cases those docstring should have a marker that they are from an extension maybe anyways? However, looking forward to your example :)

[sethaxen]

Yes, it's weird because in REPL extensions work but Documenter seems to ignore them. I will try to make an MWE today.

Not certain if it's the issue you're seeing, but for Documenter to pick up a docstring for a new method added in an extension, you need to add the extension module as well, see e.g. https://github.com/sethaxen/StanLogDensityProblems.jl/blob/b5067218d0d66a36c7c996fc3ae248eeda8801fd/docs/src/index.md?plain=1#L10-L17

[kellertuer]

Cool, that should at least fix the problem I had. Will keep that in mind and Update Manopt then some time.

[mateuszbaran]

Thanks, I will check if it fixes this issue.

[mateuszbaran]

It looks like Seth's approach worked :tada: . Now this PR should be good to go (i.e. to be reviewed).

[kellertuer]

Oh my comment ended up just in the commit :/

It is – that we could add all extensions where you stated that – and great that it works, will add that in Manopt next then as well.

[mateuszbaran]

Done :slightly_smiling_face: . Though I don't know how to include docs for ManifoldsOrdinaryDiffEqDiffEqCallbacksExt.StitchedChartSolution without moving it to the main package.

[sethaxen]

Though I don't know how to include docs for ManifoldsOrdinaryDiffEqDiffEqCallbacksExt.StitchedChartSolution without moving it to the main package.

You don't. Functions and types defined only in an extension cannot be easily accessed by the user and so should be considered implementation details. If the user is expected to access StitchedChartSolution, then you should drop it's type constraints and move its definition into the main package and just its constructor in the extension.

[mateuszbaran]

That's what I suspected. StitchedChartSolution could be made more user-facing but that can be done later when it's needed.

[mateuszbaran]

Eh... it looks like Pluto notebooks don't work with Julia 1.9. @kellertuer do we go back to building docs with Julia 1.8 or do you know how to fix that error?

[kellertuer]

That was still PlutoStaticHTML we use? That was not even working when Pluto itself had just patch updates usually, so we should probably continue with 1.8 for now and switch to Quarto as well at some point (in the not so far future).

[mateuszbaran]

I see, then let's use 1.8 for now.

[kellertuer]

Yes, maybe open an issue for the move? I will try to fix the cache&CI stuff on Manopt first and then do the same thing here, so we do not have to commit the md files.

[mateuszbaran]

Sure, cool.

[mateuszbaran]

The non-covered lines look like false positives.

[kellertuer]

Yes they are (have the same problem in Manopt.jl). I am just surprised the static extension check is within the requires check, you can reduce that slightly by doing it the other way around, see

https://github.com/JuliaManifolds/Manopt.jl/blob/1406c3cb1cf4307f6584b585cb4654ccd2ad9c42/src/Manopt.jl#L174-L187

[mateuszbaran]

What do you mean by other way around? To me your example looks the same as the code here.

[kellertuer]

Oh, maybe I got confused by the representation on codecov then, you are right, this is the same order, then all is fine and we can not avoid the two additional lines missed.