Fix some checks not ideal to onnx-ml (onnx#781)

* Fix some checks not ideal to onnx-ml 1. Need to consider cases where no default ONNX operator set exists 2. Cannot assume that value_info's are all tensors (CoreML's neural network classifiers contain ZipMap, the they may produce Maps) * Address comments and improve some more error messages * Make SSA not abbreviated * Address a comment * a fix
martyfranz · Apr 21, 2018 · e3da0f9 · e3da0f9
1 parent 4890619
commit e3da0f9
Showing 1 changed file with 28 additions and 15 deletions.
diff --git a/onnx/checker.cc b/onnx/checker.cc
@@ -45,15 +45,26 @@ void check_value_info(const ValueInfoProto& value_info, const CheckerContext&) {
       enforce_has_field(type, elem_type);
       enforce_has_field(type, shape);
     } break;
+#ifdef ONNX_ML
+    case TypeProto::kSequenceType: {
+      const auto& type = value_info.type().sequence_type();
+      enforce_has_field(type, elem_type);
+    } break;
+    case TypeProto::kMapType: {
+      const auto& type = value_info.type().map_type();
+      enforce_has_field(type, key_type);
+      enforce_has_field(type, value_type);
+    } break;
+#endif
     default:
-      fail_check("Unrecognized type value case: ", value_case);
+      fail_check("Unrecognized type value case (value_info name: ", value_info.name(), "): ", value_case);
   }
 }
 
 void check_tensor(const TensorProto& tensor, const CheckerContext& /*ctx*/) {
   enforce_has_field(tensor, data_type);
   if (tensor.data_type() == TensorProto::UNDEFINED) {
-    fail_check("setting data_type field to UNDEFINED is not allowed");
+    fail_check("setting data_type field (tensor name: ", tensor.name(), ") to UNDEFINED is not allowed");
   }
 
   int num_value_fields = 0;
@@ -78,11 +89,11 @@ void check_tensor(const TensorProto& tensor, const CheckerContext& /*ctx*/) {
 #undef check_data_field
 
   if (num_value_fields != 1) {
-    fail_check("TensorProto should contain one and only one value field.");
+    fail_check("TensorProto (tensor name: ", tensor.name(), ") should contain one and only one value field.");
   }
   if (has_raw_data) {
     if (tensor.data_type() == TensorProto::STRING) {
-      fail_check("STRING data should not be stored in raw_data field");
+      fail_check("STRING data (tensor name: ", tensor.name(), ") should not be stored in raw_data field");
     }
     return;
   } else {
@@ -129,7 +140,7 @@ void check_tensor(const TensorProto& tensor, const CheckerContext& /*ctx*/) {
         break;
 
       default:
-        fail_check("Unrecognized data_type: ", tensor.data_type());
+        fail_check("Unrecognized data_type (tensor name: ", tensor.name(), "): ", tensor.data_type());
     }
   }
 
@@ -152,7 +163,7 @@ void check_attribute(
 
 #define check_type(expected_type)                        \
   if (attr.has_type() && attr.type() != expected_type) { \
-    fail_check("type field and data field mismatch.");   \
+    fail_check("type field and data field mismatch in attribute ", attr.name(), ".");   \
   }
 
 #define check_singular_field(field, type) \
@@ -183,7 +194,7 @@ void check_attribute(
 #undef check_repeated_field
 
   if (used_fields != 1) {
-    fail_check("Attribute should contain one and only one value field.");
+    fail_check("Attribute (name: ", attr.name(), ") should contain one and only one value field.");
   }
 
   if (attr.has_t()) {
@@ -209,7 +220,7 @@ void check_node(
   enforce_non_empty_field(node, op_type);
 
   if (node.input().empty() && node.output().empty()) {
-    fail_check("NodeProto has zero input and zero output.");
+    fail_check("NodeProto (name: ", node.name(), ", type: ", node.op_type(), ") has zero input and zero output.");
   }
 
   // Resolve domain for node
@@ -254,7 +265,7 @@ void check_graph(
   for (const auto& value_info : graph.input()) {
     if (output_names.count(value_info.name())) {
       fail_check(
-          "Graph must be in SSA form, however '",
+          "Graph must be in single static assignment (SSA) form, however '",
           value_info.name(),
           "' has been used as graph input names multiple times.");
     }
@@ -304,7 +315,7 @@ void check_graph(
       }
       if (output_names.count(output)) {
         fail_check(
-            "Graph must be in SSA form, however '",
+            "Graph must be in single static assignment (SSA) form, however '",
             output,
             "' has been used as output names multiple times.");
       }
@@ -336,14 +347,16 @@ void check_model(const ModelProto& model) {
   for (const auto& opset_import : model.opset_import()) {
     opset_imports[opset_import.domain()] = static_cast<int>(opset_import.version());
   }
-  auto dit = opset_imports.find(ONNX_DOMAIN);
-  if (dit == opset_imports.end()) {
-    if (model.ir_version() >= 3) {
+  if (model.ir_version() >= 3) {
+    if (opset_imports.empty())
       fail_check(
           "model with IR version >= 3 must specify opset_import for ONNX");
-    } else {
+  } else {
+    if (opset_imports.empty())
       opset_imports[ONNX_DOMAIN] = 1;
-    }
+    else
+      fail_check(
+          "model with IR version < 3 cannot have opset_import specified");
   }
   ctx.set_opset_imports(opset_imports);
   LexicalScopeContext lex_ctx;