areusch commented on a change in pull request #7919:
URL: https://github.com/apache/tvm/pull/7919#discussion_r624530604
##########
File path: include/tvm/runtime/registry.h
##########
@@ -52,6 +52,45 @@
namespace tvm {
namespace runtime {
+/*!
+ * \brief Check if signals have been sent to the process and if so
+ * invoke the registered signal handler in the frontend environment.
+ *
+ * When runnning TVM in another langugage(python), the signal handler
+ * may not be immediately executed, but instead the signal is marked
+ * in the interpreter state(to ensure non-blocking of the signal handler).
+ *
+ * This function can be explicitly invoked to check the cached signal
+ * and run the related processing if a signal is marked.
+ *
+ * Invoke this function periodically in a long running C++ function
+ * to check if KeyboardInterrupt happens in a python execution environment.
+ *
+ * Not inserting this function will not cause any correctness
+ * issue, but will delay the KeyboardInterrupt until the function returns
+ * to the python side. So this function is not needed in most API
+ * functions can finish quickly in a reasonable amount of time.
+ *
+ * \code
+ *
+ * int check_signal_every_kiter = 10;
+ *
+ * for (int iter = 0; iter < very_large_number; ++iter) {
+ * if (iter % check_signal_every_kiter == 0) {
+ * tvm::runtime::EnvCheckSignals();
+ * }
+ * // do work here
+ * }
+ *
+ * \endcode
+ *
+ * \note This function is a nop when no signal checking function is registered.
+ *
+ * \throws This function throws approperiate exception if an error happens,
Review comment:
ah you are correct, my mistake. however, I don't agree with the
implementation here. `PyErr_CheckSignals` states:
>It checks whether a signal has been sent to the processes and if so,
invokes the corresponding signal handler. If the signal module is supported,
this can invoke a signal handler written in Python. In all cases, the default
effect for SIGINT is to raise the KeyboardInterrupt exception.
If, in Python, I do:
```
old_sighandler = None
def cleanup_temp_files(*args):
shutil.rmtree("/tmp/foo")
old_sighandler(*args)
old_sighandler = signal.signal(signal.SIGINT, cleanup_temp_files)
```
which is perfectly legal, but `shutil.rmtree` throws an exception, you will
mask that exception here with KeyboardInterrupt.
Instead, it seems we need to throw a special C++-side exception which is
caught in TVMFuncCall and then return normally to Python as if no error
occurred (perhaps first asserting that `PyErr_Occurred() != nullptr`). this
allows whatever exception was raised by the signal handler to propagate
normally with correct traceback information. if for some reason
`PyErr_Occurred() == nullptr` at this time, we could continue to raise a
TVM-side error indicating that a signal handler interrupted the call. Perhaps
the return value of TVMFuncCall should be modified to account for this scenario.
In addition, I think it makes sense to push the handling of this into the
FFI. Different frontends may handle this differently--for instance, other
frontends may not be capable of executing the signal handler from a callback
function such as `PyErr_CheckSignals`, so it seems like it makes sense to me
merely to treat such a function as "should I continue?" and return an
indicative error code when that function says no.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
