Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Aug 10, 2021

I've just refactored ECOS with MatrixOfConstraints (jump-dev/ECOS.jl#121).
There is something tricky that will also be problematic for other similar solvers.
When you implement copy_to!(dest::Optimizer, src::OptimizerCache), src contains exactly the data you need to call the solver directly. However, you cannot call it yet, you need to wait for optimize! to be called.
So you need to store a cache src in dest. You need to store a copy since src could be modified or its data could be free'd before optimize! is called.
This create a lot of edge cases that kind of defeat the purpose of #1381 to remove the need for the optimizer to have a cache as this is handled by the CachingOptimizer.
This PR defines a one-shot optimization function that both gives src and optimizes.
For one-shot solver such as most conic solvers, this will considerably simplify the MOI wrapper. It would prevent using these wrapper with copy_to and optimize! separately without any other MOI layer but it does not matter much.
99% of the use cases would be using it with JuMP or with MOI.instantiate with bridges which would add a CachingOptimizer layer anyone.
So there shouldn't be much difference for the user. It would just simplify a lot the writing of these solvers. No need to complicate it to cover the case where src is modified between copy and optimizer while we know this will almost only be used by the CachingOptimizer's optimize! method which just calls optimize directly after the copy.

The docstring might be clarified to insist that this is a very particular use case. We could even move the function in MOI.Utilities if that would clarify it.

@blegat blegat requested a review from odow August 10, 2021 19:47
@odow
Copy link
Member

odow commented Aug 10, 2021

It would prevent using these wrapper with copy_to and optimize! separately without any other MOI layer but it does not matter much

Doesn't this seem like a big deal?

If modifying the OptimizerCache is going to cause invalidations, why can't we just disallow modifications of OptimizerCache?

This seems like an abstraction problem to have to add to MOI if it is only used in CachingOptimizer and requires a specific set of circumstances.

Couldn't ECOS just hold a pointer to src, and then in optimize! to the necessary transformations? copy_to doesn't actually have to modify anything in the ECOS model.

@blegat
Copy link
Member Author

blegat commented Aug 11, 2021

Doesn't this seem like a big deal?

Yes, no big deal at all. That's my point, we don't lose much.

If modifying the OptimizerCache is going to cause invalidations, why can't we just disallow modifications of OptimizerCache?
Couldn't ECOS just hold a pointer to src, and then in optimize! to the necessary transformations? copy_to doesn't actually have to modify anything in the ECOS model.

MatrixOfConstraints does not allow modification. However, one could do:

MOI.copy_to(optimizer, model)
MOI.empty!(model)
MOI.optimize!(optimizer)

But we don't want to complicate everything for this use case since the only use case we care about it the use when optimize! is called in CachingOptimizer

@odow
Copy link
Member

odow commented Aug 11, 2021

Okay, I'd approve merging this for now, but we'd need to see ECOS et al working before tagging MOI 0.10. Or at least, say in the docstring that this is experimental, and solvers relying on it should be careful. I guess it's okay to break some things post MOI 0.10 so long as we are the only ones using it.

@blegat
Copy link
Member Author

blegat commented Aug 11, 2021

Sounds good, good to merge with the last commit ?

@odow odow merged commit 6b3d549 into master Aug 11, 2021
@odow odow deleted the bl/copy_opt branch August 11, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants