JuliaNLSolvers / Optim.jl

Optimization functions for Julia
Other
1.1k stars 214 forks source link

Make MathOptInterface.jl a weak dependency #1081

Closed theogf closed 3 months ago

theogf commented 4 months ago

MOI adds a very large loading time to Optim.jl although it is not always necessary see : https://github.com/jump-dev/MathOptInterface.jl/issues/2442

This PR makes MOI.jl a weak dependency, this is the difference of time for precompiling the package:

master:

julia> @time Pkg.precompile("Optim")
Precompiling Optim
  2 dependencies successfully precompiled in 112 seconds. 53 already precompiled.
113.622423 seconds (776.52 k allocations: 56.027 MiB, 0.02% gc time, 1.25% compilation time: 92% of which was recompilation)
julia> @time using Optim
  3.414563 seconds (3.74 M allocations: 231.737 MiB, 3.75% gc time, 0.28% compilation time)

This branch

julia> @time Pkg.precompile("Optim")
  4.763922 seconds (2.90 M allocations: 213.363 MiB, 1.90% gc time, 91.22% compilation time: 92% of which was recompilation)

julia> @time using Optim
  0.401095 seconds (451.49 k allocations: 30.527 MiB, 6.22% gc time, 2.45% compilation time)

For retro-compatibility I used Requires.jl as suggested in the docs.

One important change is that I could not create a reference to Optim.Optimizer <: MOI.AbstractOptimizer in Optim, so I resorted to make a constructor function moi_optimizer() that is then defined in OptimMOIExt.jl.

theogf commented 4 months ago

Bump

MilesCranmer commented 3 months ago

+1, this looks to be by far the largest contributor to load time in SymbolicRegression.jl, by far, even though I don't use MOI at all –

    322.5 ms  MathOptInterface
     79.3 ms  MutableArithmetics
     65.1 ms  DynamicQuantities
     47.5 ms  DynamicExpressions 25.11% compilation time
     29.5 ms  FillArrays
     23.1 ms  ForwardDiff
     15.8 ms  SymbolicRegression
     14.6 ms  SpecialFunctions
     14.5 ms  CompilerSupportLibraries_jll 37.01% compilation time (47% recompilation)
      8.2 ms  StatsBase
      6.9 ms  Optim
      5.4 ms  MacroTools
      3.3 ms  OpenSpecFun_jll 52.55% compilation time
      3.2 ms  IrrationalConstants
      3.2 ms  LossFunctions
      2.9 ms  MLJModelInterface
      2.7 ms  Missings
      2.2 ms  NLSolversBase
      2.1 ms  FillArrays → FillArraysSparseArraysExt
      2.0 ms  DynamicExpressions → DynamicExpressionsOptimExt
      2.0 ms  OpenLibm_jll
      2.0 ms  Zlib_jll
      1.7 ms  LineSearches
      1.5 ms  TranscodingStreams
      1.2 ms  ArrayInterface
      1.1 ms  Bzip2_jll
      1.1 ms  DocStringExtensions
      1.1 ms  FiniteDiff
      1.0 ms  CodecZlib
      1.0 ms  DiffResults
      1.0 ms  ProgressBars
      0.9 ms  Requires
      0.8 ms  DiffRules
      0.8 ms  SortingAlgorithms
      0.8 ms  StaticArraysCore
      0.7 ms  CodecBzip2
      0.7 ms  CommonSubexpressions
      0.7 ms  DataAPI
      0.7 ms  Parameters
      0.7 ms  PositiveFactorizations
      0.7 ms  ScientificTypesBase
      0.6 ms  ArrayInterface → ArrayInterfaceStaticArraysCoreExt
      0.6 ms  FillArrays → FillArraysStatisticsExt
      0.6 ms  LogExpFunctions
      0.6 ms  StatisticalTraits
      0.6 ms  StatsAPI
      0.5 ms  DynamicQuantities → DynamicQuantitiesLinearAlgebraExt
      0.5 ms  NaNMath
      0.5 ms  PackageExtensionCompat
      0.5 ms  Reexport
      0.5 ms  SuiteSparse
      0.5 ms  TranscodingStreams → TestExt
      0.5 ms  Tricks
      0.5 ms  UnPack
