Remove dirty flag

[blegat]

The dirty flag added in #2709 removes some flexibility for the MOI backend as it assumes that setting an attributes always modifies the solution. However, for DiffOpt, you can set attributes that are the gradient and ask for forward and backward differentiation. And you can set a different gradient and do differentiation again, without the need to reoptimize.
I'm tempted to say that we should revert #2709 and rely on MOI model to implement MOI.TerminationStatus consistently. The consistency being check by MOI tests as we've done with the rest of the API.
Another way to make this work is to add a function in MOI does_setting_this_attributes_requires_a_resolve so that DiffOpt can implement a method that returns false.

[matbesancon]

I could definitely see other custom attributes with similar behavior, adding information on top of the solution without modifying the existing model

[odow]

Can you provide an example where this is a deal-breaker?

There are also other work-arounds, like calling MOI directly to bypass the JuMP flag. You could also use DiffOpt in direct mode.

I'm not in favor of removing this flag. Setting it resolves a serious pain-point for users: it's too easy for them to end up with incorrect solutions otherwise.

[blegat]

Can you provide an example where this is a deal-breaker?

With DiffOpt, you solve the optimize once and then you can provide derivatives by acting as a matrix product (or adjoint matrix product) and you might give product of several vector but you only want to solve once. Even if you want only one product,, you don't want to force the user to have to specify the gradient before calling optimize.

There are also other work-arounds, like calling MOI directly to bypass the JuMP flag. You could also use DiffOpt in direct mode.

Having to use these workarounds for DiffOpt and all other use cases wouldn't be ideal.

I'm not in favor of removing this flag. Setting it resolves a serious pain-point for users: it's too easy for them to end up with incorrect solutions otherwise.

I agree that it's a pain point. I also agree that we may not want to handle it at the level of all MOI wrapper as you argue in #2709 (comment) in case the user want to experience the interaction with the solver in DIRECT mode.
It seems however that having this solver-dependent behavior already starts being confusing whenever you are using a CachingOptimizer. Since the CachingOptimizer transparently handles emptying and copying to the solver, it would make sense to enforce this consistent behavior at the level of the CachingOptimizer. This would also ensure that we keep a consistent behavior between JuMP and MOI.
We can probably add that dirty flag in CachingOptimizer and add invalidates_results(::AnyAttribute) = true in MOI that specifies whether setting that attribute invalidates any is_set_by_optimize attribute.

[odow]

I meant in code. What is the syntax? What are the alternatives?

all other use cases

What other use-cases are there?

[blegat]

Here are codes:
https://github.com/jump-dev/DiffOpt.jl/blob/d60c53f516b51ade544227cef2fcf28104a97493/docs/src/examples/sensitivity-analysis-svm.jl#L140-L165
https://github.com/jump-dev/DiffOpt.jl/blob/d60c53f516b51ade544227cef2fcf28104a97493/docs/src/chainrules_unit.md#forward-differentiation

Another example could be feasibility interface. You set constraint attributes that give tolerance and which norm to use, then you can ask whether the solution is feasible. Then you can change the tolerance and ask again.

Another example are conflicts. Suppose you could give attributes to parametrize conflict resolution. Setting these attributes would not require calling optimize! again, it would require calling compute_conflict! again.

[odow]

DiffOpt already has a JuMP interface, so why can't it just provide different helper methods for setting ForwardInConstraint?

for Xi in 1:N
    dy[Xi] = 1.0  # set
    MOI.set(
        model,
        DiffOpt.ForwardInConstraint(),
        cons,
        MOI.Utilities.vectorize(dy .* b),
    )
    DiffOpt.forward(model)
    dw = MOI.get.(model, DiffOpt.ForwardOutVariablePrimal(), w)
    db = MOI.get(model, DiffOpt.ForwardOutVariablePrimal(), b)
    push!(∇, norm(dw) + norm(db))
    dy[Xi] = 0.0  # reset the change made above
end

becomes

function DiffOpt.set_forward_value(model, cons, dy, b)
    MOI.set(
        backend(model),
        DiffOpt.ForwardInConstraint(),
        index.(cons),
        MOI.Utilities.vectorize(dy .* index.(b)),
    )
    return
end

function DiffOpt.forward_value(model, x)
    return MOI.get(model, DiffOpt.ForwardOutVariablePrimal(), x)
end


for Xi in 1:N
    dy[Xi] = 1.0  # set
    set_forward_value(model, cons, dy, b)
    DiffOpt.forward(model)
    dw = forward_value(model, w)
    db = forward_value(model, b)
    push!(∇, norm(dw) + norm(db))
    dy[Xi] = 0.0  # reset the change made above
end

Suppose you could give attributes to parametrize conflict resolution

This is going to be solver-dependent, so the user should use direct mode. Then we wouldn't need to change this.

[joaquimg]

Only using direct mode would prevent adding a layer on top of DiffOpt.
Same for DiffOpt.set_forward_value that would not play nice with other layers.
I am in favor of keeping track of the attributes that invalidate a solution.

You would say that users still have to call DiffOpt.forward(model) that would not play nice with other layers.
This is something I would like to change in DiffOpt. I think forward/backward results could be deleted in DiffOpt every time the user set input data, and in the first get, forward/backward are called internally (lazily), the results are computed and returned.
This lazy implementation was done in Xpress: jump-dev/Xpress.jl#143

[odow]

that would not play nice with other layers.

This is a JuMP only solution. It doesn't matter what the layers are.

All we need to do is skip these flags:

JuMP.jl/src/JuMP.jl

