Fix type instabilities in ADNLPModels.jl #352

MaxenceGollier · 2025-06-05T09:09:07Z

@amontoison @tmigot This is where I am so far.

Attached are my benchmark results...
grad_benchmarks.zip

amontoison · 2025-06-05T14:49:24Z

src/ADNLPModels.jl

    push!(args, if field in keys(kwargs) && typeof(kwargs[field]) <: ADBackend
      kwargs[field]
-    elseif field in keys(kwargs) && typeof(kwargs[field]) <: DataType
+    elseif field in keys(kwargs) && typeof(kwargs[field]) <: Union{DataType, UnionAll}


@MaxenceGollier Why do you need UnionAll?

Because the type ReverseDiffADGradient or ForwardDiffADGradient are now parametric types and it is no longer true that ReverseDiffADGradient <: DataType...

@tmigot Do you remember why you tested typeof(kwargs[field]) <: DataType ?
The condition typeof(kwargs[field]) <: Type is always true so I don't know why we are testing that.

Yup, it doesn't make sense as is. Maybe it should be kwargs[field] <: Type to make sure the constructor call next make sense.

Why not kwargs[field] <: ADBackend ?? This seems to work locally, we are really trying to see if we can create a backend just from kwargs[field] as per the next line...
It would also make more sense with the previous check where we check if kwargs[field] is already an ADBackend by using typeof...

src/nlp.jl

test/utils.jl

src/ADNLPModels.jl

amontoison · 2025-06-06T14:50:10Z

@tmigot Are you fine with the new set_adbackend ?

tmigot · 2025-06-06T17:38:12Z

@tmigot Are you fine with the new set_adbackend ?

I will check that today

tmigot · 2025-06-06T21:13:55Z

test/utils.jl

+  @test typeof(get_adbackend(newer_nlp).hessian_backend) <: ADNLPModels.ReverseDiffADHessian
+end
+
+function test_allocations(nlp)


Can you create an issue in NLPModelsTest.jl to add this in all models?

@MaxenceGollier can you put the link here once it's done, thanks!

src/ADNLPModels.jl

tmigot · 2025-06-07T00:13:17Z

src/ADNLPModels.jl

    push!(args, if field in keys(kwargs) && typeof(kwargs[field]) <: ADBackend
      kwargs[field]
-    elseif field in keys(kwargs) && typeof(kwargs[field]) <: DataType
+    elseif field in keys(kwargs) && typeof(kwargs[field]) <: Union{DataType, UnionAll}


Yup, it doesn't make sense as is. Maybe it should be kwargs[field] <: Type to make sure the constructor call next make sense.

src/ADNLPModels.jl

src/nlp.jl

tmigot · 2025-06-07T00:18:42Z

Why the tests are failing? Is it just because NLS grad! function uses other function behind and should not be tested or is there some typing issue in NLPModels?

Your benchmarks are indeed surprising @MaxenceGollier , what were the tests problems?

test/utils.jl

tmigot · 2025-06-07T00:20:39Z

@MaxenceGollier @amontoison I moved the PR from draft to review to run the package benchmark by curiosity

MaxenceGollier · 2025-06-07T09:27:29Z

Why the tests are failing? Is it just because NLS grad! function uses other function behind and should not be tested or is there some typing issue in NLPModels?

That's what I have been trying to figure out, some Hessian (with JET I mean) tests also seem to fail for some reason so there might be other unstabilities hidden in this repo...

what were the tests problems?

What do you mean ? as I said, I still have to figure things out, I have a lot of urgent work to do on something else next week, I will be available to work this out the week after that...

Can you create an issue in NLPModelsTest.jl to add this in all models?

Yes good idea, I found similar issues on QuadraticModels.jl... (still need to open an issue but I don't have much time currently)

MaxenceGollier · 2025-06-26T12:59:31Z

What do you think of this version of set_adbackend ? @amontoison @tmigot. I tried to implement all your suggestions

MaxenceGollier · 2025-06-26T14:27:33Z

I think tests keep failing due to inherent type instabilities in ForwardDiff.jl. See in ForwardDiff/docs/advanced.md :

If your input dimension is constant across calls, you should explicitly select a chunk size rather than relying on ForwardDiff's heuristic. There are two reasons for this. The first is that ForwardDiff's heuristic depends only on the input dimension, whereas in reality the optimal chunk size will also depend on the target function. The second is that ForwardDiff's heuristic is inherently type-unstable, which can cause the entire call to be type-unstable.

The remaining type instabilities should be solved by having a closer look to those of ForwardDiff.

src/forward.jl

amontoison · 2025-06-29T22:43:50Z

@tmigot
Can you take care of the review? I am still traveling this week and won't be able to have a look before 2 weeks at least.

tmigot

Thanks a lot @MaxenceGollier for the great work! I made some comments, but we are going in a good direction.

test/Project.toml

test/utils.jl

src/forward.jl

src/ADNLPModels.jl

tmigot · 2025-06-30T01:53:43Z

src/ADNLPModels.jl

+function ADNLPModel(nlp::ADNLPModel, new_adbackend::ADModelBackend)
+    return _set_adbackend(nlp, new_adbackend)
+end
+
+function ADNLPModel(nlp::ADNLPModel; kwargs...)
+    return _set_adbackend(nlp; kwargs...)
+end
+
+function ADNLSModel(nlp::ADNLSModel; kwargs...)
+    return _set_adbackend(nlp; kwargs...)
+end
+
+function ADNLSModel(nlp::ADNLSModel, new_adbackend::ADModelBackend)
+    return _set_adbackend(nlp, new_adbackend)
 end


Good idea, but it doesn't work with this

ADNLPModels.jl/src/ADNLPModels.jl

Line 35 in fd0d248

function ADNLPModel!(model::AbstractNLPModel; kwargs...)

.
Because, we are already building mixed models.

Do we really need the ADNLPModel(nlp::ADNLPModel; kwargs...) and ADNLSModel(nlp::ADNLSModel; kwargs...) ?

No perhaps we don't.
We need to remove the test_getter_setter test then which basically test these functions (these are causing the tests to fail on my branch).

I am not sure though if it is a good idea to remove the functions. Let me know what you think. I we do remove them, I will juste remove the test_getter_setter thing and tests should pass.

src/forward.jl

tmigot · 2025-06-30T01:58:23Z

I think tests keep failing due to inherent type instabilities in ForwardDiff.jl. See in ForwardDiff/docs/advanced.md :

If your input dimension is constant across calls, you should explicitly select a chunk size rather than relying on ForwardDiff's heuristic. There are two reasons for this. The first is that ForwardDiff's heuristic depends only on the input dimension, whereas in reality the optimal chunk size will also depend on the target function. The second is that ForwardDiff's heuristic is inherently type-unstable, which can cause the entire call to be type-unstable.

The remaining type instabilities should be solved by having a closer look to those of ForwardDiff.

I am unfortunately not too surprised by this. Thanks to your analysis we will improve our part of the code.
We should try to think of a way to still test that our code is good.

amontoison · 2025-07-22T05:33:19Z

bump

MaxenceGollier · 2025-08-06T10:48:10Z

I am unfortunately not too surprised by this. Thanks to your analysis we will improve our part of the code. We should try to think of a way to still test that our code is good.

I agree. Perhaps the best we can do is to call the report_opt macro but let it ignore all the issues from the ForwardDiff package.
If our code is stable with all other backends then we'd consider that our code is good enough.

MaxenceGollier · 2025-09-15T16:20:38Z

I think this is ready for another round of review @tmigot @amontoison.
FreeBSD fails because of timeout. And i don't know how to run the benchmarks in GitHub...

amontoison · 2025-10-16T05:36:49Z

@MaxenceGollier Can you rebase your branch and fix the CI tests?

MaxenceGollier · 2025-10-16T19:05:30Z

@amontoison FreeBSD fails because we should use JET@v0.10 if we use Julia 1.12 and @v0.9 for Julia < 1.12. Do you know how we could do this well ?

test/Project.toml

amontoison · 2025-10-16T19:12:11Z

@amontoison FreeBSD fails because we should use JET@v0.10 if we use Julia 1.12 and @v0.9 for Julia < 1.12. Do you know how we could do this well ?

I update the compat entry to support both JET 0.9, 0.10.

MaxenceGollier · 2025-10-16T21:03:12Z

Thank you @amontoison, I need to update the doc now.
I would like to see the benchmarks ran by GitHub before i do that how can i get them to run ?

amontoison · 2025-10-16T21:11:53Z

We need to open another PR to fix the benchmarks with Julia 1.12.
Once it is merged, we can rebase this branch on top of it.
I can't have a look before Sunday / next week if you don't do it before.

tmigot

Thanks @MaxenceGollier ! It looks good to me overall. Can you also remove the code related to set_backend in the documentation:
https://github.com/MaxenceGollier/ADNLPModels.jl/blob/type_stab/docs/src/backend.md
https://github.com/MaxenceGollier/ADNLPModels.jl/blob/type_stab/docs/src/sparse.md

test/Project.toml

amontoison · 2025-10-21T22:31:29Z

@tmigot We should be consistent for Maxence. I think a non-inplace set_backend is still relevant and it can return a new ADNLPModel.

tmigot · 2025-10-21T22:35:45Z

@tmigot We should be consistent for Maxence and don't give contrary comments. I think a non-inplace set_backend is still relevant and return a new ADNLPModel.

Do we plan to add the new set_adbackend in this PR?

amontoison · 2025-10-21T23:25:28Z

@tmigot We should be consistent for Maxence and don't give contrary comments. I think a non-inplace set_backend is still relevant and return a new ADNLPModel.

Do we plan to add the new set_adbackend in this PR?

We can just rename set_adbackend! as set_adbackend and specify that we return a new ADNLPModels where we "recycle" the backends + setup the new one instead of updating the current ADNLPModels. It should be quite minor modifications in the docstring and code.

What do you think?

tmigot · 2025-10-21T23:30:59Z

Sounds, good.

MaxenceGollier · 2025-10-22T02:45:12Z

Big GitHub noob question before moving on: where can i see the benchmark results posted ?

Shouldn't it be posted as a comment here ?

amontoison · 2025-10-22T04:23:44Z

A rebase was needed for sure, let's see if doing a PR from a fork is also an issue.
I fixed a few CI errors, reintroduced the set_adbackend and updated the related documentation.
When the PR will be ready, I will ask Maxence to get ownership of the commits.

MaxenceGollier changed the title ~~Type stab~~ Type stability in ADNLPModels Jun 5, 2025

amontoison reviewed Jun 5, 2025

View reviewed changes

amontoison reviewed Jun 6, 2025

View reviewed changes

src/ADNLPModels.jl Outdated Show resolved Hide resolved

tmigot requested changes Jun 7, 2025

View reviewed changes

tmigot reviewed Jun 7, 2025

View reviewed changes

test/utils.jl Outdated Show resolved Hide resolved

tmigot added run gradient benchmark run Jacobian product benchmark run Jacobian benchmark run Hessian benchmark run Hessian product benchmark labels Jun 7, 2025

tmigot marked this pull request as ready for review June 7, 2025 00:20

tmigot added the do not merge This is an experiment or work in progress label Jun 7, 2025

MaxenceGollier commented Jun 27, 2025

View reviewed changes

src/forward.jl Outdated Show resolved Hide resolved

amontoison requested a review from tmigot June 29, 2025 22:42

tmigot requested changes Jun 30, 2025

View reviewed changes

amontoison mentioned this pull request Jul 22, 2025

Disable gradient and Hessian backends for NLSModels #357

Merged

amontoison reviewed Oct 16, 2025

View reviewed changes

test/Project.toml Outdated Show resolved Hide resolved

amontoison reviewed Oct 16, 2025

View reviewed changes

test/Project.toml Outdated Show resolved Hide resolved

tmigot requested changes Oct 21, 2025

View reviewed changes

test/Project.toml Show resolved Hide resolved

amontoison force-pushed the type_stab branch from de12f4e to 4c7a31c Compare October 22, 2025 04:00

More type stability in ADNLPModels.jl

3958d46

amontoison force-pushed the type_stab branch from 4c7a31c to 3958d46 Compare October 22, 2025 04:01

amontoison removed run gradient benchmark run Jacobian product benchmark run Jacobian benchmark run Hessian benchmark run Hessian product benchmark labels Oct 22, 2025

amontoison added 2 commits October 21, 2025 23:17

[CI] Fix tests with buildkite

b567c8a

Update the documentation for set_backend

836900c

amontoison changed the title ~~Type stability in ADNLPModels~~ Fix type instabilities in ADNLPModels.jl Oct 22, 2025

amontoison added run gradient benchmark run Jacobian product benchmark run Jacobian benchmark run Hessian benchmark run Hessian product benchmark labels Oct 22, 2025

Fix type instabilities in ADNLPModels.jl #352

Are you sure you want to change the base?

Fix type instabilities in ADNLPModels.jl #352

Uh oh!

Conversation

MaxenceGollier commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxenceGollier Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amontoison commented Jun 6, 2025

Uh oh!

tmigot commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmigot commented Jun 7, 2025

Uh oh!

Uh oh!

tmigot commented Jun 7, 2025

Uh oh!

MaxenceGollier commented Jun 7, 2025

Uh oh!

MaxenceGollier commented Jun 26, 2025

Uh oh!

MaxenceGollier commented Jun 26, 2025

Uh oh!

Uh oh!

amontoison commented Jun 29, 2025

Uh oh!

tmigot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmigot commented Jun 30, 2025

Uh oh!

amontoison commented Jul 22, 2025

Uh oh!

MaxenceGollier commented Aug 6, 2025

Uh oh!

MaxenceGollier commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amontoison commented Oct 16, 2025

Uh oh!

MaxenceGollier commented Oct 16, 2025

Uh oh!

MaxenceGollier Jun 26, 2025 •

edited

Loading

MaxenceGollier commented Sep 15, 2025 •

edited

Loading

MaxenceGollier commented Oct 16, 2025 •

edited

Loading

amontoison commented Oct 21, 2025 •

edited

Loading

MaxenceGollier commented Oct 22, 2025 •

edited

Loading