[BREAKING] Move functionality to extension packages

[lxvm]

This pr reduces Polyhedra.jl dependencies by moving the functionality it uses with JuMP.jl to an extension package. These changes will be breaking because it now requires loading JuMP and requires Julia v1.9. See the checklist below for what needs to be done:

• Can other dependencies be made extensions as well? (e.g. RecipesBase.jl or GenericLinearAlgebra.jl and GeometryBasics.jl)
• Is it OK to comment out JuMP calls in a debugging print statement in function detect_new_linearities?

I would appreciate additional feedback from maintainers on the utility of this pr as well as its approach. Thank you!

[blegat]

Thanks! Yes, I think it's a good idea. We should try to keep it working for Julia v1.6 as well as long as it's the LTS. In order to do that, we can simply keep the old behavior on Julia v1.6 as done in NLopt: https://github.com/JuliaOpt/NLopt.jl/blob/master/src/NLopt.jl#L633-L637
So the behavior in Julia v1.6 remains the same: the load time is not decreased even when JuMP is not used but in Julia v1.9 it's using the extension only if JuMP is loaded.

Can you post timings for comparison with Julia v1.9 or v1.10 ?

[blegat]

I think GenericLinearAlgebra is probably not a good candidate for this but RecipesBase seems to be a good one. GeometryBasics as well.

[codecov]

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (268c701) 88.95% compared to head (9493f37) 88.92%.

Files	Patch %	Lines
ext/PolyhedraJuMPExt.jl	75.00%	6 Missing ⚠️
src/Polyhedra.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   88.95%   88.92%   -0.04%     
==========================================
  Files          37       37              
  Lines        3170     3170              
==========================================
- Hits         2820     2819       -1     
- Misses        350      351       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lxvm]

@blegat Thank you for your recommendations regarding compatibility and additional extensions. I have used the strategy in NLopt to maintain 1.6 compatibility and I also moved RecipesBase and GeometryBasics to extensions.

Although these changes could be considered breaking in Julia 1.9 and above, the recommended usage in the docs and examples always loads the extension packages and so I expect these changes wouldn't impact very many users. One exception is that 3d plotting would require loading the plotting package (Makie/MeshCat) before calling Polyhedra.Mesh. If you think these problems are serious then this pr could go into a breaking, minor release. (Notably, I changed Polyhedra.Mesh from a struct to a function that returns a struct defined in the GeometryBasics extension.)

For example, the following works:

using Polyhedra, GLMakie
v = convexhull([0, 0, 0]) + conichull([1, 0, 0], [0, 1, 0], [0, 0, 1])
p = polyhedron(v)
m = Polyhedra.Mesh(p)
mesh(m; color=:blue)

but this would no longer work

using Polyhedra
v = convexhull([0, 0, 0]) + conichull([1, 0, 0], [0, 1, 0], [0, 0, 1])
p = polyhedron(v)
m = Polyhedra.Mesh(p)
using GLMakie
mesh(m; color=:blue)

[lxvm]

Starting from this gist I wrote some tests for precompilation times with the help of the juliaup installer

Test script

for (url, rev) in [("https://github.com/lxvm/Polyhedra.jl", "ext"), ("https://github.com/JuliaPolyhedra/Polyhedra.jl", "master")]
code = """
import Pkg
@show @time Pkg.add(url="$url", rev="$rev")
@show @time Pkg.precompile()
@show @time using Polyhedra
"""
for jl in [`julia +lts`, `julia +release`]
    @info "" jl ver=read(`$jl -E 'VERSION'`, String) |> strip
    mktempdir() do depot
        mkdir(joinpath(depot, "registries"))
        
        cp(expanduser("~/.julia/registries/General"), joinpath(depot, "registries/General"))
        # alternatively:
        # run(addenv(`$jl -e 'using Pkg; pkg"registry add https://github.com/JuliaRegistries/General"'`, Dict("JULIA_DEPOT_PATH" => depot)))

        for (i, date) in enumerate(["2023-12-28"])
            cd(joinpath(depot, "registries/General")) do
                commit = read(`git rev-list -n 1 --first-parent --before=$date master`, String) |> strip
                run(`git checkout $commit`)
            end
            mktempdir() do env
                cd(env)
                write("script.jl", code)
                # if i == 1
                #     @info "Initial run in depot"
                #     run(addenv(`$jl --project script.jl`, Dict("JULIA_DEPOT_PATH" => depot)))
                # end
                @info "$i-th run in depot"
                run(addenv(`$jl --project script.jl`, Dict("JULIA_DEPOT_PATH" => depot)))
            end
        end
    end
end
end

Relevant results are summarized here, taken from the raw output below

Branch	Julia version	Pkg.add (s)	using Polyhedra (s)
ext	1.6.7	97.245989	4.532855
ext	1.10.0	74.781329	1.366950
master	1.6.7	101.986754	5.395770
master	1.10.0	88.853759	1.687544

It looks like the speed-up of updating from v1.6.7 to v1.10.0 is more relevant than the improvements from moving some dependencies to extensions, although I notice Pkg.add is 14 seconds faster with this pr on v1.10.0, which is what I was hoping for.

Script output

┌ Info: 
│   jl = `julia +lts`
└   ver = "v\"1.6.7\""
...
 97.245989 seconds (8.37 M allocations: 669.307 MiB, 0.28% gc time, 0.27% compilation time)
#= /tmp/jl_tviiBK/script.jl:2 =# @time(Pkg.add(url = "https://github.com/lxvm/Polyhedra.jl", rev = "ext")) = nothing
  0.209968 seconds (288.25 k allocations: 19.944 MiB, 2.75% gc time, 32.47% compilation time)
