Allowing an error handler from caller #12487
Conversation
Summary: To give a chance to the caller to decide how to handle these errors, e.g., sending to logcat, or writing extra diagnostics events, etc. Differential Revision: D78232763
yunquan812
requested review from
JacobSzwejbka,
lucylq and
swolchok
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12487
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8a02503 with merge base 00491fd ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D78232763 |
This PR needs a
|
Summary: To give a chance to the caller to decide how to handle these errors, e.g., sending to logcat, or writing extra diagnostics events, etc. Differential Revision: D78232763
| @@ -49,7 +49,7 @@ Kernel* registered_kernels = reinterpret_cast<Kernel*>(registered_kernels_data); | |||
| size_t num_registered_kernels = 0; | |||
|
|
|||
| // Registers the kernels, but may return an error. | |||
| Error register_kernels_internal(const Span<const Kernel> kernels) { | |||
| Error register_kernels_internal(const Span<const Kernel> kernels, ErrorHandler errorHandler) { | |||
There was a problem hiding this comment.
Why is the return value not sufficient?
There was a problem hiding this comment.
I guess what situation exists where you can pass in the errorHandler to operate on the error generated by this function, but you couldnt just operate on it after its returned?
There was a problem hiding this comment.
If all errors are returned, then we can just operate on it after it is returned, but there are 2 errors (RegistrationExceedingMaxKernels and RegistrationAlreadyRegistered) are not returned (see this line). If the caller wants to handle these 2 errors, the caller needs to specify the errorHandler to get a chance to handle these errors.
|
|
||
| return Error::Ok; | ||
| if (errorHandler != nullptr) { | ||
| err = errorHandler(err); |
There was a problem hiding this comment.
This makes me a bit nervous. Why do we want to consume an Error and translate it to another Error? In practice, are you going to add product specific Error like LoggingFailure etc?
There was a problem hiding this comment.
I'm not against an error handler, just not sure what are we doing with the returned error here
There was a problem hiding this comment.
A pattern I would definitely against is to convert Error::RegistrationAlreadyRegistered into Error::Ok if that's what you are trying to do here
Summary: To give a chance to the caller to decide how to handle these errors, e.g., sending to logcat, or writing extra diagnostics events, etc.
Differential Revision: D78232763