fix review comments

Author: pengwa

tests/common.py

@@ -16,7 +16,7 @@
 __all__ = ["TestConfig", "get_test_config", "unittest_main",
            "check_tf_min_version", "skip_tf_versions",
            "check_opset_min_version", "check_target", "skip_caffe2_backend", "skip_onnxruntime_backend",
-           "skip_specific_opset_version", "check_onnxruntime_incompatibility", "validate_const_node",
+           "skip_opset", "check_onnxruntime_incompatibility", "validate_const_node",
            "group_nodes_by_type"]
 
 
@@ -156,7 +156,7 @@ def check_opset_min_version(min_required_version, message=""):
     return unittest.skipIf(config.opset < min_required_version, reason)
 
 
-def skip_specific_opset_version(opset_v, message=""):
+def skip_opset(opset_v, message=""):
     """ Skip if opset = opset_v """
     config = get_test_config()
     reason = _append_message("conversion requires opset != {}".format(opset_v), message)

tests/test_backend.py

@@ -1399,7 +1399,7 @@ def test_erf(self):
             self._run_test_case([_OUTPUT], {_INPUT: x_val}, rtol=0.01)
 
     @check_opset_min_version(8, "Scan")
-    @skip_specific_opset_version(9, "ReverseSequence not supported")
+    @skip_opset(9, "ReverseSequence")
     def test_reverse_sequence_batch_major(self):
         x_val = np.array([[[1, 2, 3], [4, 5, 6], [0, 0, 0]],
                           [[1, 2, 3], [4, 5, 6], [7, 8, 9]],
@@ -1430,7 +1430,7 @@ def test_reverse_sequence_batch_major(self):
         self._run_test_case([_OUTPUT], {_INPUT: x_val})
 
     @check_opset_min_version(8, "Scan")
-    @skip_specific_opset_version(9, "ReverseSequence not supported")
+    @skip_opset(9, "ReverseSequence")
     def test_reverse_sequence_time_major(self):
         x_val = np.array([[[1, 2, 3], [1, 2, 3], [1, 2, 3]],
                           [[4, 5, 6], [4, 5, 6], [0, 0, 0]],

tests/test_custom_rnncell.py

@@ -13,7 +13,7 @@
 from tensorflow.contrib import rnn
 from tensorflow.python.ops import init_ops
 from backend_test_base import Tf2OnnxBackendTestBase
-from common import check_tf_min_version, check_opset_min_version, unittest_main, skip_specific_opset_version
+from common import check_tf_min_version, check_opset_min_version, unittest_main, skip_opset
 
 
 # pylint: disable=missing-docstring,invalid-name,unused-argument,using-constant-test
@@ -284,7 +284,7 @@ def test_multi_rnn_lstm(self, state_is_tuple=True):
 
     @check_opset_min_version(8, "Scan")
     @check_tf_min_version("1.8")
-    @skip_specific_opset_version(9, "ReverseSequence cannot be efficient mapped in OPSET 9")
+    @skip_opset(9, "ReverseSequence")
     def test_bidrectional_attention_wrapper_lstm_encoder(self):
         size = 30
         time_step = 3

tf2onnx/rewriter/custom_rnn_rewriter.py

@@ -85,6 +85,10 @@ def _parse_rnn_loop(self, context):
         return True
 
     def need_rewrite(self, context):
+        if self.g.opset < 8:
+            log.debug("skip the custom_rnn_rewriter due to lower opset version %s", self.g.opset)
+            return False
+
         context.rnn_scope = self._get_rnn_scope_name(context.while_context_scope)
 
         if not self._parse_rnn_loop(context):
@@ -108,15 +112,15 @@ def rewrite(self, context):
                 if self.g.opset == 8:
                     nodes = self._adapt_scan_sequence_input_or_output("input", state_input, False)
                     state_inputs_initial_values.append(nodes[-1].output[0])
-                else:
+                else:  # since opset 9
                     state_inputs_initial_values.append(state_input)
 
             scan_inputs_initial_values = []
             for scan_input in scan_props.scan_inputs_initial_values:
                 if self.g.opset == 8:
                     nodes = self._adapt_scan_sequence_input_or_output("input", scan_input, False)
                     scan_inputs_initial_values.append(nodes[-1].output[0])
-                else:
+                else:  # since opset 9
                     scan_inputs_initial_values.append(scan_input)
 
             cell_g_info = context.cell_graph
@@ -192,7 +196,7 @@ def _connect_scan_with_output(self, context, scan_node):
                     nodes = self._adapt_scan_sequence_input_or_output("state_output_reshape",
                                                                       scan_node.output[index], True)
                     self.g.replace_all_inputs(self.g.get_nodes(), out_tensor_value_info.id, nodes[-1].output[0])
-                else:
+                else:  # since opset 9
                     self.g.replace_all_inputs(self.g.get_nodes(), out_tensor_value_info.id, scan_node.output[index])
             index += 1
 
@@ -202,7 +206,7 @@ def _connect_scan_with_output(self, context, scan_node):
                     nodes = self._adapt_scan_sequence_input_or_output("scan_output_reshape",
                                                                       scan_node.output[index], True)
                     self.g.replace_all_inputs(self.g.get_nodes(), out_tensor_value_info.id, nodes[-1].output[0])
-                else:
+                else:  # since opset 9
                     self.g.replace_all_inputs(self.g.get_nodes(), out_tensor_value_info.id, scan_node.output[index])
             index += 1
 

tf2onnx/tfonnx.py

@@ -1460,8 +1460,8 @@ def reverse_op9(ctx, node, name, args):
     # here. Actually using loops to do that is kind of meaningless since there will be performance
     # issue there for sure.
 
-    raise RuntimeError("ReverseSequence is not supported to convert in OPSET9,"
-                       " if possible please try use OPSET 8 instead.")
+    raise NotImplementedError("ReverseSequence is not supported to convert in OPSET 9,"
+                              " if possible please try using OPSET 8 instead.")
 
 
 def shape_op(ctx, node, name, args):