One: [onert] Using macro OP_REQUIRES instead of runtime_error/assert

[dayo09]

Issue

OP_REQUIRES

@glistening made OP_REQUIRES in operationValidator to replace asserts, because there was an issue of not-checking in release mode because the validations were all in asserts.
https://github.com/Samsung/ONE/commit/1153669f341a14caaa5ce95a46e716f015d450e0

However, there are many operation validating codes over runtime, from compile to execute. But some of them are still in assert.

Suggestion

Before changing asserts into runtime_error,

• I want to introduce OP_REQUIRES into onert core by include/util/Utils.h
• In addition, I introduces OP_REQUIRES_MSG, which simply with message
• In addition, I added filename:function:line in front of runtime error message.

Example : OP_REQUIRES_MSG

Before

      if (!(current >= 0 && current < shape_rank && in_shape.dim(current) == 1))
      {
        throw std::runtime_error(
            "The following conditions must be met: 0 <= dim < Shape rank, dim == 1");
      }

Error during ~ : The following conditions must be met: 0 <= dim < Shape rank, dim == 1



md5-ddb4728095ea5bbfd9ad9cbfce3c5af3



```bash
Error during ~ : Failed at ShapeInference.cc:inferSqueezeShape:742 The following conditions must be met: 0 <= dim < Shape rank, dim == 1



md5-2a616664fbc92b2e461eff9b3acf3f35



```bash 
 Error during model prepare : OperationValidator failed at line 1055



md5-5b1f0d0e71a70666055e12f6b2049478



```bash 
 Error during model prepare : Failed at OperationValidator.cc:visit:1055

[glistening]

@dayo09 I prefer to put OP_REQUIRES family in a separate header (like op_requires.h, check.h or something else) since util/Utils is too general name that it could have everything in there. If including the new header requires too much change, you may choose to include the new header in util/Utils.h. Plus, I have to confess that the name OP_REQUIRES comes from tensorflow because I like the name.

[dayo09]

@glistening It sounds good to separate header as op_requires.h.
I actually like the name OP_REQUIRES because it's very straightforward to understand, but I was thinking about coming up with another name for non-op condition checks..

[glistening]

@dayo09 If you're going to use check for runtime check other than operators, it is reasonable to rename.

[dayo09]

@glistening
Well.. How about these?

(1) RUNTIME_REQUIRES for non-op condition check, OP_REQUIRES for op condition check
OR
(2) ONERT_CHECK for all checks
OR
(3) ONERT_VALIDATE for all all checks

[wateret]

@dayo09 #2726 looks relavant.

Long story short, I would like to keep asserts. IMHO both assert and throw have different roles.

• assert is to check developers'(our) faults, For things that should never happen
• throw is for invalid user inputs and other runtime errors(like IO/Network errors) that are unavoidable

[glistening]

@wateret As I understand, @dayo09 is not about to remove all assert.

not-checking in release mode because the validations were all in asserts.

She will change some asserts to throw that should be checked in release build also.

[wateret]

@glistening Got it, thanks.

[wateret]

It sounds good to separate header as op_requires.h

I thought OP_REQUIRES are only used in OperationValidator. If so, should we move it to op_requires.h?

Plus, it is expected to throw different kind of exceptions(not only std::runtime_error) which looks relevant to this work. #2813

[glistening]

@wateret

I thought OP_REQUIRES are only used in OperationValidator.

You're right. OP_REQUIRES was supposed to be used in OperationValidator. It is the reason I used OP_.
@dayo09 would like to use it out of OperationValidator.

If so, should we move it to op_requires.h?

It depends. If OP_REQUIRES is going to be used in some place other than operator validation, we need another name.
I think @dayo09 suggested https://github.com/Samsung/ONE/issues/3270#issuecomment-660786587.
After the name is chosen, we can name the containing file.

[dayo09]

@wateret I checked the issue and found out that issue contains throwing 'out of range exception' or 'file not found exception'...
I agree that is needed.
To point out differences, what I want to improve here is to get more information-such as filename, function, line- when exception occurs, and to make the code simply and readable in lines. I am focusing on operator checks, which spreads over runtime codes, but this kinda implementation can be used other than operation checks actually.

Why I want to changes some asserts

I also agree that asserts are needed. Why I want to change some asserts into exception is : when it comes to operator checks or kernel validations, it's different to tell if code ran wrongly or model file's input was wrong. Most of them are actually exception in tensorflow/armcompute for example, but in our codes, many what-should-be-exceptions are in asserts. It's changed in operationValidator by glistening, but there are still many asserts what should be exceptions.

[wateret]

@glistening @dayo09 Thanks for clarification. I agree with most of them.

There is just one thing that I'm not sure about. let's see the example error message.

Error during ~ : Failed at ShapeInference.cc:inferSqueezeShape:742 The following conditions must be met: 0 <= dim < Shape rank, dim == 1

This message style looks more like an assertion to me. I mean, it looks internal-developer-friendly. I do not think it is good idea to show which source file, function and line number in exception message. It is totally for runtime debugging so I think it would be better to not expose those info(at least in release build).

[hyunsik-yoon]

Related to https://github.com/Samsung/ONE/issues/2726

[dayo09]

@wateret

About exposing line/function/filename

I think that exposing line, function or file can be acceptable if there is no security issue. Our project is an opensource anyways. It will cause a problem if we put exception in interanl repo's released version. To solve it, we can just come up with special type of exception for internal version, for example, OP_REQUIRES_PRIVATE, which doesn't expose line/func/filename.
Actually I just searched about it and Boost library supports a wrapper which enables line/func/filename printing of exception.
Practically it's so hard to debug with some exception messages, we can just revise some of them but it seemed easier for me to change the function itself. Or we can actually make a exception class and manage it.

[wateret]

@dayo09

I am not talking about security. Say I am a user of some program and I tried to open a file that is not exist, I would be confused if the program shows lineno/function/filename in the error message. Just error message would be enough for users. If it was an assertion that info is useful for developers. It is what I meant.

To solve it, we can just come up with special type of exception for internal version, for example, OP_REQUIRES_PRIVATE, which doesn't expose line/func/filename.

For macro definition we don't need to separate definitions for debug and release. We can just define a same macro differently for debug and release.

```c

ifdef DEBUG

define OP_REQUIRES(...) .....

else

define OP_REQUIRES(...) .....

endif

[wateret]

@dayo09 I think I've shown my opinions enough, but please go on if you'd like to. I don't have strong objection, what I said is just my preference.

[ragmani]

It would be nice to change macro's naming to more intuitive. If an macro throw an exception, I prefer the macro name have THROW.

[glistening]

@ragmani What about ONERT_THROW_UNLESS or ONERT_THROW_IF_NOT? Or you may suggest another.