[onert] Dedicated exception for invalid data type#16229
[onert] Dedicated exception for invalid data type#16229arkq wants to merge 1 commit intoSamsung:masterfrom
Conversation
| else | ||
| { | ||
| throw std::runtime_error{"Quantize: Unsupported data type"}; | ||
| throw UnsupportedDataTypeException{"Quantize", _input->data_type()}; |
There was a problem hiding this comment.
In general looks good to me but maybe we should consider some better way of handling cases where both input and output type can be unsupported. Probably UnsupportedDataTypeException can be easy extended for 1 input, 1 output and I'm thinking on more generic version.
Maybe UnsupportedDataTypeException(const std::string op_name, const std::vector<ITensor>& inputs, const std::vector<ITensor>& outpus) with message "Operation X with combination inputs data types: ... and output data types: ... is not supported" but it's also not perfect and maybe shouldn't use such details of backend.
There was a problem hiding this comment.
I've been trying to make the exception less generic and hierarchical, but then there is a problem of which exception is the base one. Unsupported data type can be on behalf of an operation or in some other part of the code. Then we have other operation-originated exceptions... Data types are for input, input on the left side, input on the right side, output, and maybe some other cases. So, instead of created dedicated exception I chose more generic approach. However, there is a dedicated constructor for the operation originated exception, so the name of the operation is available in the error message (and potentially might be available in the exception itself).
There was a problem hiding this comment.
I see the problems.
It's a bigger task but it will be nice that each operation has some print capabilities (maybe via visitor design pattern to avoid changes in many places?) with basic info like operation name, inputs/outputs shape, data type. It can be used in error messages. But as for now it's not must have.
| auto input = getGGMLTensor(_input); | ||
| auto indices = getGGMLTensor(_indices); | ||
| auto output = getGGMLTensor(_output); | ||
| auto input = getGGMLTensor("FullyConnected", _input); |
There was a problem hiding this comment.
Perhaps I'm missing something but why is this used with FullyConnected op name?
| { | ||
|
|
||
| struct ggml_tensor getGGMLTensor(const IPortableTensor *tensor); | ||
| struct ggml_tensor getGGMLTensor(std::string op, const IPortableTensor *tensor); |
There was a problem hiding this comment.
Since this is GGML (3rdparty) related, I'd suggest to catch the existing exception from getGGMLTensor and rethrow the specific UnsupportedDataTypeException at the call site. This will lower the amount of changes.
| { | ||
|
|
||
| class OnertException : public std::exception | ||
| class Exception : public std::exception |
There was a problem hiding this comment.
I'm not sure that the change of the existing name is required here. What was the reason behind it?
There was a problem hiding this comment.
I wanted to remove redundancy in naming. Errors are in the onert namespace, so when using fully qualified name you have onert::OnertException, then I would have to follow the same pattern with other exceptions, i.e.: onert::OnertUnsupportedDataTypeException. So, in order to shorten exception names and remove redundancy I've removed the Onert prefix (there is no std::std_exception, but std::exception). If you prefer the old behavior I will restore that.
There was a problem hiding this comment.
I also like to remove the redundancy. However, it is beyond this PR. Could you please don't change this in this PR? We may get other's opinion in separate PR.
| throw std::runtime_error{"Unsupported type size"}; | ||
| case DataType::QUANT_GGML_Q4_0: | ||
| case DataType::QUANT_GGML_Q8_0: | ||
| // GGML block quantize type data size is not supported |
There was a problem hiding this comment.
If the GGML data types are not supported I think they should fall into the default case as before. This change just lists them explicitly but all the previous cases return the size for the supported data types, while this one breaks. Plus the default is now gone which is a potential problem in static analysis tools. I'm not sure that I agree with this approach.
There was a problem hiding this comment.
On contrary, using default in most cases is problematic because static analysis can not verify that some keys are missing. IMHO, the best practice when dealing with enums is to list all keys explicitly (if possible) and handle "default" case outside of the switch. Then, in case when new enum value is added, compiler will report that not all values are handled - you don't have to run the code to see that some functionality is not implemented.
This commit introduces dedicated exception for reporting unsupported or invalid data type. ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy <a.bokowy@samsung.com>
4 participants
This commit introduces dedicated exception for reporting unsupported or invalid data type.
In case of invalid input data for the example
nnpackage/examples/v1.3.0/two_tflites/mv1.0.tflitemodel the output looks like:and the
nnfw_session::run()returnsNNFW_STATUS_UNSUPPORTED_DATA_TYPEstatus code.This PR is small part of issues diagnosed here: #16102 (comment)
ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy a.bokowy@samsung.com