MilesCranmer commented 3 months ago

@theogf this is currently not compatible with Julia 1.8 as package extensions weren't added yet – I would recommend adding https://github.com/cjdoris/PackageExtensionCompat.jl to manage this.

odow commented 3 months ago

Instead of Requires, you could do https://github.com/JuliaOpt/NLopt.jl/blob/301b759ce9221514e7508081e5be2ba0cc11e03c/src/NLopt.jl#L633-L641

@blegat should take a look at this.

I think this would also be a breaking change, but it's reasonable to call this a bug-fix. It shouldn't affect too many people.

theogf commented 3 months ago

@MilesCranmer I followed your suggestion and replace Requires by PackageExtensionCompat

blegat commented 3 months ago

You can keep the name Optim.Optimizer see for instance what we did for NLopt

theogf commented 3 months ago

You can keep the name Optim.Optimizer see for instance what we did for NLopt

Ah you are using global, interesting!

theogf commented 3 months ago

@MilesCranmer The solution in NLopt would mean switching back to the Requires solution as it needs more fine tuning, I guess keeping the consistency with the previous version is more important? Note that under the hood, PackageExtensionCompat is just using Requires.

blegat commented 3 months ago

I guess keeping the consistency with the previous version is more important?

Yes, it's best to avoid having a breaking change. This naming is also a convention followed by all JuMP solvers.

MilesCranmer commented 3 months ago

Note that under the hood, PackageExtensionCompat is just using Requires.

Yeah this is why I suggested removing the Requires line, since you aren't using it here. Therefore on Julia 1.9+, Requires.jl won't be loaded unnecessarily.

MilesCranmer commented 3 months ago

What @odow is suggesting is not incompatible with PackageExtensionCompat; you just need to define Optimizer in the main codebase so it is exportable; then overload it in the extension. This is true regardless of whether you use plain Requires or PackageExtensionCompat. But if you use Requires you need to do a bunch of other stuff manually to keep compatibility for Julia < 1.9: https://discourse.julialang.org/t/package-extensions-for-julia-1-9/93397/5. Easier (and less error prone) to just let PackageExtensionCompat handle this for you.

theogf commented 3 months ago

Ok I somehow managed. I tested on 1.8 + 1.10 and it seems to work. There was unfortunately no setglobal! in v < 1.9 and there is no Compat.jl equivalent for it so I had to use @eval.

blegat commented 3 months ago

Thanks! Yes, it's a bit delicate to get this working for both Julia v1.6 and v1.10 ^^

MilesCranmer commented 3 months ago

It allowed me to define Optim.Optimizer as a const for v1.8-

Then I would do the following:

if VERSION >= v"1.9.0-DEV.0"
    @eval const Optimizer = OptimMOIExt.Optimizer
else
    @eval Optimizer = OptimMOIExt.Optimizer
end
theogf commented 3 months ago

It allowed me to define Optim.Optimizer as a const for v1.8-

Then I would do the following:

if VERSION >= v"1.9.0-DEV.0"
    @eval const Optimizer = OptimMOIExt.Optimizer
else
    @eval Optimizer = OptimMOIExt.Optimizer
end

I'd rather avoid using @eval if possible (could not find another solution for <v1.9).

MilesCranmer commented 3 months ago

What's wrong with an @eval? It's all static expressions here

ericphanson commented 3 months ago

Since it's already a hard dep, I think it makes sense to move to a weak dep only on 1.9+, as was done in NLopt. Then Requires is not needed at all (not even via PackageExtensionCompat).

MilesCranmer commented 3 months ago

Since it's already a hard dep, I think it makes sense to move to a weak dep only on 1.9+, as was done in NLopt. Then Requires is not needed at all (not even via PackageExtensionCompat).

