[mlir][python] Include diagnostic messages in exceptions

[rkayaith]

Currently diagnostics from parsing, running pipelines, op verification etc. end up on stderr. These can instead be captured using the diagnostic engine bindings, and included in the python exception message. See this (and similar) TODOs:

llvm-project/mlir/lib/Bindings/Python/IRCore.cpp

Lines 2592 to 2593 in 985f7ff

	// TODO: Rework error reporting once diagnostic engine is exposed
	// in C API.

[rkayaith]

Adding a temporary handler in functions to capture diagnostics is pretty straightforward, but that prevents the caller from using their own handler to report messages, e.g. they may be able to use the diagnostic location to include more context. Not sure how to handle that (other than plumbing a dont_capture_errors flag everywhere, which doesn't seem great).

[llvmbot]

@llvm/issue-subscribers-mlir-python

[ftynse]

A potential approach could be to check if there is a handler already set. If so, just let the errors flow there and raise an exception with the default message. Otherwise, add a temporary handler to capture the diagnostic in a string and construct the exception form that string.

Alternatively, we may be able to temporarily replace the handle with our own, intercept the diagnostic to construct the string, and then send diagnostic further to the original handler. The C++ API for handlers also allows us to chain handlers. The latest-added handler can look at the message and return failure() to indicate that it didn't process it fully, so the other handlers can try. Don't remember offhand if this is exposed to the C API.

[rkayaith]

A potential approach could be to check if there is a handler already set. If so, just let the errors flow there and raise an exception with the default message. Otherwise, add a temporary handler to capture the diagnostic in a string and construct the exception form that string.

We wouldn't know whether the existing handler will actually handle the diagnostic though, or how well it handles it (e.g. it could just be a catch-all that logs unhandled diagnostics instead of letting them go to stderr, in which case the temporary handler would be better off handling things). Also currently there's no C++ api for looking at the registered handlers.

I'm thinking the simplest thing would be to just attach the diagnostic information to the raised error, that way the caller can inspect them if they want to re-raise with a different message. Something like

try:
  ...
except ErrorWithDiagnostics as e:
  # [(loc("foo"), "error_message1", [])
  #  (loc("bar"), "error_message2", [(loc("baz"), "note")])]
  print(e.error_diagnostics)

But I don't know if custom exceptions are supported in pybind, from an initial look it seems like it'd require some hacks.

[ftynse]

We can add functionality in C++, too... In my opinion, if the user has registered a handler, we should assume they know what they want and it's their responsibility to handle the diagnostics that come their way so we shouldn't interfere with that.

Raising a custom exception could work, but I wouldn't want the user to have to connect that exception to their diagnostic handling if they have already set up the handler. Their handler can also receive the diagnostic.

[rkayaith]

I have an initial draft of the custom exception approach here (for now just applied to one function, but once there's agreement on the approach I'll apply it everywhere): https://reviews.llvm.org/D143869

And here's how I would like to use this downstream:

• when I initially create an ir.Context, set up a catch-all handler that handles all diagnostics by logging them, or using warnings.warn. I use the python bindings to provide a python interface to my compiler, so I don't want any diagnostics going directly to stderr.
• In some cases I might wrap specific MLIR functions with try / except to provide better error messages, but for the most part the default message would be good enough (today I have to wrap every call that might emit error diagnostics)

So for my use case I would really prefer that the diagnostics associated with an exception don't also end up in handlers. If I'm fine with the default error message, the diagnostics will end up in the catch-all handler and be logged unnecessarily. And if I'm wrapping specific calls, I already have access to the diagnostics through the custom exception, so there's no need for them to go through handlers.

In my opinion, if the user has registered a handler, we should assume they know what they want and it's their responsibility to handle the diagnostics that come their way so we shouldn't interfere with that.

In my case I'm not sure how the "catch-all" handler would be able to tell that a diagnostic has already been reported through an exception.

Raising a custom exception could work, but I wouldn't want the user to have to connect that exception to their diagnostic handling if they have already set up the handler. Their handler can also receive the diagnostic.

Maybe I'm over-indexing on my own use case, but I'm not sure I see where it'd be preferable to consume errors through handlers over an exception. The latter seems more natural in python.

[ftynse]

Not all diagnostics are errors and should result in exceptions. We have warnings, which may or may not be exceptions, and remarks that definitely aren't.

I can see the point of not wanting diagnostics already included in an exception to also go to the handler, but there is also the opposite. In a typical debugging situation, one would want to know that the C++ side did produce an (error) diagnostic that was later suppressed by the Python side. I suppose we can make this controllable on the Python side: when the user is setting the handle, let them specify whether they want it to receive diagnostics that resulted in exceptions or not. This will require some additional tracking in the bindings code.

[rkayaith]

Not all diagnostics are errors and should result in exceptions. We have warnings, which may or may not be exceptions, and remarks that definitely aren't.

Yes definitely, I only intend to capture error diagnostics as part of the exception, everything else would continue on to handlers

I can see the point of not wanting diagnostics already included in an exception to also go to the handler, but there is also the opposite. In a typical debugging situation, one would want to know that the C++ side did produce an (error) diagnostic that was later suppressed by the Python side. I suppose we can make this controllable on the Python side: when the user is setting the handle, let them specify whether they want it to receive diagnostics that resulted in exceptions or not. This will require some additional tracking in the bindings code.

How about a context-level option for this, instead of per-handler? If the option is set, exception diagnostics would be propagated to handlers. That seems like it would be simpler to implement, and is probably sufficient for debugging scenarios.

[ftynse]

How about a context-level option for this, instead of per-handler? If the option is set, exception diagnostics would be propagated to handlers. That seems like it would be simpler to implement, and is probably sufficient for debugging scenarios.

SGTM