Type stability in ADNLPModels #352

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

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

Attached are my benchmark results...
grad_benchmarks.zip

…dels.jl into type_stab

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

src/ADNLPModels.jl

  args = []
  for field in fieldnames(ADNLPModels.ADModelBackend)
    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

Co-authored-by: Alexis Montoison <[email protected]>

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

  args = []
  for field in fieldnames(ADNLPModels.ADModelBackend)
    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

…dels.jl into type_stab

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.

…dels.jl into type_stab

Co-authored-by: Tangi Migot <[email protected]>

MaxenceGollier and others added 6 commits May 24, 2025 10:43

improve type stability in ADModels

7b54102

add type stability unit tests

d8e5f0c

Merge branch 'JuliaSmoothOptimizers:main' into type_stab

f23d684

fix set_adbackend

89b1560

Merge branch 'JuliaSmoothOptimizers:main' into type_stab

598c644

Merge branch 'type_stab' of https://github.com/MaxenceGollier/ADNLPMo…

5ba6c83

…dels.jl into type_stab

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

amontoison reviewed Jun 5, 2025

View reviewed changes

Apply suggestions for type stability

c221595

Co-authored-by: Alexis Montoison <[email protected]>

amontoison reviewed Jun 6, 2025

View reviewed changes

src/ADNLPModels.jl Outdated Show resolved Hide resolved

Update src/ADNLPModels.jl

5dc16f4

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

update set_adbackend function and add constructors

38728c0

Merge branch 'type_stab' of https://github.com/MaxenceGollier/ADNLPMo…

512185c

…dels.jl into type_stab

resolve type stabs for NLS

4e5a9df

revert changes to runtests

27dc8d9

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

Open

Merge branch 'JuliaSmoothOptimizers:main' into type_stab

5913c53

MaxenceGollier and others added 5 commits August 6, 2025 12:53

revert type stability changes

8b98f54

Merge branch 'type_stab' of https://github.com/MaxenceGollier/ADNLPMo…

aae01d2

…dels.jl into type_stab

Update type stability tests

a9f2809

Co-authored-by: Tangi Migot <[email protected]>

specify JET version

81a8de9

ignore ForwardDiff for type stability tests

6fb99be

Type stability in ADNLPModels #352

Are you sure you want to change the base?

Type stability in ADNLPModels #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!

Uh oh!

MaxenceGollier Jun 26, 2025 •

edited

Loading