Not sure it’s possible to have a conditional weak dependency? Note that NLopt has it as a hard dependency; it still gets installed. If you want it to be a weak dependency (i.e., not forced to be installed) and to also be compatible with <1.9, Requires seems to be required. (I would prefer to have make my downstream users install and precompile a library if it’s not used)

PackageExtensionCompat is a no-op on 1.9+ so I think it’s fine.

blegat commented 3 months ago

Note that NLopt has it as a hard dependency; it still gets installed. If you want it to be a weak dependency (i.e., not forced to be installed) and to also be compatible with <1.9

IIUC, when you have a package both as hard and weak dependency then it's interpreted as hard dependency on Julia < 1.9 and as weak dependency on Julia >= 1.9. This made it so that Julia v1.9 would ignore a hard dependency when it's also weak precisely to allow this use case

MilesCranmer commented 3 months ago

Oh, weird. What is the purpose of the [weakdeps] in that case?

pkofod commented 3 months ago

If there's no good way to do this, we have to just accept the large load time on old versions of Julia I suppose. Thanks for working on this. I have a deadline early next week and have been away for that reason. I will review this as soon as I'm back. I don't know if you have time to look at the discussion @KristofferC but I trust your judgement on the best course of action here :)

ericphanson commented 3 months ago

Btw I am referring to this strategy: https://pkgdocs.julialang.org/v1/creating-packages/#Transition-from-normal-dependency-to-extension

It is very simple and commonly used since it does not affect older usage at all, and does not need Requires or other compatibility packages.

Oh, weird. What is the purpose of the [weakdeps] in that case?

So it works as a weak dep on 1.9+. The weird thing is it’s duplicated as a “dependency” as well, so older Pkg will continue to work. Newer Pkg will see it is both a weak dep and a dep and treat it like a weak dep.

MilesCranmer commented 3 months ago

Got it, thanks! Okay I'm on board with that. I would personally prefer using PackageExtensionCompat so that old Julia versions can also have short load times (many of my downstream users have cluster-installed Julia pinned to an older version; not everybody is always up-to-date, especially less experienced users who are less patient about load time), but no strong feelings.

theogf commented 3 months ago

I am not sure what the conclusion is here :sweat_smile:

Should I implement the solution proposed by @blegat and @ericphanson ?

MilesCranmer commented 3 months ago

Well, Requires.jl has a 0.5ms load time on Julia 1.9+, and in using it, we save a 300ms load time on Julia <1.9. Seems like an obvious solution to use PackageExtensionCompat. i.e., go with the current PR. But the others might disagree on the basis of minimizing dependencies. I think for massive gains like this it's worth it though!

I would also add the

if VERSION >= v"1.9.0-DEV.0"
    @eval const Optimizer = OptimMOIExt.Optimizer
else
    @eval Optimizer = OptimMOIExt.Optimizer  # Or other way to get `const`?
end

as mentioned earlier.

theogf commented 3 months ago

I unified it as :

function __init__()
    @static if VERSION >= v"1.9"
        @eval Optim begin
            OptimMOIExt = Base.get_extension(@__MODULE__, :OptimMOIExt)
            const Optimizer = OptimMOIExt.Optimizer
        end
    else
        @eval Optim begin
            using .OptimMOIExt
            const Optimizer = OptimMOIExt.Optimizer
        end
    end
end
MilesCranmer commented 3 months ago

LGTM. But you also need to add MathOptInterface to targets.test in Project.toml, otherwise it won't install it during testing. (Did you run the tests and verify they pass?)

As we can see, the load times are more than 50% reduction of where they were before!

PackageExtensionCompat.jl is only a 0.5ms load time on v1.10, which is literally less than the measurement noise:

