HiGHS icon indicating copy to clipboard operation
HiGHS copied to clipboard

highspy: unpythonic error handling

Open few opened this issue 1 year ago • 3 comments

The following python program exits with status code 0.

import highspy
h = highspy.Highs()
h.readModel("does_not_exist.mps")
h.run()

and this output:

Running HiGHS 1.7.2 (git hash: 44dd10c75-dirty): Copyright (c) 2024 HiGHS under MIT licence terms
ERROR:   File does_not_exist.mps not found
Coefficient ranges:
Model   status      : Empty
HiGHS run time      :          0.00

Clearly, exit code 0 is wrong as this was not a successful run. What should have been done instead? For a simple script, this would work:

import highspy
h = highspy.Highs()
assert h.readModel("does_not_exist.mps") == highspy.HighsStatus.kOk
h.run()

But if this code is part of a larger program that shouldn't crash right away, using an assert is not an option. If, for example, one wants proper exceptions to be handled by the caller, one could start guessing what the problem might have been and raise an appropriate exception:

import highspy
h = highspy.Highs()
if h.readModel("does_not_exist.mps") != highspy.HighsStatus.kOk:
    raise FileNotFoundError("does_not_exist.mps")
h.run()

But then one has to add this boilerplate code around every highspy call and has to think about what the proper exception should be.

This problem is not limited to readModel, but (almost?) all highspy functions. For example, I couldn't even guess why addVar could not return Ok. I guess that would be some exceptional circumstance, which means it would be ok to raise an exception.

All in all, the current error handling in highspy feels like using a C API and not a python API. What's the solution? One way would be to define a HighsException and write python wrapper functions around every highs call, perform the error handling in the wrapper, and raise HighsException if necessary. But since there are people around here who are more knowledgable than me in writing python APIs, they might come up with better ideas.

EDIT: addVariable is already a wrapper that works like this and raises a generic Exception. There is a handful of other functions that work like this.

few avatar Sep 01 '24 05:09 few

For

import highspy h = highspy.Highs() h.readModel("does_not_exist.mps") h.run()

My view is that, since h.readModel returns HighsStatus.kError, this should be checked by the user. But maybe this is what you mean when you say that "highspy feels like using a C API and not a python API"

After being passed a file name that does not exist, the incumbent model in HiGHS remains empty. Hence no error occurs when h.run() is executed, so the return of HighsStatus.kOk is appropriate.

Note

  • There are very few HiGHS methods where internal errors can occur. h.run() is the main case.
  • Almost all situations where HiGHS returns HighsStatus.kError are due to errors in user data - such as the file name that does not exist.
  • All HiGHS methods that don't return data - where errors cannot occur - return a HighsStatus, even if it's vanishingly unlikely that an internal error could occur. In your example of addVar, it's possible for a memory allocation error to occur - not that HiGHS traps such an occurrence!

If Python users debug by observing exceptions rather than checking return codes, then there's a discussion to be had. If HiGHS raised an exception, I think that C++/C programmers would be unhappy. Of course, I've admitted that HiGHS generally doesn't trap memory allocation errors, but these are very unlikely since problems that don't fit into memory are generally far too big to solve. We have seen this once, and added memory allocation checking to presolve. We should add it to the model "addition" methods.

jajhall avatar Sep 02 '24 16:09 jajhall

For

import highspy h = highspy.Highs() h.readModel("does_not_exist.mps") h.run()

My view is that, since h.readModel returns HighsStatus.kError, this should be checked by the user. But maybe this is what you mean when you say that "highspy feels like using a C API and not a python API"

Exactly. I'd say h.readModel("does_not_exist.mps") is very similar to this code:

with open("does_not_exist.txt") as f:
    print(f.read())

which raises FileNotFoundError: [Errno 2] No such file or directory: 'does_not_exist.txt'. The current behaviour is very surprising for a python API. For C/C++-APIs people are used to APIs returning error codes as C has no exceptions and in C++ they are avoided often enough.

If HiGHS raised an exception, I think that C++/C programmers would be unhappy.

I don't argue for the C++ code of highs to throw exceptions. It can be done solely in the python wrapper as already happens for some functions like addVariable. It should throw a more specific exception (at least a generic HighsException) to distinguish different sources of exceptions though.

few avatar Sep 02 '24 18:09 few

Thanks. Useful dialogue. Team HiGHS can do little in Python, so it's really up to the community to propose a better way of handling exceptions

jajhall avatar Sep 02 '24 18:09 jajhall