Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C API] Add getters for logfile and output #454

Closed
wants to merge 1 commit into from

Conversation

odow
Copy link
Collaborator

@odow odow commented Feb 9, 2021

I realized I needed this for the Julia interface to be able to restore the output after setting to NULL. (Unless @matbesancon knows how to pass stdout from Julia to C?)

But perhaps a better way is to remove the setHighsOutput and setHighsLogfile, rename Highs_runQuiet to Highs_setQuiet, and introduce Highs_unsetQuiet

int Highs_setQuiet(void* highs) {
  int return_status = (int)((Highs*)highs)->setHighsLogfile(NULL);
  if (return_status) return return_status;
  return (int)((Highs*)highs)->setHighsOutput(NULL);
}

int Highs_unsetQuiet(void* highs) {
  int return_status = (int)((Highs*)highs)->setHighsLogfile(stdout);
  if (return_status) return return_status;
  return (int)((Highs*)highs)->setHighsOutput(stdout);
}

Thoughts?

@matbesancon
Copy link
Contributor

Unless @matbesancon knows how to pass stdout from Julia to C?

One could technically pass Base.stdout as a void pointer option to the C API, and then have it cast as a FILE * on the solver side. Although this could offer flexible logging, it feels non-robust. I would go with set and unset quiet, and possibly exposing setHighsLogfile but not necessary from the Julia wrapper perspective

@odow
Copy link
Collaborator Author

odow commented Feb 9, 2021

One could technically pass Base.stdout as a void pointer option to the C API, and then have it cast as a FILE * on the solver side.

I tried innumerable variations of this, including passing cglobal(:jl_uv_stdout) and Core.io_pointer(Core.stdout) without success. The issue is that Base.stdout is a nicely packaged struct with lots of other stuff going on.

@matbesancon
Copy link
Contributor

The only thing I found was https://docs.julialang.org/en/v1/base/io-network/#Base.redirect_stdout-Tuple{Function,Any} with which one could always let HiGHS run on stdout and then pipe it to stdnull if non-verbose. That would be a bit wasteful since HiGHS would still be producing the stream of logs for nothing

@galabovaa
Copy link
Contributor

But perhaps a better way is to remove the setHighsOutput and setHighsLogfile, rename Highs_runQuiet to Highs_setQuiet, and introduce Highs_unsetQuiet

I think this makes it more clear. What do you think @jajhall?

@jajhall
Copy link
Member

jajhall commented Feb 9, 2021

But perhaps a better way is to remove the setHighsOutput and setHighsLogfile, rename Highs_runQuiet to Highs_setQuiet, and introduce Highs_unsetQuiet

I think this makes it more clear. What do you think @jajhall?

Output and logging in HiGHS are clearly over-engineered from a user perspective. The "idea" was to allow output and logging to go to different files, if a user so wished.
In
https://www.gurobi.com/documentation/9.1/refman/outputflag.html#parameter:OutputFlag
Gurobi just has one logging stream that, by default, goes to the screen (and to a file when run from the command line or interactive shell),
OutputFlag is the on-off logging flag that users want to be able to set. Gurobi also allows finer control:

  • Logging to the screen can be switched off with LogToConsole
  • Logging to the file can be redirected by setting the string LogFile to be a file name, or switched off by setting it to ""
    I suggest that we duplicate Gurobi's functionality. It's not hard to implement, as all HiGHS output/logging is handled via calls to two methods. These methods are soon unified.
    For development work, there should be an additional "advanced" option to make the output verbose, but not the three levels - controlled via a bitmap - that HiGHS has at present: just a "verbose" parameter to the logging method that, when set, will mean that the output is displayed if the verbose option is set. Let's bring @lgottwald into the discussion, too

All this would mean that we get rid of Highs_runQuiet, setHighsOutput and setHighsLogfile, and don't introduce Highs_unsetQuiet, whose behaviour will lead to queries from users. Does it revert to default, or previous settings of output options?

@lgottwald
Copy link
Contributor

I agree that setting the output could be a bit simpler. When I tried to do it my first try was to the the message_level option to 0 and was confused that HiGHS still produced output. It would be nice to have a single way of disabling enabling output and remove the output/log file split. Also it would be nice if this option and some other common options like setting limits, solver type to use, would be exposed via an API function in addition to the parameter system. Then there is type safety and the IDE/compiler can give hints about proper usage.

Something else that I noticed about the API is that there are unnecessary copies involved sometimes. I changed the API for passModel to receive the model by value, and move assign it to the internal model. This allows me to solve a modified version of a problem by creating a copy, doing the modification and passing it into passModel with std::move to avoid creating another copy. The passing by value is best practice in c++11 when an internal copy of an object is created to leave the choice to the user/caller about wether they still need their copy.

@jajhall
Copy link
Member

jajhall commented Feb 9, 2021

Also it would be nice if this option and some other common options like setting limits, solver type to use, would be exposed via an API function in addition to the parameter system. Then there is type safety and the IDE/compiler can give hints about proper usage.

I don't know what this means. At the moment, options set with calls to setHighsOptionValue have the value checked for being of the right type and within the permitted limits

Something else that I noticed about the API is that there are unnecessary copies involved sometimes. I changed the API for passModel to receive the model by value, and move assign it to the internal model. This allows me to solve a modified version of a problem by creating a copy, doing the modification and passing it into passModel with std::move to avoid creating another copy. The passing by value is best practice in c++11 when an internal copy of an object is created to leave the choice to the user/caller about whether they still need their copy.

Again, I don't have the C++ knowledge to comment on this

Copy link
Member

@jajhall jajhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense until we revise the output/logging in HiGHS

@odow
Copy link
Collaborator Author

odow commented Feb 13, 2021

Makes sense until we revise the output/logging in HiGHS

You don't want me to change this to unsetQuiet?

@jajhall
Copy link
Member

jajhall commented Feb 13, 2021

Makes sense until we revise the output/logging in HiGHS

You don't want me to change this to unsetQuiet?

No. I chatted to @galabovaa today, and I've agreed to change the logging/output top the Gurobi standard next week, so it can wait.

@odow
Copy link
Collaborator Author

odow commented Jun 21, 2021

Closing because this is now implemented another way.

@odow odow closed this Jun 21, 2021
@odow odow deleted the od/output branch June 21, 2021 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants