areusch commented on a change in pull request #7919:
URL: https://github.com/apache/tvm/pull/7919#discussion_r624542412
##########
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:
> The main complexity of pushing the handling to FFI layer is that we
will need to introduce specific compiled functions in the FFI layer
In this case, the functions are designed to be used with the FFI layer. So
I'm not sure I see a problem with moving the use of the functions closer to the
place where they are intended to be used.
In this case, the complexity is actually the same, because you remove the
need to expose PyErr_Clear() to TVM and instead replace its use with asserting
`PyError_Occurred() != nullptr`. The complexity in the TVM runtime is then
reduced to that around the single frontend-agnostic function which asks "should
I continue?"
Could you clarify about "I have also tried to propagate the exact error to
the python side?" Theoretically, you should not need to propagate any error
here--Python should take care of that.
I'm okay with how we treat the generic function now, though I do think that
if there is just one, it may be a bit overcomplicated to introduce a registry.
A single PackedFunc should suffice here.
--
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]
