Unified error handling in NNVM and Relay frontends

[markrogersjr]

Consolidated helper functions across several files and created custom exceptions. @yzhliu @wweic

[tqchen]

Thanks @KoinFlyp I am not very sure about this change, as the exception themselves are relatively minimal and it does not make sense to introduce another layer of abstraction.

While it makes sense to introduce some of the new exception hierarchy, it might be better to keep the style as raise ExceptionName(string) so that most reader can clearly see what is going on without having to dig into the implementation of raise_xyz function

[yidawang]

#2279 The high level idea is that for product line usage, we would require a unified error handling mechanism. Having it abstracted as a layer would be helpful towards this direction.

Having functions to unified the description of errors could make the error handling mechanism clearer. raising ExceptionName(string) works but the string can be freely defined by the developer, which is sort of out of control. It doesn't sound tedious to me to go one more step to the raise_xyz function as a developer/reader.

[tqchen]

To be clear, I am not against the structured error categorization. But I am not sure about the current proposal of raise_xyz function. Because it is less apparent on what was being raised and a developer needs to dig into the additional layer of the raise_xyz in order to see what is going on.

If it is the same exception(RuntimeError), I would simply inline the error message and follow the same convention. Otheriwse, perhaps it is better to just subclass the RuntimeError to get a new Error class and raise that one.

[yidawang]

Acknowledge that following the Python exception convention makes sense. However, what we want is to be able to return error messages to end users in a systematic way. Having developers independently enter error messages makes this goal difficult to achieve. Perhaps we can define unified error messages to inline with the exception? @KoinFlyp

[tqchen]

see my followup comments in #2279 I think perhaps a simple way is to clearly document the error message convention and have specific error type when necessary.

Notably, existing frameworks(TF, PT) manage to use raise style error in a consistent fashion, I believe it is not a too hard task, with clear error message documentation.

[markrogersjr]

Thanks for the feedback. Perhaps the best approach to error handling is driven by what sorts of errors the user will face. On one end of the spectrum, we have errors that are very specific and cannot be grouped with others, such as the nuances of a particular operator or framework. On the other end of the spectrum, there are errors that are so similar that their handling would be pretty much identical, e.g., unsupported operators.

I'm not too sure what an error message convention would look like, but keeping the fine-grained hierarchy of error types (which inherit from the Python primitive error types) and documenting them thoroughly seems necessary.

[wweic]

The PR consists of 3 parts:

1. Categorized errors types.
2. New exception class hierarchy: OperatorAttributeRequired, OperatorAttributeValueNotValid, etc and error message formatting.
3. Helper functions to throw above exceptions.

I think 1 and 2 are absolutely necessary and I don't find big issues here. For 3 I was on the fence, but after second thought, I would vote to get rid of them. This abstraction doesn't really reduce code complexity, instead adding a bit for new developers.

Also please add good documentation for the exception classes. Maybe with example failure cases, error message format.

[jroesch]

I think we should just have error classes which take the right pieces of state, instead of constructing a string every time, say for missing attribute.

raise MissingAttribute(op, attr)

where

class MissingAttribute(Exception):
  def __init__(self, op, attr):
     msg = ...
     super().__init(msg)

In general there is a lot of repeated error code in the code base and it would be good to move towards these, currently most errors come in the form of checks which are annoying and hard to handle because there is little way to categorize what went wrong. Some errors are unrecoverable but we should make it easier to catch recoverable errors.

[tqchen]

I created #2838 to provide some scaffolds on this. Let us move more discussions to the RFC

[markrogersjr]

Holding off until #2838 is merged.

[markrogersjr]

Can someone please take a look? @jroesch @srkreddy1238 @Huyuwei @yidawang

[yidawang]

cc @tqchen

[JennyChou5]

is this redundant?

[markrogersjr]

raise_not_supported is certainly redundant and has been removed. get_nnvm_op and required_attr are helper functions that invoke error handling but ultimately return various objects such as symbols or attributes.

[tqchen]

@KoinFlyp Would be great if you can also request some additional reviewers, in particular from those who you do not interact physically :)

[markrogersjr]

@tqchen will do, thanks :)

@kazum @srkreddy1238 @Huyuwei

[tqchen]

Please fix the lint errors, @yidawang @JennyChou5, please https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

[wweic]

@KoinFlyp There are integration tests failure in http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-2828/9/pipeline/235.

[markrogersjr]

Tests passed

[tqchen]

Thanks, @JennyChou5 @yidawang @KoinFlyp this is now merged