Not-interface members (and general considerations about CRTP)

[mapgccv]

Let's consider the accessors: https://github.com/ampl/mp/blob/f3da5f8931e1270e82f8ca80b669b58bd0cae5f6/solvers/gurobidirect/gurobibackend.h#L123 and following.

I do realize that using this Curiously Recurring Template Pattern we do resolve these methods dynamically, but these methods are actually only declared here, there is no trace of them in the backend class nor in the solver class, as far as i can see. Is this a design choice? 1) If it is, wouldn't this make more difficult to understand what a person has to implement in order to implement a solver? 2) And this leads me to yet another thing: is it really necessary to use this CRTP for solver drivers?

My personal opinion is that it makes the code much harder to follow (and more scary to new users) than using virtual functions, is there a reason besides speed (which I think is not really relevant here, as for big enough models most time will be spent in the solver itself) to have the backend infrastructure implemented like this? What do you think, @glebbelov?

[glebbelov]

This is surely hard to understand and would require a good explanation in what would be a successor paper, like "Hooking your solver to AMPL MP". The accessors are not an issue. Where CRTP seems crucial in the Backend are the ACCEPT_CONSTRAINT. Without CRTP, assuming that Converter only receives a reference to Backend and not the actual implementation class, the Backend would have to declare virtual AcceptanceLevel(const ConstrType* ) for every ConstrType.

Which is ok when the set of constraints is fixed. But assume user adds a new solver and defines some new constraint types. How could we then inform the Converter? Well, ok, the Converter would need to use the new constraints anyway, so it could receive a reference to an extended Backend instead.

Or, if we still make the final implementation class (e.g., GurobiBackend) a template parameter for Converter, then we can keep the AcceptanceLevel() functions via CRTP. And make the normal accessors virtual for convenience, although this is not necessary then.

Actually for most accessors, an empty function is defined in order to simplify unit tests. Making them all abstract would need every TestBackend to define them. The same can be done with CRTP by just omitting compulsory methods such as GetBackendName() from Backend. Missing abstract methods simplify error messages a bit, I agree, although for missing CRTP methods like PrimalSolution() the message is understandable as well. What do you think, @mapgccv and @fdabrandao ?

[fdabrandao]

I am not a fan of CRTP, but I see how it can be useful for ACCEPT_CONSTRAINT. @glebbelov, could you please prepare a short document explaining what anyone would need to know in order to implement a solver driver (with basic functionaries) using MP as it is right now? With that it should be easier to see how complicated this would look for a new user and see what may need to be changed to make it easier to understand.

[glebbelov]

First, this is not urgent I think because we should 1st probably get the interface working and then refactor. However, it may help reduce the refactoring if we do it now.

Virtual functions are certainly better supported in C++ than static dispatch in terms of warnings/error messages etc. There are also the guiding keywords 'override' and 'final'. It's clear for everyone that virtual functions have to be redefined in the final Backend. For the non-critial accessors, it's no issue to use virtual functions, happy to do it. Another visual benefit of that would be to remove the corresp. MP_DISPATCH macros. (Or, keep them for a unified style? Otherwise, we mix CRTP and virtual potentially with confusion).

By the way, this leads us to the idea of separating modeling and solving logic in separate base classes, like BackendSolving(MIP)Logic and BackendModelingLogic. The SolvingLogic can be reused for existing MP interfaces which don't use FlatConverter (at the moment, every existing MP backend implements its own sequence of major solving steps although it's very similar). They can be placed in the main include/mp folder.

In terms of documentation, last year's "Hooking Your Solver to AMPL MP" document (see on Overleaf), Sec 3.1, specifies what needs to be defined in a final Backend. Although the list is outdated, it's pretty compact. To prepare the user for CRTP, we could say for both Converter and Backend:

Both Converter and Backend rely on static dispatch via CRTP [....]. Thus, instead of virtual functions, a base class should call a derived class' method using MP(CONST)DISPATCH macros, for example: obj_value = MP_DISPATCH( CurrentPoolObjectiveValue() ); For the implementation Backend class, CRTP means that the implemented methods, such as accessors, cannot use the override or final keywords: double ObjectiveValue() const;

Otherwise, if we change accessors to virtual, we would finish differently:

However, most solver-logic override methods in the Backend are implemented as virtual functions for convenience, so the final Backend would declare them as follows: double ObjectiveValue() const override final;

The document is now 'all-in-one' and should ideally separate background information from how-to's.

Yet another question: do we need a cmdline option to list all supported suffixes?

[fdabrandao]

Yeah, this is not urgent at all. I just wanted to point out that it would be better to see how would the documentation look like before making any changes that could not be necessary. Looking at section 3.1 of that document, it looks fairly simple, and I think the only additional thing to make this easy for anyone to start developing new drivers could be a kind of template/example driver (that would compile but not do anything, or eventually, just show some model information) so that users could start from it and just fill the blanks. This should make clear the set of methods (and corresponding declarations) that would need to be implemented and in that case having CRTP or not should not impact the user that much since there would not be a need to dig into parent classes. @mapgccv, wouldn't a template driver help to address your point 1.?

[glebbelov]

Thinking of this discussion, I tried to reformat gurobibackend.h (and a little bit cplexbackend.h) to highlight these issues. gurobibackend.h now comments "PART 1: Accessor API" and "PART 2: internal details". cplexbackend.h is currently an example of a rather incomplete backend. Later, something like that can be left as a template, but I am convinced gurobi is good for it now.

[glebbelov]

Changed most accessors to virtual in 92cfcc6