#= /tmp/jl_tviiBK/script.jl:3 =# @time(Pkg.precompile()) = nothing
  4.532855 seconds (9.99 M allocations: 754.598 MiB, 8.27% gc time, 4.66% compilation time)
#= /tmp/jl_tviiBK/script.jl:4 =# @time(using Polyhedra) = nothing
...
┌ Info: 
│   jl = `julia +release`
└   ver = "v\"1.10.0\""
...
 74.781329 seconds (5.47 M allocations: 551.063 MiB, 0.59% gc time, 3.88% compilation time)
#= /tmp/jl_rLyGlv/script.jl:2 =# @time(Pkg.add(url = "https://github.com/lxvm/Polyhedra.jl", rev = "ext")) = nothing
  0.230983 seconds (81.43 k allocations: 12.014 MiB)
#= /tmp/jl_rLyGlv/script.jl:3 =# @time(Pkg.precompile()) = nothing
  1.366950 seconds (1.16 M allocations: 76.507 MiB, 4.63% gc time, 1.19% compilation time)
#= /tmp/jl_rLyGlv/script.jl:4 =# @time(using Polyhedra) = nothing
...
┌ Info: 
│   jl = `julia +lts`
└   ver = "v\"1.6.7\""
...
101.986754 seconds (8.37 M allocations: 669.528 MiB, 0.23% gc time, 0.28% compilation time)
#= /tmp/jl_A3xZbi/script.jl:2 =# @time(Pkg.add(url = "https://github.com/JuliaPolyhedra/Polyhedra.jl", rev = "master")) = nothing
  0.181841 seconds (285.18 k allocations: 19.723 MiB, 2.96% gc time, 23.68% compilation time)
#= /tmp/jl_A3xZbi/script.jl:3 =# @time(Pkg.precompile()) = nothing
  5.395770 seconds (10.60 M allocations: 784.972 MiB, 6.74% gc time, 2.02% compilation time)
#= /tmp/jl_A3xZbi/script.jl:4 =# @time(using Polyhedra) = nothing
...
┌ Info: 
│   jl = `julia +release`
└   ver = "v\"1.10.0\""
...
 88.853759 seconds (5.88 M allocations: 627.711 MiB, 0.62% gc time, 3.40% compilation time)
#= /tmp/jl_AxlWQe/script.jl:2 =# @time(Pkg.add(url = "https://github.com/JuliaPolyhedra/Polyhedra.jl", rev = "master")) = nothing
  0.251903 seconds (107.12 k allocations: 15.400 MiB)
#= /tmp/jl_AxlWQe/script.jl:3 =# @time(Pkg.precompile()) = nothing
  1.687544 seconds (1.46 M allocations: 96.250 MiB, 5.54% gc time, 0.91% compilation time)
#= /tmp/jl_AxlWQe/script.jl:4 =# @time(using Polyhedra) = nothing

[lxvm]

Also, regarding the CI, I think it breaks on v1.10 due to changed printing of rationals and the documentation build broke because I originally set the version to 0.8 and have since reverted to 0.7.7, assuming this can be considered non-breaking.

[blegat]

I think the breaking change with Mesh is fine, we can release a new breaking release in Polyhedra easily since we haven't hit v1.0 yet and it makes sense. I would add an error in case someone write Mesh(p) before GeometryBasics is loaded saying that it needs to be loaded first before using Mesh. Otherwise, someone might loose a lot of time debugging wondering why suddenly Mesh isn't available while on the other script he has it was ^^ I even think this error should remain there forever and not just be there because of the breaking change.

[blegat]

Can you rebase on top of the master branch ?

[lxvm]

@blegat I've rebased, uncommented the lines you requested, and added an error message for Mesh so that

using Polyhedra
v = convexhull([0, 0, 0]) + conichull([1, 0, 0], [0, 1, 0], [0, 0, 1])
p = polyhedron(v)
m = Polyhedra.Mesh(p)

throws

ERROR: this method requires using GeometryBasics
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] Mesh(p::DefaultPolyhedron{Rational{…}, Polyhedra.Intersection{…}, Polyhedra.Hull{…}})
   @ Polyhedra ~/.julia/dev/Polyhedra/src/Polyhedra.jl:115
 [3] top-level scope
   @ REPL[4]:1
Some type information was truncated. Use `show(err)` to see complete types.

[lxvm]

I took the opportunity of making a breaking change to run the Aqua tests, which most notably caught the type piracy in the FullDim union type / function. I renamed the union type to FullDims and am happy to change it if necessary. I know for a fact that this will cause CDDLib.jl to break, although the test suite still passes on my machine. I could follow up with prs to the Polyhedra.jl dependencies.

Moreover, this pr's changes are now summarized in a NEWS.md file

[blegat]

Indeed, it's type piracy. You can move this to a separate PR if you don't want it to block this one. We could make a breaking change and then a breaking release so that CDDLib is allowed to be updated. However, I would rather keep FullDim for the type and rename the function. The function had the same since it was like a constructor. If it's not a constructor, its naming should be lower case with underscores, e.g., typed_fulldim. The only difference between typed_fulldim and fulldim is that typed_fulldim gives a StaticArrays.Size if the arrays are StaticArrays

[lxvm]

Sounds good, I think it would be better to deal with the issues Aqua raises in a separate pr to get this one through, so I'll revert those commits to keep only the changes to extensions.

[lxvm]

We should be clear to run the tests again. If all goes well, I think we should revisit the version number and news file.

[blegat]

Thanks!