You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/07/07 08:27:17 UTC
[GitHub] [tvm] blackkker opened a new pull request, #12028: [WIP][Pylint] Making frontend tests pylint compliant
blackkker opened a new pull request, #12028:
URL: https://github.com/apache/tvm/pull/12028
Fix pylint issues in https://github.com/apache/tvm/issues/11414
List of files to complete:
- [x] tests/python/frontend/caffe/test_forward.py
- [x] tests/python/frontend/caffe2/model_zoo/init.py
- [x] tests/python/frontend/caffe2/test_forward.py
- [x] tests/python/frontend/coreml/model_zoo/init.py
- [ ] tests/python/frontend/coreml/test_forward.py
- [ ] tests/python/frontend/darknet/test_forward.py
- [ ] tests/python/frontend/keras/test_forward.py
- [ ] tests/python/frontend/mxnet/model_zoo/init.py
- [ ] tests/python/frontend/mxnet/model_zoo/dqn.py
- [ ] tests/python/frontend/mxnet/model_zoo/inception_v3.py
- [ ] tests/python/frontend/mxnet/model_zoo/mlp.py
- [ ] tests/python/frontend/mxnet/model_zoo/resnet.py
- [ ] tests/python/frontend/mxnet/model_zoo/squeezenet.py
- [ ] tests/python/frontend/mxnet/model_zoo/vgg.py
- [ ] tests/python/frontend/mxnet/test_forward.py
- [ ] tests/python/frontend/mxnet/test_graph.py
- [ ] tests/python/frontend/mxnet/test_qnn_ops_utils.py
- [ ] tests/python/frontend/oneflow/test_forward.py
- [ ] tests/python/frontend/oneflow/test_vision_models.py
- [ ] tests/python/frontend/onnx/test_forward.py
- [ ] tests/python/frontend/paddlepaddle/test_forward.py
- [ ] tests/python/frontend/pytorch/qnn_test.py
- [ ] tests/python/frontend/pytorch/test_forward.py
- [ ] tests/python/frontend/pytorch/test_fx_quant.py
- [ ] tests/python/frontend/pytorch/test_lstm.py
- [ ] tests/python/frontend/pytorch/test_object_detection.py
- [ ] tests/python/frontend/pytorch/test_rnns.py
- [ ] tests/python/frontend/tensorflow/test_bn_dynamic.py
- [ ] tests/python/frontend/tensorflow/test_control_flow.py
- [ ] tests/python/frontend/tensorflow/test_debugging.py
- [ ] tests/python/frontend/tensorflow/test_forward.py
- [ ] tests/python/frontend/tensorflow/test_no_op.py
- [ ] tests/python/frontend/tensorflow2/common.py
- [ ] tests/python/frontend/tensorflow2/test_functional_models.py
- [ ] tests/python/frontend/tensorflow2/test_sequential_models.py
- [ ] tests/python/frontend/test_common.py
- [x] tests/python/frontend/tflite/test_forward.py
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1203964598
> Got some problem when fixing `dangerous-default-value` in `pytorch/test_forward.py`.
>
> https://github.com/apache/tvm/blob/39ffe0a5ce14c2105b7d48ee420e82e194787d8f/tests/python/frontend/pytorch/test_forward.py#L122-L124
>
> I've tried several ways but none of them fully solve the problem as [64](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/64/tests) [62](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/62/pipeline) show. Any idea? @areusch @SebastianBoblest
Hi, this function ''verify_model'' is really complicated and the relation between the arguments is quite complex.
For example, if model_name is not a string, what should it be then? None or is something else also possible.
This function alone actually requires extensive testing in my opinion.
But I think this is a separate issue.
Why does
input_data = input_data or []
not work in this case? It should be as close as possible to an empty list as default argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r933045267
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -498,7 +501,7 @@ def _test_convolution(
add_shapes_to_graph_def=True,
):
"""One iteration of convolution with given shapes and attributes"""
-
+ # pylint: disable=dangerous-default-value
Review Comment:
You have a mutable default argument here:
[def _test_convolutio](https://github.com/apache/tvm/blob/45b2d39d8fce7a11a596d03c6b914aa47fc2f952/tests/python/frontend/tensorflow/test_forward.py#L495)
If you want to know why this is bad, look for example here: https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/
To resolve this issue change
deconv_output_shape=[] to deconv_output_shape=None
and add
deconv_output_shape = deconv_output_shape or []
add the start of the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1207736755
cc @areusch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1211437054
thanks @blackkker and sorry for yet another delay. i tried to resolve the merge conflicts myself here, hopefully it goes green and can merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r915688152
##########
tests/python/frontend/coreml/test_forward.py:
##########
@@ -422,6 +427,7 @@ def verify_unary_sqrt(input_dim):
def verify_unary_rsqrt(input_dim, epsilon=0):
+ """Unary rsqrt"""
Review Comment:
Actually I think so. But disabling C0114 in some functions is also inappropriate. So i just follow others.
@areusch any ideas?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929639019
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -294,7 +297,7 @@ def is_gpu_available():
local_device_protos = device_lib.list_local_devices()
gpu_list = [x.name for x in local_device_protos if x.device_type == "GPU"]
- if len(gpu_list) > 0:
+ if len(gpu_list) > 0: # pylint: disable=len-as-condition
Review Comment:
Ok！
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929630679
##########
tests/python/frontend/oneflow/test_vision_models.py:
##########
@@ -14,7 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name
+# pylint: disable=import-self, invalid-name, consider-using-f-string
Review Comment:
My mistake, i will remove it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1203989596
>
> >
>
> Not work casue it will result in `RuntimeError: Boolean value of Tensor with more than one value is ambiguous` as [ci_60](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/60/tests) show.
Ok in this case you might need the more explicit form of this:
input_data = [] if input_data is None else input_data
Often, the shortened version
input_data = input_data or []
is used, however, it has a problem with complex expressions like tensors or numpy arrays that cannot be simply evaluated to True or False.
Sorry for that, that was actually my fault.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker closed pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker closed pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
URL: https://github.com/apache/tvm/pull/12028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r915696791
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -4317,10 +4326,10 @@ def convert_sub_dummy(self, op):
]
out = math_ops.subtract(in_data[0], in_data[1])
in_name = [x[1] for x in zip(in_data, ("in_0:0", "in_1:0"))]
- input_tensors = [x for x in in_data]
+ input_tensors = in_data
output_tensors = [out]
in_node = [0] * len(in_name)
- for i in range(len(in_name)):
+ for i, _ in enumerate(in_name):
in_node[i] = in_name[i].split(":")[0] if ":" in in_name[i] else in_name[i]
Review Comment:
```suggestion
in_node[i] = in_name[i].split(":")[0]
```
If ":" is not found, split will return [in_node_[i]]
##########
tests/python/frontend/coreml/test_forward.py:
##########
@@ -566,6 +579,7 @@ def test_forward_unary():
@tvm.testing.uses_gpu
def test_forward_reduce():
+ """Reduce"""
from enum import Enum
Review Comment:
Couldn't this be imported at top-level?
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2194,14 +2209,14 @@ def __test_elemwise(in_data):
tf.quantization.fake_quant_with_min_max_args(
in_data[0], min=out_min, max=out_max, name="inq_0"
)
- if None != in_data[0]
+ if in_data[0] != None
Review Comment:
```suggestion
if in_data[0] is not None
```
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2194,14 +2209,14 @@ def __test_elemwise(in_data):
tf.quantization.fake_quant_with_min_max_args(
in_data[0], min=out_min, max=out_max, name="inq_0"
)
- if None != in_data[0]
+ if in_data[0] != None
else tf.quantization.fake_quant_with_min_max_args(
data[0], min=out_min, max=out_max, name="const_tensor0"
),
tf.quantization.fake_quant_with_min_max_args(
in_data[1], min=out_min, max=out_max, name="inq_1"
)
- if None != in_data[1]
+ if in_data[1] != None
Review Comment:
```suggestion
if in_data[1] is not None
```
##########
tests/python/frontend/coreml/test_forward.py:
##########
@@ -202,12 +205,12 @@ def verify_ConcatLayerParams(input1_dim, input2_dim):
@tvm.testing.uses_gpu
-def test_forward_ConcatLayerParams():
- verify_ConcatLayerParams((1, 1, 2, 2), (1, 2, 2, 2))
- verify_ConcatLayerParams((1, 2, 4, 4), (1, 3, 4, 4))
+def test_forward_concat_layer_params():
+ verify_concat_layer_params((1, 1, 2, 2), (1, 2, 2, 2))
+ verify_concat_layer_params((1, 2, 4, 4), (1, 3, 4, 4))
-def verify_UpsampleLayerParams(input_dim, scale, mode):
+def _verify_UpsampleLayerParams(input_dim, scale, mode):
Review Comment:
```suggestion
def _verify_upsample_layer_params(input_dim, scale, mode):
```
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2212,50 +2227,37 @@ def __test_elemwise(in_data):
for x in zip(
in_data, (("inq_0", (inq0_min, inq0_max)), ("inq_1", (inq1_min, inq1_max)))
)
- if None != x[0]
+ if x[0] != None
Review Comment:
```suggestion
if x[0] is not None
```
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2212,50 +2227,37 @@ def __test_elemwise(in_data):
for x in zip(
in_data, (("inq_0", (inq0_min, inq0_max)), ("inq_1", (inq1_min, inq1_max)))
)
- if None != x[0]
+ if x[0] != None
}
if math_op is math_ops.equal:
out = math_op(inq_data[0], inq_data[1])
out = with_fused_activation_function(out, fused_activation_function)
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- )
- else:
- out = math_op(inq_data[0], inq_data[1])
- out = with_fused_activation_function(out, fused_activation_function)
- out = tf.quantization.fake_quant_with_min_max_args(
- out, min=out_min, max=out_max, name="out"
- )
-
- # Note same_qnn_params uses experimental_new_converter as toco failed
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- quantized=True,
- input_range=input_range,
- experimental_new_converter=same_qnn_params,
- )
+ # Note same_qnn_params uses experimental_new_converter as toco failed
+ compare_tflite_with_tvm(
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x + ":0" for x in input_range.keys()],
+ [x[1] for x in zip(in_data, inq_data) if x[0] != None],
+ [out],
+ quantized=True,
+ input_range=input_range,
+ experimental_new_converter=same_qnn_params,
+ )
else:
out = math_op(
in_data[0]
- if None != in_data[0]
+ if in_data[0] != None
Review Comment:
```suggestion
if in_data[0] is not None
```
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2212,50 +2227,37 @@ def __test_elemwise(in_data):
for x in zip(
in_data, (("inq_0", (inq0_min, inq0_max)), ("inq_1", (inq1_min, inq1_max)))
)
- if None != x[0]
+ if x[0] != None
}
if math_op is math_ops.equal:
out = math_op(inq_data[0], inq_data[1])
out = with_fused_activation_function(out, fused_activation_function)
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- )
- else:
- out = math_op(inq_data[0], inq_data[1])
- out = with_fused_activation_function(out, fused_activation_function)
- out = tf.quantization.fake_quant_with_min_max_args(
- out, min=out_min, max=out_max, name="out"
- )
-
- # Note same_qnn_params uses experimental_new_converter as toco failed
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- quantized=True,
- input_range=input_range,
- experimental_new_converter=same_qnn_params,
- )
+ # Note same_qnn_params uses experimental_new_converter as toco failed
+ compare_tflite_with_tvm(
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x + ":0" for x in input_range.keys()],
+ [x[1] for x in zip(in_data, inq_data) if x[0] != None],
+ [out],
+ quantized=True,
+ input_range=input_range,
+ experimental_new_converter=same_qnn_params,
+ )
else:
out = math_op(
in_data[0]
- if None != in_data[0]
+ if in_data[0] != None
else ops.convert_to_tensor(data[0], dtype=data[0].dtype),
in_data[1]
- if None != in_data[1]
+ if in_data[1] != None
else ops.convert_to_tensor(data[1], dtype=data[1].dtype),
)
out = with_fused_activation_function(out, fused_activation_function)
compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x[1] for x in zip(in_data, ("in_0:0", "in_1:0")) if None != x[0]],
- [x for x in in_data if None != x],
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x[1] for x in zip(in_data, ("in_0:0", "in_1:0")) if x[0] != None],
Review Comment:
```suggestion
[x[1] for x in zip(in_data, ("in_0:0", "in_1:0")) if x[0] is not None],
```
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2212,50 +2227,37 @@ def __test_elemwise(in_data):
for x in zip(
in_data, (("inq_0", (inq0_min, inq0_max)), ("inq_1", (inq1_min, inq1_max)))
)
- if None != x[0]
+ if x[0] != None
}
if math_op is math_ops.equal:
out = math_op(inq_data[0], inq_data[1])
out = with_fused_activation_function(out, fused_activation_function)
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- )
- else:
- out = math_op(inq_data[0], inq_data[1])
- out = with_fused_activation_function(out, fused_activation_function)
- out = tf.quantization.fake_quant_with_min_max_args(
- out, min=out_min, max=out_max, name="out"
- )
-
- # Note same_qnn_params uses experimental_new_converter as toco failed
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- quantized=True,
- input_range=input_range,
- experimental_new_converter=same_qnn_params,
- )
+ # Note same_qnn_params uses experimental_new_converter as toco failed
+ compare_tflite_with_tvm(
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x + ":0" for x in input_range.keys()],
+ [x[1] for x in zip(in_data, inq_data) if x[0] != None],
+ [out],
+ quantized=True,
+ input_range=input_range,
+ experimental_new_converter=same_qnn_params,
+ )
else:
out = math_op(
in_data[0]
- if None != in_data[0]
+ if in_data[0] != None
else ops.convert_to_tensor(data[0], dtype=data[0].dtype),
in_data[1]
- if None != in_data[1]
+ if in_data[1] != None
Review Comment:
```suggestion
if in_data[1] is not None
```
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2212,50 +2227,37 @@ def __test_elemwise(in_data):
for x in zip(
in_data, (("inq_0", (inq0_min, inq0_max)), ("inq_1", (inq1_min, inq1_max)))
)
- if None != x[0]
+ if x[0] != None
}
if math_op is math_ops.equal:
out = math_op(inq_data[0], inq_data[1])
out = with_fused_activation_function(out, fused_activation_function)
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- )
- else:
- out = math_op(inq_data[0], inq_data[1])
- out = with_fused_activation_function(out, fused_activation_function)
- out = tf.quantization.fake_quant_with_min_max_args(
- out, min=out_min, max=out_max, name="out"
- )
-
- # Note same_qnn_params uses experimental_new_converter as toco failed
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- quantized=True,
- input_range=input_range,
- experimental_new_converter=same_qnn_params,
- )
+ # Note same_qnn_params uses experimental_new_converter as toco failed
+ compare_tflite_with_tvm(
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x + ":0" for x in input_range.keys()],
+ [x[1] for x in zip(in_data, inq_data) if x[0] != None],
+ [out],
+ quantized=True,
+ input_range=input_range,
+ experimental_new_converter=same_qnn_params,
+ )
else:
out = math_op(
in_data[0]
- if None != in_data[0]
+ if in_data[0] != None
else ops.convert_to_tensor(data[0], dtype=data[0].dtype),
in_data[1]
- if None != in_data[1]
+ if in_data[1] != None
else ops.convert_to_tensor(data[1], dtype=data[1].dtype),
)
out = with_fused_activation_function(out, fused_activation_function)
compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x[1] for x in zip(in_data, ("in_0:0", "in_1:0")) if None != x[0]],
- [x for x in in_data if None != x],
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x[1] for x in zip(in_data, ("in_0:0", "in_1:0")) if x[0] != None],
+ [x for x in in_data if x != None],
Review Comment:
```suggestion
[x for x in in_data if x is not None],
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r931630231
##########
tests/python/frontend/caffe2/test_forward.py:
##########
@@ -14,15 +14,22 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+# pylint: disable=no-else-return
Review Comment:
this one should be disabled globally per #11327 . do you need it here?
##########
tests/python/frontend/onnx/test_forward.py:
##########
@@ -14,23 +14,35 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+# pylint: disable=import-self, invalid-name, unused-argument, unused-variable, missing-function-docstring, redefined-builtin, consider-using-f-string
Review Comment:
possible to not define some of these at file level? otherwise, reminder to add a comment here.
##########
tests/python/frontend/keras/test_forward.py:
##########
@@ -14,12 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+# pylint: disable=invalid-name, missing-docstring
Review Comment:
can you just comment why you added these?
##########
tests/python/frontend/keras/test_forward.py:
##########
@@ -113,173 +114,175 @@ def to_channels_last(arr):
tvm.testing.assert_allclose(kout, tout, rtol=1e-5, atol=1e-5)
-def get_mobilenet(keras):
- if hasattr(keras.applications, "MobileNet"):
+def get_mobilenet(keras_):
Review Comment:
since `keras_` isn't a private member, might suggest keras_mod or keras_lib or something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r933048302
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -498,7 +501,7 @@ def _test_convolution(
add_shapes_to_graph_def=True,
):
"""One iteration of convolution with given shapes and attributes"""
-
+ # pylint: disable=dangerous-default-value
Review Comment:
Thanks for your answer, learned a lot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1211835192
@tvm-bot merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1204050200
> >
>
> > >
> >
> >
> > Not work casue it will result in `RuntimeError: Boolean value of Tensor with more than one value is ambiguous` as [ci_60](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/60/tests) show.
>
> Ok in this case you might need the more explicit form of this: input_data = [] if input_data is None else input_data
>
> Often, the shortened version input_data = input_data or [] is used, however, it has a problem with complex expressions like tensors or numpy arrays that cannot be simply evaluated to True or False. Sorry for that, that was actually my fault.
Haha, my fault too. My first version is something like this `input_data = [] if input_data is None else input_data`. But I took your version because it looks cooler and simpler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929632890
##########
tests/python/frontend/pytorch/test_forward.py:
##########
@@ -227,13 +230,14 @@ def verify_model_with_input(
compiled_output = relay_model.get_output(0).numpy()
assert_shapes_match(baseline_outputs, compiled_output)
- if assert_shape_only == False:
+ if assert_shape_only is False:
tvm.testing.assert_allclose(baseline_outputs, compiled_output, rtol=rtol, atol=atol)
# Single operator tests
@tvm.testing.uses_gpu
def test_forward_pixel_shuffle():
+ """PixelShuffle"""
Review Comment:
Ok！
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929714530
##########
tests/python/frontend/keras/test_forward.py:
##########
@@ -14,21 +14,21 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+# pylint: disable=invalid-name, redefined-outer-name, missing-function-docstring
Review Comment:
Disable invalid-name refer to other files. And i remove redefined-outer-name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1211829523
>
Unfortunately, it doesn't go green and i fix the conflicts. This time, I hope you can merge as soon as possible beacuse solving conflicts is annoying. Cry with joy. @areusch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1213578084
thanks @blackkker ! and so sorry for the delays here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r931629212
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -14,29 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=import-self, invalid-name, unused-argument, unused-variable, redefined-builtin
Review Comment:
ah yeah fair. if you have cycles, it would be great to remove these var--this lint check does tend to catch some bugs. at least though, for any pylint disable, let's add a comment explaining why we're adding it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929114832
##########
tests/python/frontend/oneflow/test_vision_models.py:
##########
@@ -14,7 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name
+# pylint: disable=import-self, invalid-name, consider-using-f-string
Review Comment:
could you say why this one is disabled?
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -14,29 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=import-self, invalid-name, unused-argument, unused-variable, redefined-builtin
Review Comment:
can you say why you're disabling these extras?
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2557,14 +2579,15 @@ def _test_forward_add_n(inputs):
temp.append(tf.placeholder(shape=each.shape, dtype=each.dtype))
output = tf.add_n(temp)
compare_tflite_with_tvm(
- [each for each in inputs],
+ [each for each in inputs], # pylint: disable=unnecessary-comprehension
Review Comment:
list(inputs), here and below
##########
tests/python/frontend/keras/test_forward.py:
##########
@@ -125,7 +125,7 @@ def get_mobilenet(keras):
@tvm.testing.uses_gpu
-class TestKeras:
+class TestKeras: # pylint: disable=missing-class-docstring
Review Comment:
can you just add a brief docstring instead?
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -498,7 +501,7 @@ def _test_convolution(
add_shapes_to_graph_def=True,
):
"""One iteration of convolution with given shapes and attributes"""
-
+ # pylint: disable=dangerous-default-value
Review Comment:
please don't disable this one, it can lead to very insidious false test results. here and below.
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -1831,6 +1851,7 @@ def test_forward_concatenation():
def _test_unary_elemwise(math_op, data, quantized, quant_range=[-6, 6], int_quant_dtype=tf.int8):
"""One iteration of unary elemwise"""
+ # pylint: disable= dangerous-default-value
Review Comment:
don't disable this one--instead say `quant_range=(-6, 6)`
##########
tests/python/frontend/pytorch/test_forward.py:
##########
@@ -227,13 +230,14 @@ def verify_model_with_input(
compiled_output = relay_model.get_output(0).numpy()
assert_shapes_match(baseline_outputs, compiled_output)
- if assert_shape_only == False:
+ if assert_shape_only is False:
tvm.testing.assert_allclose(baseline_outputs, compiled_output, rtol=rtol, atol=atol)
# Single operator tests
@tvm.testing.uses_gpu
def test_forward_pixel_shuffle():
+ """PixelShuffle"""
Review Comment:
per the previous discussion, can you adjust the docstrings to match the function name?
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -14,21 +14,30 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=import-self, invalid-name, unused-argument, unused-variable
Review Comment:
similar question with these
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -1777,16 +1791,22 @@ def test_forward_shape():
limit = tf.placeholder(dtype=tf.int32, shape=[], name="limit")
delta = tf.placeholder(dtype=tf.int32, shape=[], name="delta")
r = tf.range(start, limit, delta, tf.int32, name="range")
- out = tf.shape(r, out_type=tf.dtypes.int32)
+ out = tf.shape(r, out_type=dtype)
+ out = tf.add(out, tf.constant([1], dtype=dtype))
compare_tflite_with_tvm(
- [x for x in np.nditer(data)],
+ [x for x in np.nditer(data)], # pylint: disable=unnecessary-comprehension
Review Comment:
can we do list(np.nditer(data)) to avoid disabling?
##########
tests/python/frontend/darknet/test_forward.py:
##########
@@ -23,16 +24,16 @@
"""
import numpy as np
import tvm
-from tvm import te
from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
-download_testdata.__test__ = False
from tvm.relay.testing.darknet import LAYERTYPE
from tvm.relay.testing.darknet import __darknetffi__
from tvm.relay.frontend.darknet import ACTIVATION
from tvm import relay
+download_testdata.__test__ = False
Review Comment:
yeah i believe so, i tried it locally and it seems to be ok for this particular test. it seems like that's designed to tell pytest that download_testdata isn't a test, but i'm not sure why it would think that to begin with.
##########
tests/python/frontend/caffe/test_forward.py:
##########
@@ -37,9 +32,13 @@
import tvm
import tvm.testing
from tvm import relay
-from tvm.contrib import utils, graph_executor
+from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
+os.environ["GLOG_minloglevel"] = "2"
Review Comment:
ah sorry, my mistake; nevermind
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -294,7 +297,7 @@ def is_gpu_available():
local_device_protos = device_lib.list_local_devices()
gpu_list = [x.name for x in local_device_protos if x.device_type == "GPU"]
- if len(gpu_list) > 0:
+ if len(gpu_list) > 0: # pylint: disable=len-as-condition
Review Comment:
given the list is initialized on previous like, i think you can adopt the preferred syntax `if gpu_list:` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1205421237
@areusch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r915691941
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2212,50 +2227,37 @@ def __test_elemwise(in_data):
for x in zip(
in_data, (("inq_0", (inq0_min, inq0_max)), ("inq_1", (inq1_min, inq1_max)))
)
- if None != x[0]
+ if x[0] != None
}
if math_op is math_ops.equal:
out = math_op(inq_data[0], inq_data[1])
out = with_fused_activation_function(out, fused_activation_function)
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- )
- else:
- out = math_op(inq_data[0], inq_data[1])
- out = with_fused_activation_function(out, fused_activation_function)
- out = tf.quantization.fake_quant_with_min_max_args(
- out, min=out_min, max=out_max, name="out"
- )
-
- # Note same_qnn_params uses experimental_new_converter as toco failed
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if None != x[0]],
- [out],
- quantized=True,
- input_range=input_range,
- experimental_new_converter=same_qnn_params,
- )
+ # Note same_qnn_params uses experimental_new_converter as toco failed
+ compare_tflite_with_tvm(
+ [x[1] for x in zip(in_data, data) if x[0] != None],
+ [x + ":0" for x in input_range.keys()],
+ [x[1] for x in zip(in_data, inq_data) if x[0] != None],
+ [out],
+ quantized=True,
+ input_range=input_range,
+ experimental_new_converter=same_qnn_params,
+ )
else:
out = math_op(
in_data[0]
- if None != in_data[0]
+ if in_data[0] != None
else ops.convert_to_tensor(data[0], dtype=data[0].dtype),
in_data[1]
- if None != in_data[1]
+ if in_data[1] != None
else ops.convert_to_tensor(data[1], dtype=data[1].dtype),
)
out = with_fused_activation_function(out, fused_activation_function)
compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if None != x[0]],
- [x[1] for x in zip(in_data, ("in_0:0", "in_1:0")) if None != x[0]],
- [x for x in in_data if None != x],
+ [x[1] for x in zip(in_data, data) if x[0] != None],
Review Comment:
```suggestion
[x[1] for x in zip(in_data, data) if x[0] is not None],
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r916460595
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -2234,16 +2234,28 @@ def __test_elemwise(in_data):
out = math_op(inq_data[0], inq_data[1])
out = with_fused_activation_function(out, fused_activation_function)
- # Note same_qnn_params uses experimental_new_converter as toco failed
- compare_tflite_with_tvm(
- [x[1] for x in zip(in_data, data) if x[0] is not None],
- [x + ":0" for x in input_range.keys()],
- [x[1] for x in zip(in_data, inq_data) if x[0] is not None],
- [out],
- quantized=True,
- input_range=input_range,
- experimental_new_converter=same_qnn_params,
- )
+ compare_tflite_with_tvm(
Review Comment:
Restore lines deleted by accident
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1203968943
>
Not work casue it will result in `RuntimeError: Boolean value of Tensor with more than one value is ambiguous` as [ci_60](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/60/tests) show.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929632890
##########
tests/python/frontend/pytorch/test_forward.py:
##########
@@ -227,13 +230,14 @@ def verify_model_with_input(
compiled_output = relay_model.get_output(0).numpy()
assert_shapes_match(baseline_outputs, compiled_output)
- if assert_shape_only == False:
+ if assert_shape_only is False:
tvm.testing.assert_allclose(baseline_outputs, compiled_output, rtol=rtol, atol=atol)
# Single operator tests
@tvm.testing.uses_gpu
def test_forward_pixel_shuffle():
+ """PixelShuffle"""
Review Comment:
Ok！
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929636324
##########
tests/python/frontend/darknet/test_forward.py:
##########
@@ -23,16 +24,16 @@
"""
import numpy as np
import tvm
-from tvm import te
from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
-download_testdata.__test__ = False
from tvm.relay.testing.darknet import LAYERTYPE
from tvm.relay.testing.darknet import __darknetffi__
from tvm.relay.frontend.darknet import ACTIVATION
from tvm import relay
+download_testdata.__test__ = False
Review Comment:
Ok！
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1196289756
@areusch basically completed, there are still a few questions that need to be confirmed with you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r933037538
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -498,7 +501,7 @@ def _test_convolution(
add_shapes_to_graph_def=True,
):
"""One iteration of convolution with given shapes and attributes"""
-
+ # pylint: disable=dangerous-default-value
Review Comment:
How can i pass ci without disable it. https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/51/pipeline/
@areusch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r933045267
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -498,7 +501,7 @@ def _test_convolution(
add_shapes_to_graph_def=True,
):
"""One iteration of convolution with given shapes and attributes"""
-
+ # pylint: disable=dangerous-default-value
Review Comment:
You have a mutable default argument here:
[def _test_convolutio](https://github.com/apache/tvm/blob/45b2d39d8fce7a11a596d03c6b914aa47fc2f952/tests/python/frontend/tensorflow/test_forward.py#L495)
If you want to know why this is bad, look for example here: https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/
To resolve this issue change
deconv_output_shape=[] to deconv_output_shape=None
and add
deconv_output_shape = deconv_output_shape or []
at the start of the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r932947473
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -14,20 +14,30 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=unused-argument, disable=import-outside-toplevel, inconsistent-return-statements
Review Comment:
1. import-outside-toplevelhttps://github.com/apache/tvm/blob/9a4d80c5fe7d756baa83708fd9679c7ae0fac195/tests/python/frontend/tflite/test_forward.py#L190-L199
2. inconsistent-return-statements https://github.com/apache/tvm/blob/9a4d80c5fe7d756baa83708fd9679c7ae0fac195/tests/python/frontend/tflite/test_forward.py#L1976-L1995
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -14,20 +14,30 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=unused-argument, disable=import-outside-toplevel, inconsistent-return-statements
Review Comment:
1. import-outside-toplevel https://github.com/apache/tvm/blob/9a4d80c5fe7d756baa83708fd9679c7ae0fac195/tests/python/frontend/tflite/test_forward.py#L190-L199
2. inconsistent-return-statements https://github.com/apache/tvm/blob/9a4d80c5fe7d756baa83708fd9679c7ae0fac195/tests/python/frontend/tflite/test_forward.py#L1976-L1995
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1203913515
Got some problem when fixing `dangerous-default-value` in `pytorch/test_forward.py`.
https://github.com/apache/tvm/blob/39ffe0a5ce14c2105b7d48ee420e82e194787d8f/tests/python/frontend/pytorch/test_forward.py#L122-L124
I've tried several ways but none of them fully solve the problem as [64](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/64/tests) [62](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-12028/62/pipeline) show. Any idea? @areusch @SebastianBoblest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] SebastianBoblest commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
SebastianBoblest commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r915683420
##########
tests/python/frontend/coreml/test_forward.py:
##########
@@ -422,6 +427,7 @@ def verify_unary_sqrt(input_dim):
def verify_unary_rsqrt(input_dim, epsilon=0):
+ """Unary rsqrt"""
Review Comment:
Isn't it better to disable C0114 in pylint than to add doc-strings that are not useful?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r931628111
##########
tests/python/frontend/pytorch/test_forward.py:
##########
@@ -227,13 +230,14 @@ def verify_model_with_input(
compiled_output = relay_model.get_output(0).numpy()
assert_shapes_match(baseline_outputs, compiled_output)
- if assert_shape_only == False:
+ if assert_shape_only is False:
tvm.testing.assert_allclose(baseline_outputs, compiled_output, rtol=rtol, atol=atol)
# Single operator tests
@tvm.testing.uses_gpu
def test_forward_pixel_shuffle():
+ """PixelShuffle"""
Review Comment:
er i was thinking just use an exact copy of the test name: `test_forward_pixel_shuffle`. Then we don't create two atoms to grep for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r919748281
##########
tests/python/frontend/darknet/test_forward.py:
##########
@@ -23,16 +24,16 @@
"""
import numpy as np
import tvm
-from tvm import te
from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
-download_testdata.__test__ = False
from tvm.relay.testing.darknet import LAYERTYPE
from tvm.relay.testing.darknet import __darknetffi__
from tvm.relay.frontend.darknet import ACTIVATION
from tvm import relay
+download_testdata.__test__ = False
Review Comment:
Not sure, can i remove？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r919747209
##########
tests/python/frontend/caffe/test_forward.py:
##########
@@ -37,9 +32,13 @@
import tvm
import tvm.testing
from tvm import relay
-from tvm.contrib import utils, graph_executor
+from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
+os.environ["GLOG_minloglevel"] = "2"
Review Comment:
Not sure, this line existed before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on PR #12028:
URL: https://github.com/apache/tvm/pull/12028#issuecomment-1177381813
> I have some minor suggestions but all in all this look good. Especially the conditions "!= None" should be changed to "is not None" in my opinion however.
LGTM! Thanks for your advice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r918374814
##########
tests/python/frontend/caffe/test_forward.py:
##########
@@ -37,9 +32,13 @@
import tvm
import tvm.testing
from tvm import relay
-from tvm.contrib import utils, graph_executor
+from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
+os.environ["GLOG_minloglevel"] = "2"
Review Comment:
is this one needed?
##########
tests/python/frontend/darknet/test_forward.py:
##########
@@ -23,16 +24,16 @@
"""
import numpy as np
import tvm
-from tvm import te
from tvm.contrib import graph_executor
from tvm.contrib.download import download_testdata
-download_testdata.__test__ = False
from tvm.relay.testing.darknet import LAYERTYPE
from tvm.relay.testing.darknet import __darknetffi__
from tvm.relay.frontend.darknet import ACTIVATION
from tvm import relay
+download_testdata.__test__ = False
Review Comment:
is this needed?
##########
tests/python/frontend/coreml/test_forward.py:
##########
@@ -827,10 +845,10 @@ def test_can_build_keras_to_coreml_to_relay():
if __name__ == "__main__":
- test_forward_AddLayerParams()
- test_forward_ConcatLayerParams()
- test_forward_MultiplyLayerParams()
- test_forward_UpsampleLayerParams()
+ test_forward_add_layer_params
Review Comment:
tvm.testing.main() here
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -4978,6 +5015,8 @@ def test_forward_nms_v5():
# Main
# ----
if __name__ == "__main__":
+ test_all_elemwise()
Review Comment:
tvm.testing.main()
##########
tests/python/frontend/coreml/test_forward.py:
##########
@@ -422,6 +427,7 @@ def verify_unary_sqrt(input_dim):
def verify_unary_rsqrt(input_dim, epsilon=0):
+ """Unary rsqrt"""
Review Comment:
maybe as a stand-in here, you could just repeat the test name verbatim? it'd be great to avoid abbreviating so that we continue to refer to each unit test with exactly one name
##########
tests/python/frontend/keras/test_forward.py:
##########
@@ -14,21 +14,21 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+# pylint: disable=invalid-name, redefined-outer-name, missing-function-docstring
Review Comment:
could you say why you're disabling lint when you do that? also, just a gentle push to try and avoid doing that unless theres an intractable problem here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929644792
##########
tests/python/frontend/tensorflow/test_forward.py:
##########
@@ -14,29 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=import-self, invalid-name, unused-argument, unused-variable, redefined-builtin
Review Comment:
Just because there are too many lines to modify. LOL. Should I remove all unused variable？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r929651035
##########
tests/python/frontend/pytorch/test_forward.py:
##########
@@ -227,13 +230,14 @@ def verify_model_with_input(
compiled_output = relay_model.get_output(0).numpy()
assert_shapes_match(baseline_outputs, compiled_output)
- if assert_shape_only == False:
+ if assert_shape_only is False:
tvm.testing.assert_allclose(baseline_outputs, compiled_output, rtol=rtol, atol=atol)
# Single operator tests
@tvm.testing.uses_gpu
def test_forward_pixel_shuffle():
+ """PixelShuffle"""
Review Comment:
Just to ensure, adjust the docstrings like this `Test forward pixel shuffle`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] blackkker commented on a diff in pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
blackkker commented on code in PR #12028:
URL: https://github.com/apache/tvm/pull/12028#discussion_r932947473
##########
tests/python/frontend/tflite/test_forward.py:
##########
@@ -14,20 +14,30 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=import-self, invalid-name, unused-argument
+# pylint: disable=unused-argument, disable=import-outside-toplevel, inconsistent-return-statements
Review Comment:
1. import-outside-toplevel can be remove when tflite https://github.com/apache/tvm/blob/9a4d80c5fe7d756baa83708fd9679c7ae0fac195/tests/python/frontend/tflite/test_forward.py#L190-L199
2. inconsistent-return-statements https://github.com/apache/tvm/blob/9a4d80c5fe7d756baa83708fd9679c7ae0fac195/tests/python/frontend/tflite/test_forward.py#L1976-L1995
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #12028: [WIP][Pylint] Making frontend tests pylint compliant
Posted by GitBox <gi...@apache.org>.
areusch merged PR #12028:
URL: https://github.com/apache/tvm/pull/12028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org