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

highspy: unpythonic error handling #1906

Open
few opened this issue Sep 1, 2024 · 3 comments
Open

highspy: unpythonic error handling #1906

few opened this issue Sep 1, 2024 · 3 comments

Comments

@few
Copy link

few commented Sep 1, 2024

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.

@jajhall
Copy link
Sponsor Member

jajhall commented Sep 2, 2024

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.

@few
Copy link
Author

few commented Sep 2, 2024

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.

@jajhall
Copy link
Sponsor Member

jajhall commented Sep 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants