Problems with Symbolics?

PierreMartinon commented 4 months ago

Something seems wrong with Symbolics at the moment: multiple warnings about deprecated calls, and errors on unknown method. ERROR: LoadError: MethodError: no method matching operation(::Symbolics.TermCombination) Downgrading from 5.28 to 5.27.1 does not fix the problems. Stacktrace point to ADNLPModels, downgrading from 0.7.2 to 0.7.1 changes nothing. Will keep watching.

PierreMartinon commented 4 months ago

Problem solved after some package updating. CI tests are fine now for julia 1.9 and 1.10

Edit: broken again

PierreMartinon commented 4 months ago

More precisely, the problem seems to be in SymbolicUtils, and fixes are in progress (update shows some downgrading from 1.7.0 to 1.6.0 at the moment). To be continued

jbcaillau commented 4 months ago

@PierreMartinon thank you for this (had not seen it). I actually just posted an issue on ADNLPModels.jl: given your comment, I am trying to update everything and re-check.

jbcaillau commented 4 months ago

@PierreMartinon tried to update everything but same error as yours still there.

@tmigot @amontoison any clue?

amontoison commented 4 months ago

I opened a PR to drop Symbolics.jl / SparseDiffTools.jl and use SparsityTracer.jl: https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/pull/230

Can you try with this branch of ADNLPModels.jl if you have the same error?

amontoison commented 4 months ago

I quickly tried and It seems that CTBase.Dynamics is not generic enough for the sparsity analysis:

ERROR: MethodError: no method matching (::CTBase.Dynamics{…})(::Int64, ::Vector{…}, ::SparseConnectivityTracer.GradientTracer{…}, ::Vector{…})

Closest candidates are:
  (::CTBase.Dynamics{Autonomous, Fixed})(::Real, ::Union{Real, AbstractVector{<:Real}}, ::Union{Real, AbstractVector{<:Real}}, ::Union{Real, AbstractVector{<:Real}})
   @ CTBase ~/.julia/packages/CTBase/xrEbg/src/functions.jl:792
  (::CTBase.Dynamics{Autonomous, Fixed})(::Union{Real, AbstractVector{<:Real}}, ::Union{Real, AbstractVector{<:Real}})
   @ CTBase ~/.julia/packages/CTBase/xrEbg/src/functions.jl:788

tmigot commented 4 months ago

Hi guys! Did you manage to identify which version change introduce the bug?

jbcaillau commented 4 months ago

Thanks @amontoison On it, doing some checks🤞🏾@tmigot keeping you posted.

I quickly tried and It seems that CTBase.Dynamics is not generic enough for the sparsity analysis:

ERROR: MethodError: no method matching (::CTBase.Dynamics{…})(::Int64, ::Vector{…}, ::SparseConnectivityTracer.GradientTracer{…}, ::Vector{…})

Closest candidates are:
  (::CTBase.Dynamics{Autonomous, Fixed})(::Real, ::Union{Real, AbstractVector{<:Real}}, ::Union{Real, AbstractVector{<:Real}}, ::Union{Real, AbstractVector{<:Real}})
   @ CTBase ~/.julia/packages/CTBase/xrEbg/src/functions.jl:792
  (::CTBase.Dynamics{Autonomous, Fixed})(::Union{Real, AbstractVector{<:Real}}, ::Union{Real, AbstractVector{<:Real}})
   @ CTBase ~/.julia/packages/CTBase/xrEbg/src/functions.jl:788

PierreMartinon commented 4 months ago

Hi everyone. Not sure if this helps, but in the CI tests that first failed, then passed, and now fails again, SymbolicsUtils was 1.6.0 then 1.7.0 then down to 1.6.0 again. The weird thing is that even though 1.7.0 is the latest, I only get 1.6.0 when adding the package in julia.

jbcaillau commented 4 months ago

@amontoison @PierreMartinon @tmigot confirming the problem persists with your branch, @amontoison (*)

I suspect that the problem is that our Dynamics function that implement the rhs $f(x,u)$ (autonomous case) or $f(t,x,u)$ (nonautonomous case) of the ODE $\dot{x}(t) = f([t,]x(t),u(t))$ accepts for the state $x$ and the control $u$ union of vectors and reals. (When the dimension is one, one may prefer to write x instead of x[1].) And Real is not a subtype of Vector.

What do you think @amontoison and @tmigot ?

(*) @amontoison naive question / side note: to perform this test

I cloned a local copy of CTDirect.jl, activated it
inside this project, I added your branch according to
```
add https://github.com/amontoison/ADNLPModels.jl#SCT
```

so that the project got recompiled with that code. Correct? Another (simpler) way to check this?

jbcaillau commented 4 months ago

Hi again @tmigot; I do not know right now, but I can try to pin previous versions and check. Indeed, the typing of CTBase.Dynamics hasn't changed recently, so this seems to be connected with updates either of ADNLPModels or Symbolic... (or both).

@PierreMartinon any clue?

Hi guys! Did you manage to identify which version change introduce the bug?

PierreMartinon commented 4 months ago

Reverting to CTDirect 0.4.6 / CTBase 0.7.2, the error is still there, and those versions passed the CI tests when they were released. I really suspect this has something to do with the symbolic operations somewhere. The 1.7.0 version of SymbolicUtils that allowed the CI to pass seems to have been unregistered, maybe they are still fixing some quirks with it. With 1.6.0 we see a lot of warnings about a deprecated istree() method, as well as the aforementioned error.

tmigot commented 4 months ago

It looks like this is related https://github.com/JuliaSymbolics/SymbolicUtils.jl/issues/595 I would suggest to either

Fix SymbolicUtils to 1.5.1: ⌃ [d1185830] SymbolicUtils v1.5.1 (which is not ideal); or,
add a comment to the SymbolicUtils issue and wait for them to fix the 1.6.

jbcaillau commented 4 months ago

Thanks @tmigot

It looks like this is related JuliaSymbolics/SymbolicUtils.jl#595 I would suggest to either

Fix SymbolicUtils to 1.5.1: ⌃ [d1185830] SymbolicUtils v1.5.1 (which is not ideal); or,

Where is this dependence?

add a comment to the SymbolicUtils issue and wait for them to fix the 1.6.

Done

amontoison commented 4 months ago

@jbcaillau @PierreMartinon Off-topic: I added a breakage test for control-toolbox/OptimalControl.jl in ADNLPModels.jl here. It means that we check that we don't break anything in your ecosystem with we do a modification in ADNLPModels.jl.

jbcaillau commented 4 months ago

@tmigot @amontoison hi there, where exactly (= in which toml file) is the dependence towards SymbolicUtils.jl in ADNLPModels?

It looks like this is related JuliaSymbolics/SymbolicUtils.jl#595 I would suggest to either

Fix SymbolicUtils to 1.5.1: ⌃ [d1185830] SymbolicUtils v1.5.1 (which is not ideal) [...]

Where is this dependence?

amontoison commented 4 months ago

In ADNLPModels.jl we don't have a compat entry for Symbolics.jl because it's an optional dependency: https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/blob/main/src/ADNLPModels.jl#L185

@tmigot We should probably update ADNLPModels.jl to have a way to add compat entry for Symbolics.jl?

tmigot commented 4 months ago

Because, Symbolics.jl is an implicit dependency we don't have a compat for it. So, maintaining the compat for Symbolics should be on your side.

Now for SymbolicsUtils, in theory, we don't need to maintain a version compat, because it is an implicit dependency. If there is a breaking release in SymbolicsUtils, then it would later mean a breaking release in Symbolics.jl and so we usually don't see the breaking changes in SymbolicsUtils. However, if we really need a patch now. We could add a version compatibility for SymbolicsUtils in the Project.toml and remove when we upgrade later on.

In ADNLPModels.jl we don't have a compat entry for Symbolics.jl because it's an optional dependency: https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/blob/main/src/ADNLPModels.jl#L185

@tmigot We should probably update ADNLPModels.jl to have a way to add compat entry for Symbolics.jl?

We have the test/Project.toml as a "guideline" for the tested version, but it is not a contrained for users of the combination ADNLPModels/Symbolics. https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/blob/main/test/Project.toml @amontoison what about we temporarily add a compat entry for SymbolicUtils there?

jbcaillau commented 4 months ago

@tmigot @amontoison Hi guys, thank you for your input. Now I get it why I could not find a reference to Symbolic[Utils].jl in your toml. It is just @required (nice mechanism, BTW).

[Require.jl] README says

@required packages can be added to the test environment of a Julia project for integration tests, or directly to the project to document compatible versions in the [compat] section of Project.toml.

So yes, a compat somewhere would be really nice as we are stuck to solve anything 😬. Can be done in a fix branch of ADNLPModels.jl on which we will add a temporary dependency on our side, if you'd rather.

amontoison commented 4 months ago

ADNLPModels.jl doesn't install Symbolics.jl by default unless the user already has it. In the control-toolbox organization, it is a dependency of your package, so we should add more restrictive compatibility settings in your packages. We can help you with that.

Note that it's not an issue to add SymbolicUtils.jl as a dependency if you already have Symbolics.jl because it will be downloaded by Symbolics.jl in all cases. This will simply give us more control over the version installed, which is what we want at this point.

jbcaillau commented 4 months ago

Solved by @tmigot and @amontoison by this patch 🙏🏽

@PierreMartinon Keeping this issue open since there is some ongoing work in SparseConnectivityTracer.jl that might offer an alternative, as suggested by @tmigot (check this issue)

amontoison commented 4 months ago

I did a meeting with Guillaume this afternoon. I asked this modification (<: Real) for you 😉

jbcaillau commented 4 months ago

@amontoison @tmigot although the current patch runs locally, there is still package incompatibility detected by the CI in the patch-1 branch. any clue?

adrhill commented 4 months ago

I asked this modification (<: Real) for you 😉

Done! https://github.com/adrhill/SparseConnectivityTracer.jl/pull/92

amontoison commented 4 months ago

@amontoison @tmigot although the current patch runs locally, there is still package incompatibility detected by the CI in the patch-1 branch. any clue?

The compat entry on Symbolics.jl is too restrictive. It should 5 and not 5.28. CI failed but you merged the PR Jean-Baptiste. I will do an hotfix.

jbcaillau commented 4 months ago

@amontoison @tmigot although the current patch runs locally, there is still package incompatibility detected by the CI in the patch-1 branch. The compat entry on Symbolics.jl is too restrictive. It should 5 and not 5.28.

CI failed but you merged the PR Jean-Baptiste.

I will do an hotfix.

😅 hasty me

jbcaillau commented 4 months ago

This PR fixes the issue: thanks @tmigot @amontoison

@PierreMartinon still, I suggest to leave the issue open as

there are ongoing changes in Symbolics.jl
and in SparseConnectivityTracer.jl that might be used instead (check above)

PierreMartinon commented 4 months ago

Back on track, thanks a lot !

adrhill commented 3 months ago

SparseConnectivityTracer.jl that might be used instead (check above)

Feel free to ping me or open an issue if there is any missing functionality. Things are moving quickly and we released SparseConnectivityTracer v0.5.0 last Friday, which supports the majority of the NLPModels test suite.

amontoison commented 3 months ago

@PierreMartinon @jbcaillau @joseph-gergaud @ocots I just released ADNLPModels.jl v0.8.0, it should fix all your issues with Symbolics.jl.

I worked with Guillaume Dalle over the last few weeks and the PR https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/pull/230 simplifies many aspects of ADNLPModels.jl:

We no longer need Symbolics.jl and SparseDiffTools.jl, relying solely on SparseConnectivityTracer.jl.
Jacobians and Hessians are now sparse by default. Previously, an optional dependency on Symbolics.jl was required.
Faster precompilation since we no longer need to download all SciML packages.
Fixes a major issue with Symbolics.jl that affected our colleagues developing OptimalControl.jl.
Faster sparsity detection, with a speed-up of 100x compared to Symbolics.jl.
No longer need to represent Lagrange multipliers as variables for sparsity detection, reducing memory pressure. We have the Hessian with respect to x and not (x, \lambda).
Fewer non-zeros in the Jacobian and Hessians, outperforming JuMP in some cases (#83). They detect more non-zeros than we do.

Next step is to improve Colpack.jl and try a pure Julia version (SparseMatrixColorings.jl) to perform the coloring. https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/issues/237

The final goal is to use DifferentiationInterface.jl in ADNLPModels.jl such that we have a unified API to allocate the tape and switch the AD backends. It also means less entretien for us.

ocots commented 3 months ago

Thanks @amontoison for this big update! I pin also @frapac who should be interested.

amontoison commented 3 months ago

We also have benchmarks with the problems from OptimizationProblems.jl (https://github.com/adrhill/SparseConnectivityTracer.jl/issues/118#issue-2336266094). We are consistently faster for the Jacobians. As for the Hessians, it depends on the problems.

gdalle commented 3 months ago

Note that the speed up wrt to Symbolics was UP TO 100x, and only computed on Jacobian test cases (this was before SCT had Hessian functionality). Can't say how we would compare on Hessians. I just don't want to over promise 😉

ocots commented 3 months ago

We also have benchmarks with the problems from OptimizationProblems.jl

@amontoison is there a list of the problems descriptions?

amontoison commented 3 months ago

Not yet on the README.md :( But the majority of problems comes from:

jbcaillau commented 3 months ago

👍🏽 @amontoison @gdalle many thanks! having a look & try at it 🤞🏾

@PierreMartinon @jbcaillau @joseph-gergaud @ocots I just released ADNLPModels.jl v0.8.0, it should fix all your issues with Symbolics.jl.

I worked with Guillaume Dalle over the last few weeks and the PR JuliaSmoothOptimizers/ADNLPModels.jl#230 simplifies many aspects of ADNLPModels.jl: [...]

Fixes a major issue with Symbolics.jl that affected our colleagues developing OptimalControl.jl.

Faster sparsity detection, with a speed-up of 100x compared to Symbolics.jl.

PierreMartinon commented 3 months ago

Hi everyone, thanks for the update. Edit: I can confirm that 0.8.0 works fine :-) Basic test suite is 15% faster, although this is not a proper benchmarking tool. I'll look into the link below from @gdalle

gdalle commented 3 months ago

Check out the performance benchmarks too!

https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/issues/241

jbcaillau commented 3 months ago

@PierreMartinon I had not noticed this commit that merges our colleagues changes: thanks 🙏🏽

I do not see any new release of CTDirect, though: have I missed something? Otherwise can you please do it, so that @ocots can update the dependencies in OptimalControl.jl?

amontoison commented 3 months ago

We plan to update the coloring quite soon with @gdalle. I think it will not be breaking for ADNLPModels.jl and could include it in a release 0.8.1. We expect a nice speed-up for Hessian coloring (~x10) but a major improvement for Jacobian coloring (~x100 -- ~x1000).

gdalle commented 3 months ago

We expect a nice speed-up for Hessian coloring (~x10) but a major improvement for Jacobian coloring (~x100 -- ~x1000).

Overselling again ;) The benchmarks are shown here https://github.com/JuliaSmoothOptimizers/ADNLPModels.jl/issues/237

Hessian speedups will be virtually inexistent (maybe x2), Jacobian speedups will be x10 to x100 but mainly because ColPack was being used incorrectly

amontoison commented 3 months ago

Major speed-up for Jacobian coloring :)

jbcaillau commented 3 months ago

Major speed-up for Jacobian coloring :)

@amontoison overselling you 🤭

@PierreMartinon new release...?

amontoison commented 3 months ago

I will release ADNLPModels.jl v0.8.1 in 30 minutes. :rocket:

PierreMartinon commented 3 months ago

Nice ! I'll update CTDirect soon. We can keep the 0.8 compat entry for ADNLPModels for now to force the upgraded versions.

Edit: 0.6.1 is out

jbcaillau commented 3 months ago

@PierreMartinon looks like this issue can be closed: please do it if you're OK!

amontoison commented 3 months ago

The next step is to use DifferentiationInterface.jl. We could have for a speed-up for performing the AD because I expect the tapes to be better allocated.

But the main speed-up that I expect will be related to the use of the symmetry for the coloring of the Hessian.

Messenger_creation_8483570861670037 If we have less colors, it means less directional derivates. We only use column coloring in ADNLPModels.jl for Hessian and Jacobian right now. I can add it for a release 0.8.2.

PierreMartinon commented 3 months ago

Hi everyone, I will close this particular issue, but the discussion lives on !

control-toolbox / CTDirect.jl

Problems with Symbolics? #88