julia> @time_imports using Optim
      0.6 ms  Compat
      0.4 ms  Compat → CompatLinearAlgebraExt
               ┌ 3.8 ms SuiteSparse_jll.__init__()
     19.3 ms  SuiteSparse_jll 76.11% compilation time
               ┌ 4.7 ms SparseArrays.CHOLMOD.__init__() 83.53% compilation time
    132.9 ms  SparseArrays 2.97% compilation time
      0.4 ms  SuiteSparse
      1.1 ms  ArrayInterface
               ┌ 0.0 ms Requires.__init__()
      0.6 ms  Requires
      1.0 ms  FiniteDiff
      3.2 ms  IrrationalConstants
      0.8 ms  DiffRules
      0.9 ms  StaticArraysCore
      0.7 ms  ArrayInterface → ArrayInterfaceStaticArraysCoreExt
      0.9 ms  DiffResults
      8.1 ms  Preferences
               ┌ 0.8 ms OpenLibm_jll.__init__()
      1.6 ms  OpenLibm_jll
      0.6 ms  NaNMath
               ┌ 0.0 ms DocStringExtensions.__init__()
      1.3 ms  DocStringExtensions
      0.6 ms  LogExpFunctions
      0.5 ms  JLLWrappers
               ┌ 12.3 ms CompilerSupportLibraries_jll.__init__() 27.76% compilation time
     13.0 ms  CompilerSupportLibraries_jll 26.28% compilation time
               ┌ 2.8 ms OpenSpecFun_jll.__init__() 76.63% compilation time
      3.7 ms  OpenSpecFun_jll 57.06% compilation time
      5.5 ms  SpecialFunctions
      6.3 ms  MacroTools
      0.6 ms  CommonSubexpressions
     25.5 ms  ForwardDiff
               ┌ 0.1 ms Distributed.__init__()
     10.0 ms  Distributed
      2.1 ms  NLSolversBase
      0.6 ms  PositiveFactorizations
      2.8 ms  OrderedCollections
      0.4 ms  UnPack
      0.6 ms  Parameters
      1.5 ms  LineSearches
     32.7 ms  FillArrays 36.50% compilation time
      2.2 ms  FillArrays → FillArraysSparseArraysExt
      0.5 ms  PackageExtensionCompat
      0.8 ms  DataAPI
     19.3 ms  DataStructures
      1.3 ms  SortingAlgorithms
      2.9 ms  Missings
      0.9 ms  Statistics
      0.5 ms  FillArrays → FillArraysStatisticsExt
      0.7 ms  StatsAPI
      8.7 ms  StatsBase
               ┌ 0.0 ms Optim.__init__()
      5.2 ms  Optim

for a total load time of 350ms.

KristofferC commented 3 months ago

🎉

theogf commented 3 months ago

Did you run the tests and verify they pass?

I did try locally but not via ] test so it was missing. I was hoping for the CI workflows to run for testing all setups more easily :sweat_smile:

But you're right it was missing.

pkofod commented 3 months ago

🎉

thanks for taking a look!

blegat commented 3 months ago

@pkofod Can you approve the CI so that it runs ?

pkofod commented 3 months ago

@pkofod Can you approve the CI so that it runs ?

done. Let's see :)

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 85.43%. Comparing base (78ab1f4) to head (0df0cf1).

Files Patch % Lines
src/Optim.jl 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1081 +/- ## ========================================== + Coverage 84.85% 85.43% +0.58% ========================================== Files 46 45 -1 Lines 3480 3276 -204 ========================================== - Hits 2953 2799 -154 + Misses 527 477 -50 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

theogf commented 3 months ago

Nice, everything is passing. Do you want me to bump the version on this MR or will you do it after the merge? Should it be 1.10.0 or 1.9.3 ? (I think it's not worth the breakage to tag 2.0?

MilesCranmer commented 3 months ago

I think it could be 1.9.3 since the MOI code was just a wrapper; not usable unless someone had already loaded MOI elsewhere.

andreasnoack commented 3 months ago

Please see https://github.com/JuliaNLSolvers/Optim.jl/issues/1087