related issues that mention Gurobi and need updating once this one is resolved: issue926, issue972, issue1010, issue1076

related issues that mention CLP and need updating once this one is resolved: none found

other TODOs:
- update https://www.fast-downward.org/LPBuildInstructions~~
- search wiki for other mentions of Gurobi and CLP

This wasn't merged, but with the merging of issue1076 it is obsolete. I checked the pull request and found nothing that is still needed, so marking this as resolved.

I'm also marking the TODOs in the summary as done and removing issue1001 from the list of follow-up issues because the Gurobi part of it is done (AFAIK).

Yes, please. Once you merge, I'll rebase issue1076 and remove the traces of Gurobi and CLP there as well.

I see, thanks!

It looks like the Github Actions succeeded.
Shall I go ahead and merge this myself then?

In issue1076 we got rid of the preprocessor macros with an indirection. The class LPSolver now stores a pointer to an implementation of the solver interface and forwards calls there. If we compile without support for CPLEX, the class CplexSolverInterface is not present in the code and trying to instantiate an LPSolver with type CPLEX will exit with the appropriate error message. If you want to have a look, the implementation is in this pull request:

https://github.com/aibasel/downward/pull/151/files#diff-6f7a85121e4e465d643ec94b9704c2bc8883e092a74a1a4737f98c1b3a91bc77

PS: one of the "unused-parameter" pragmas is necessary due to *our own* code, i.e., the way we implement the "LP_METHOD" macro for compiles without LP support.

I don't know the new non-OSI interface code enough to know if it already takes care of this, but if we indeed want to get rid of the pragmas entirely (which would be nice), we need to change this part of the code.

I think there could be better implementation options with the more modern C++ standards we now require. For example, perhaps something with a similar effect to our LP_METHOD could be implemented using variadic templates and parameter packs, and perhaps instead of an #ifdef we could use a static if. It's generally desirable to reduce reliance on the preprocessor, but of course it's also a question of how convoluted this makes the code compared to the current quite simple solution.

To me it would make sense to address this as a separate issue if we want to address it. I'm not opening one now because it's not critical and I don't intend to work on it (at least not right now), and also I wouldn't mind including it in the "get rid of OSI" issue. But in principle I don't think it's really related.

I don't think our Github actions compile with Gurobi or CLP support, and if the support is ifdef'ed out, it can't cause the actions to fail.

To be clear, ignoring the keyword-macro diagnostic *is* needed in lp_internals.cc. The pragma and #define can only be removed in lp_solver.cc. 

I grepped the OSI headers for use of "register", and it's only used in 2 out of 74 headers; none that we include directly. The two headers are CoinHelperFunctions.hpp and CoinSearchTree.hpp. If I resolve all #includes by running the preprocessor on each header with clang -E, the word "register" occurs in 16 out of 74 headers. (It's important to grep with word boundaries as '\bregister\b' to avoid false positives. Note that using clang -E to run the preprocessor is an imperfect process because it will also resolve #ifdefs etc., and perhaps these are different in the context that we actually compile the code. But I wouldn't know them to be different in a way that would affect the results.)

On main, we have the following OSI includes:

lp_internals.cc:#include <OsiSolverInterface.hpp>
lp_internals.cc:#include <OsiClpSolverInterface.hpp>
lp_internals.cc:#include <OsiCpxSolverInterface.hpp>
lp_internals.cc:#include <OsiGrbSolverInterface.hpp>
lp_internals.cc:#include <OsiSpxSolverInterface.hpp>

lp_solver.cc:#include <OsiSolverInterface.hpp>

Of these five headers, the only two that (indirectly) use the register keyword are the CPLEX and Soplex headers (OsiCpxSolverInterface.hpp, OsiSpxSolverInterface.hpp), which explains why the pragma and #define are only needed in lp_internals.cc.

Perhaps we added them in both places just to be on the safe side? I don't know how we noticed the issue that led to the pragma and #define. If we were alerted to this via Github Actions and couldn't easily test locally, it takes a lot of annoying testing and waiting for Github to complete to figure out the exact set of source files that need the pragmas. So it would have made sense for us to include the pragma and #define in both files where they *might* be needed.

I'm fine with any solution for the pragmas. I don't care too much about what we do with them because I hope they won't be there at all in the next release.

I'm a bit surprised that
  #pragma clang diagnostic ignored "-Wkeyword-macro"
and
  #define register
wasn't necessary for clang 14, since we added this only recently when switching to c++-20. But if the Github actions are fine with it, then so am I. Maybe the problem was in the Gurobi or CLP interface.

Pull request updated, let's see what the Github Actions say.

> Some pragmas that are currently used in multiple LP solver files are only
> needed in a subset. I suggest *not* to remove from the files where they are
> currently not needed because this seems too fragile (dependency on indirect
> includes within Osi etc.).

Looking at the affected files a bit more, I changed my mind. There is a division between lp_solver.cc and lp_internals.cc here that makes some logical sense. I will try out removing all the unneeded pragmas.

Also, it's already the case that we use different pragmas in the two files, so keeping some unnecessary ones because they are necessary in the other file is not consistent with what we do in our current code.

src/search/lp/lp_solver.h:#pragma GCC diagnostic ignored "-Wunused-parameter"
=> Needed for configurations without LP support.

src/search/lp/lp_internals.cc:#pragma GCC diagnostic ignored "-Wdeprecated"
=> Not needed.

src/search/lp/lp_internals.cc:#pragma GCC diagnostic ignored "-Woverflow"
=> Not needed.

src/search/lp/lp_internals.cc:#pragma GCC diagnostic ignored "-Wsign-compare"
=> Not needed.

src/search/lp/lp_internals.cc:#pragma GCC diagnostic ignored "-Wunused-parameter"
=> Needed for configurations with LP support.

src/search/lp/lp_internals.cc:#pragma GCC diagnostic ignored "-Wmisleading-indentation"
=> Not needed.

src/search/lp/lp_internals.cc:#pragma clang diagnostic ignored "-Wkeyword-macro"
=> Needed for configurations with LP support.

src/search/lp/lp_internals.cc:#pragma GCC diagnostic ignored "-Wconstant-conversion"
=> Not needed. This one is weirdly only enabled for *clang*, not for GCC.

src/search/lp/lp_solver.cc:#pragma GCC diagnostic ignored "-Wunused-parameter"
=> Not needed.

src/search/lp/lp_solver.cc:#pragma clang diagnostic ignored "-Wkeyword-macro"
=> Not needed.

Regarding the pragmas:

I tested the release, debug and release_no_lp builds on Ubuntu 22.04 using the default (g++) compiler and the default clang++ version.

Most of the pragmas were not necessary, but some of them might be necessary on other compiler versions. If we want, we can try this out via our Github actions. What do you think?

Two oddities I found that I suggest we change for sure:

1) One g++ pragma is explicitly only added for g++ version 6 or higher. Since we're long past this as the minimum requirement, I suggest to remove the version number test.

2) One g++ pragma is only enabled when compiling with clang. Unless clang interprets g++ pragma, this makes no sense and should be removed. (It was clearly not necessary if the code currently compiles, and it's not necessary on my machine, too.)

Some pragmas that are currently used in multiple LP solver files are only needed in a subset. I suggest *not* to remove from the files where they are currently not needed because this seems too fragile (dependency on indirect includes within Osi etc.).

I suggest to try removing the other ones and see what the Github Actions say. What do you think?

I'll write a separate message on the details of what was and wasn't needed in my tests.

I wanted to get properly set up with CPLEX etc. anyway for some time, so I don't mind doing it in the context of this issue. I've now got Soplex and CPLEX to work (found a CPLEX 12.9 installer still lying around on one of my machines) and will now look into the pragmas a little bit.

I also have the "Resource currently unavailable" error and am in contact with IBM support about this. They are very responsive and working on it, but so far they could not fix it. I heard the same error from everyone I talked to, so the whole academic software catalog seems broken at the moment.

I don't think optimizing the pragma statements is worth holding up this issue for. It is unlikely that any of the statements can be removed, they do not affect anything else (warnings are only ignored during the OSI import), and we'll remove the whole file soon anyway. I also agree that we don't need experiments.

Thanks for the review, Florian!

I'll change the comment to CPLEX as suggested.

Regarding the pragmas, I think we don't normally compile with CLP and Gurobi support enabled, so I think it's doubtful that these previously triggered the warnings. But I'm trying it out anyway. Will take a while longer because this led me on a yak-shaving exercise where I now got stuck at the old problem of downloading CPLEX. I think I got to the final boss, the CPLEX download link, which then defeated me with "Resource currently unavailable; try again later". Anyway, I think I have a workaround of an existing installation somewhere, but it will need more time.

I suggest we don't run experiments for this, but let me know if you disagree.

Added TODO to update https://www.fast-downward.org/LPBuildInstructions (which discusses Gurobi and CLP) and the wiki more generally.

Pull request and commit message look fine to me. I left two minor comments but nothing that requires discussion from my side.

I created a pull request:

https://github.com/aibasel/downward/pull/154

Any volunteers to review? Perhaps Florian?

I explained what I did in the individual commit messages, in case you want to check that I looked at all necessary places.

Github Actions are still running.

A local tox run seems to have succeeded apart from the known problem related to clang-tidy-12.

Suggestion for commit message:

===========================================================================
[issue1085] Remove "CLP" and "GUROBI" options for LP solvers.

For the Gurobi LP solver, the implementation was never completed: the C++ code was there, but CMake code was missing. So this option could never be used.

CLP is the internal solver of OSI. We are moving towards eliminating our dependency of OSI and this is not supposed to be a high-performance solver and therefore a performance trap. This option was always untested, and we are not aware of any existing users.
===========================================================================

No objection from my side. I didn't expect you to have time for this so soon. Doing this before issue1076 is even better, in that case issue1076 will not leave any unimplemented cases.

Regarding the 1 hour: I meant 1 hour to get it working (I did this during the last sprint), not to get it in a mergeable state, I know this would take more time. The main blockers in the last 14 years were the licensing issue on the grid and that so few people were asking about this. (I remember one or two tried it in their local fork.) Anyway, none of this should hold up this issue.

> Fine with me. I just thought that it is more likely that we'll
> finish issue1076 before someone work on this one and then including
> it there would leave things in a more consistent state.

I want to work on it this myself (this week) if there are no objections to the removal.


If it could realistically be added in an hour, I think someone would have done it in the past 14 years or however long since we added OSI.

There is some effort to get it to work to the level that we require in our code (which includes multi-platform support where possible and having at least a little bit of an idea about performance).

Fine with me. I just thought that it is more likely that we'll finish issue1076 before someone work on this one and then including it there would leave things in a more consistent state. That issue (1076) changes Gurobi support from something that can be added in an hour to something more complicated to add, so I do see a qualitative difference there. But I agree from a user perspective it doesn't matter why it doesn't work and an completely working solution with an "if (false)" around it would be the same as no solution.

There also is an advantage for separating things in that issue1076 will be easier to close, because there we don't have to remove the failing cases from the switch statements, update the wiki, and so on.

issue1076 is the motivation, but this issue does not depend on issue1076 and would be desirable to do even if we decided against issue1076. (I am not saying this is likely, just to apply a test whether this is an independent change or not.) From the user perspective it makes more sense to me to communicate this as a separate change.

I don't see the CMake code as less critical than the C++ code, so this has been a documented but missing feature for a long time. I don't think issue1076 changes this; from a user's persepective, "it doesn't work for one reason" or "it doesn't work for two reasons" lead to the same result.

Maybe we want to do this as part of issue1076. Up until that change, the C++ code was actually correct for Gurobi (and CLP I think). The bug that you couldn't actually be used was only an issue with CMake: the Find-script for Gurobi was missing. After merging issue1076, adding a Find-script would no longer be enough, so the missing support is also in the C++ part of the code.

The LP solver options for CLP and Gurobi are untested and essentially unmaintained, but mentioned as prominently as the ones that work in our documentation. As far as I understand, for Gurobi we'd also need to do something on the CMake side to make it work.

I suggest we remove both solvers, i.e., get rid of the enum values for them and remove the existing traces of support from the code.

For Gurobi, I asked in the past if it might be worth keeping/actually making it work because it is sometimes mentioned as sometimes having great performance. We recently did some (albeit anecdotal) tests on this, and in my view they didn't bring up strong enough reasons to keep Gurobi support (issue1076, specifically msg10961).

Removing them would simplify the documentation, be less confusing to users whom we expect to use CPLEX and SoPlex, avoid a likely performance trap with CLP, and fix what I'd call a bug (documented Gurobi support doesn't work). It would also make it easier to move forward with removing the OSI layer.