Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problems with Symbolics? #88

Closed
PierreMartinon opened this issue May 15, 2024 · 48 comments
Closed

Problems with Symbolics? #88

PierreMartinon opened this issue May 15, 2024 · 48 comments
Assignees

Comments

@PierreMartinon
Copy link
Member

PierreMartinon commented May 15, 2024

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
Copy link
Member Author

PierreMartinon commented May 17, 2024

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

Edit: broken again

@PierreMartinon
Copy link
Member Author

PierreMartinon commented May 17, 2024

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
Copy link
Member

@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
Copy link
Member

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

@tmigot @amontoison any clue?

@amontoison
Copy link
Contributor

amontoison commented May 19, 2024

I opened a PR to drop Symbolics.jl / SparseDiffTools.jl and use SparsityTracer.jl:
JuliaSmoothOptimizers/ADNLPModels.jl#230

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

@amontoison
Copy link
Contributor

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
Copy link
Contributor

tmigot commented May 20, 2024

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

@jbcaillau
Copy link
Member

jbcaillau commented May 20, 2024

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
Copy link
Member Author

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
Copy link
Member

jbcaillau commented May 20, 2024

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

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?
IMG_3138

@jbcaillau
Copy link
Member

jbcaillau commented May 20, 2024

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
Copy link
Member Author

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
Copy link
Contributor

tmigot commented May 21, 2024

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,
  • add a comment to the SymbolicUtils issue and wait for them to fix the 1.6.

@jbcaillau
Copy link
Member

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

@jbcaillau jbcaillau changed the title Problems with Symbolics ? Problems with Symbolics? May 21, 2024
@amontoison
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Contributor

tmigot commented May 22, 2024

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
Copy link
Member

jbcaillau commented May 22, 2024

@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
Copy link
Contributor

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
Copy link
Member

jbcaillau commented May 22, 2024

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
Copy link
Contributor

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

@jbcaillau
Copy link
Member

jbcaillau commented May 22, 2024

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

@adrhill
Copy link

adrhill commented May 22, 2024

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

Done! adrhill/SparseConnectivityTracer.jl#92

@amontoison
Copy link
Contributor

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

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
Copy link
Member

@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
Copy link
Member

This PR fixes the issue: thanks @tmigot @amontoison

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

@PierreMartinon
Copy link
Member Author

Back on track, thanks a lot !

@adrhill
Copy link

adrhill commented May 28, 2024

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
Copy link
Contributor

amontoison commented Jun 5, 2024

@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:

  • 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.
JuliaSmoothOptimizers/ADNLPModels.jl#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
Copy link
Member

ocots commented Jun 5, 2024

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

@amontoison
Copy link
Contributor

We also have benchmarks with the problems from OptimizationProblems.jl (adrhill/SparseConnectivityTracer.jl#118 (comment)).
We are consistently faster for the Jacobians.
As for the Hessians, it depends on the problems.

@gdalle
Copy link

gdalle commented Jun 5, 2024

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
Copy link
Member

ocots commented Jun 5, 2024

We also have benchmarks with the problems from OptimizationProblems.jl

@amontoison is there a list of the problems descriptions?

@amontoison
Copy link
Contributor

amontoison commented Jun 5, 2024

@jbcaillau
Copy link
Member

👍🏽 @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
Copy link
Member Author

PierreMartinon commented Jun 6, 2024

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
Copy link

gdalle commented Jun 6, 2024

Check out the performance benchmarks too!

JuliaSmoothOptimizers/ADNLPModels.jl#241

@jbcaillau
Copy link
Member

jbcaillau commented Jun 11, 2024

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

IMG_3211

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
Copy link
Contributor

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
Copy link

gdalle commented Jun 11, 2024

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 JuliaSmoothOptimizers/ADNLPModels.jl#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
Copy link
Contributor

Major speed-up for Jacobian coloring :)

@jbcaillau
Copy link
Member

Major speed-up for Jacobian coloring :)

@amontoison overselling you 🤭

@PierreMartinon new release...?

@amontoison
Copy link
Contributor

I will release ADNLPModels.jl v0.8.1 in 30 minutes. 🚀

@PierreMartinon
Copy link
Member Author

PierreMartinon commented Jun 12, 2024

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
Copy link
Member

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

@amontoison
Copy link
Contributor

amontoison commented Jun 14, 2024

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
Copy link
Member Author

PierreMartinon commented Jun 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants