[Mlir-commits] [mlir] [mlir][python] Correctly handle python passes that emit errors without signalling failure. (PR #96996)

[Oleksandr Alex Zinenko]

https://github.com/ftynse updated https://github.com/llvm/llvm-project/pull/96996

>From 73257ee16a7f030d98d9b631fd7753b65c3946e9 Mon Sep 17 00:00:00 2001
From: Stella Laurenzo <stellaraccident at gmail.com>
Date: Thu, 27 Jun 2024 19:24:30 -0700
Subject: [PATCH 1/2] [mlir][python] Correctly handle python passes that emit
 errors without signalling failure.

It was previously possible for a pass to emit errors, which would be collected by the diagnostic handler. But if it did not then signal failure, this would trigger an assert (because the diagnostic handler collected errors which were not consumed).

There aren't great things to do in this case. After trying some options, I opted for just issuing a Python level warning when this happens. This isn't great but includes a way to further debug and doesn't change semantics on what actually constitutes an error from the API standpoint. It will at least be visible and is better than an assert.

Fixes https://github.com/llvm/torch-mlir/issues/3506 properly.
---
 mlir/lib/Bindings/Python/Pass.cpp | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/mlir/lib/Bindings/Python/Pass.cpp b/mlir/lib/Bindings/Python/Pass.cpp
index a68421b61641f6..b3fa3d0bc39e0f 100644
--- a/mlir/lib/Bindings/Python/Pass.cpp
+++ b/mlir/lib/Bindings/Python/Pass.cpp
@@ -123,12 +123,28 @@ void mlir::python::populatePassManagerSubmodule(py::module &m) {
               op.getOperation().getContext()->clearOperationsInside(op);
             }
             // Actually run the pass manager.
-            PyMlirContext::ErrorCapture errors(op.getOperation().getContext());
+            PyMlirContext::ErrorCapture error_capture(
+                op.getOperation().getContext());
             MlirLogicalResult status = mlirPassManagerRunOnOp(
                 passManager.get(), op.getOperation().get());
-            if (mlirLogicalResultIsFailure(status))
+            auto errors = error_capture.take();
+            if (mlirLogicalResultIsFailure(status)) {
               throw MLIRError("Failure while executing pass pipeline",
-                              errors.take());
+                              std::move(errors));
+            } else if (!errors.empty()) {
+              // If errors were emitted without a failure, issue a warning.
+              static const char *message =
+                  "An MLIR pass emitted an error but did not signal failure. "
+                  "Unfortunately, this error information has been lost as a "
+                  "result. Setting "
+                  "`context.emit_error_diagnostics = True` can ensure "
+                  "that all diagnostics are printed to stderr, even when being "
+                  "captured for Python exception handling.";
+              if (PyErr_WarnEx(PyExc_UserWarning, message, /*stack_level=*/1) !=
+                  0) {
+                throw py::error_already_set();
+              }
+            }
           },
           "operation"_a, "invalidate_ops"_a = true,
           "Run the pass manager on the provided operation, raising an "

>From bf599025300c7cc5c5fad8ce4aa51398b5525339 Mon Sep 17 00:00:00 2001
From: "Oleksandr \"Alex\" Zinenko" <git at ozinenko.com>
Date: Mon, 8 Jul 2024 22:26:12 +0200
Subject: [PATCH 2/2] Apply suggestions from code review

---
 mlir/lib/Bindings/Python/Pass.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mlir/lib/Bindings/Python/Pass.cpp b/mlir/lib/Bindings/Python/Pass.cpp
index b3fa3d0bc39e0f..a6418035d5ee16 100644
--- a/mlir/lib/Bindings/Python/Pass.cpp
+++ b/mlir/lib/Bindings/Python/Pass.cpp
@@ -123,11 +123,11 @@ void mlir::python::populatePassManagerSubmodule(py::module &m) {
               op.getOperation().getContext()->clearOperationsInside(op);
             }
             // Actually run the pass manager.
-            PyMlirContext::ErrorCapture error_capture(
+            PyMlirContext::ErrorCapture errorCapture(
                 op.getOperation().getContext());
             MlirLogicalResult status = mlirPassManagerRunOnOp(
                 passManager.get(), op.getOperation().get());
-            auto errors = error_capture.take();
+            auto errors = errorCapture.take();
             if (mlirLogicalResultIsFailure(status)) {
               throw MLIRError("Failure while executing pass pipeline",
                               std::move(errors));