Lines 1273 to 1303 in f5a7a85

	function MOI.set(m::Model, attr::MOI.AbstractOptimizerAttribute, value)
	m.is_model_dirty = true
	return MOI.set(backend(m), attr, value)
	end
	function MOI.set(m::Model, attr::MOI.AbstractModelAttribute, value)
	m.is_model_dirty = true
	return MOI.set(backend(m), attr, value)
	end
	function MOI.set(
	model::Model,
	attr::MOI.AbstractVariableAttribute,
	v::VariableRef,
	value,
	)
	check_belongs_to_model(v, model)
	model.is_model_dirty = true
	return MOI.set(backend(model), attr, index(v), value)
	end
	function MOI.set(
	model::Model,
	attr::MOI.AbstractConstraintAttribute,
	cr::ConstraintRef,
	value,
	)
	check_belongs_to_model(cr, model)
	model.is_model_dirty = true
	return MOI.set(backend(model), attr, index(cr), value)
	end

The easiest solution is for DiffOpt to implement a different function to provide a nicer interface at the JuMP level. This doesn't affect users of MathOptInterface who don't use JuMP.

[odow]

This was fixed in DiffOpt: jump-dev/DiffOpt.jl#154 -- although not the way I suggested above.

I'll re-iterate that I don't think we need to change JuMP; this can be solved by other packages.

[odow]

So can this be closed?

[odow]

The suggestion from today's call was to consider moving this to MOI. The full list of options are:

1. Force every solver to have the same behavior
   • Pros: every solver would be the same
   • Cons: every solver carrying a modification flag, and some use-cases that use the ability of a solver to modify things without invalidating the solution (e.g., DiffOpt) would break. It's also a lot of work and potentially hard to test.
2. Add this flag to the caching optimizer only
   • Pros: The real problem isn't with solvers returning junk, it's with caching optimizers resetting their internal solvers when the inner solver doesn't support incremental modification. This gets at the root cause and allows people to use solvers in direct mode if they choose.
   • Cons: It's a breaking change. MOI and the CachingOptimizer don't have JuMP's OptimizeNotCalled error.
3. Keep the flag in JuMP, but relax the situations in which it is set
   • Pros: Non-breaking. Few changes. More things work as expected.
   • Cons: some situations might still lead to differences between solvers, e.g., a solver which supports VariablePrimalStart but doesn't implement MOI.set for it so they can only be added in a copy_to or two-argument optimize call (SCS and ECOS will probably be in this boat).
4. Keep as-is
   • Pros: no changes
   • Cons: setting dirty on any possible change is quite coarse.

For 3 in particular, we discussed how there is no good way to know which attributes may invalidate a solution in any solver. It's a question of "does there exist a solver for which setting this attribute may invalidate the solution?" The only way to answer this question is to be defensive and say "yes" to all attributes, which is the current implementation in JuMP.

We also discussed how JuMP extensions which want different behavior (e.g., DiffOpt) can overload the MOI.set(::Model methods to avoid the flag. So maybe another solution is

JuMP.may_invalidate_solution(::MOI.AnyAttribute) = true

and then DiffOpt can define JuMP.may_invalidate_solution(::DiffOpt.ForwardInConstraint) = false.
But this would only be for new attributes, and all attributes defined in MOI and would invalidate solutions.

[blegat]

I'm in favor of 1.

some use-cases that use the ability of a solver to modify things without invalidating the solution (e.g., DiffOpt) would break

Why does DiffOpt require this ability ? This is perfectly compatible with DiffOpt because of the following.
The new rule would be: Modification that may invalidate something should always invalidate something.
So for optimizers, changing a bound should always invalidate the solution and their termination status should become MOI.OPTIMIZE_NOT_CALLED.
And this applies also for other calls than optimize!. For instance, in DiffOpt, if you set BackwardInVariablePrimal then call backward then set BackwardInVariablePrimal, it invalidates BackwardOutConstraint. However, setting BackwardInVariablePrimal won't invalidate the solution of the inner optimizer as the set will never reach the inner optimizer as it is handled by the DiffOpt layer.

Solvers that want to still allow user to be able to modify-then-query would have an optimizer attribute to allow that but it won't be the default. As the querying after modifying is ambiguous and error-prone it seems reasonable to throw an error by default and require the user to explicitly set an option for it to work.

[odow]

I'm not in favor of 1. It's too much of a breaking change to introduce prior to 1.0. That's a lot of work I want to avoid.

Going with 1 means that we need a way of knowing which attributes invalidate and when. That's going to lead to complexity.

I'm in favor of 2 or 4.

Given that we have resolved the situation in diffopt, is there really a call to make changes in JuMP? The issue in PowerSimulations was a modify-then-query of getting and setting primal starts. That works for most solvers, but it isn't guaranteed to work for all.

The current implementation is simple code-wise, and also simple to understand for users: do not modify then query in any situation.

[odow]

@jd-lara says we should add a warning that the model has been modified, not just that the optimizer hasn't been called.

That involves modifying this function:

JuMP.jl/src/JuMP.jl

Lines 1237 to 1249 in b7a84e5

	elseif attr isa MOI.TerminationStatus
	if model.is_model_dirty && mode(model) != DIRECT
	return MOI.OPTIMIZE_NOT_CALLED
	end
	return MOI.get(backend(model), attr)
	elseif attr isa MOI.PrimalStatus || attr isa MOI.DualStatus
	if model.is_model_dirty && mode(model) != DIRECT
	return MOI.NO_SOLUTION
	end
	return MOI.get(backend(model), attr)
	else
	if model.is_model_dirty && mode(model) != DIRECT
	throw(OptimizeNotCalled())