Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Pylint] Making frontend tests pylint compliant [part 1] #12028

Merged
merged 112 commits into from
Aug 12, 2022

Conversation

blackkker
Copy link
Contributor

@blackkker blackkker commented Jul 7, 2022

Fix pylint issues in #11414
List of files to complete:

  • tests/python/frontend/caffe/test_forward.py
  • tests/python/frontend/caffe2/model_zoo/init.py
  • tests/python/frontend/caffe2/test_forward.py
  • 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/oneflow/test_forward.py
  • tests/python/frontend/oneflow/test_vision_models.py
  • tests/python/frontend/onnx/test_forward.py
  • tests/python/frontend/pytorch/test_forward.py
  • tests/python/frontend/tensorflow/test_forward.py
  • 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 by @ them in the pull request thread.

Copy link
Contributor

@SebastianBoblest SebastianBoblest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/coreml/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/coreml/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
@blackkker
Copy link
Contributor Author

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.

@blackkker blackkker force-pushed the pylint_copy branch 2 times, most recently from 0ef5cf7 to defc272 Compare July 8, 2022 04:48
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @blackkker for working on this! i added some comments here. i think it needs another round of polish and then we can submit

tests/python/frontend/caffe/test_forward.py Show resolved Hide resolved
tests/python/frontend/coreml/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/coreml/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/darknet/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/keras/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
@blackkker blackkker force-pushed the pylint_copy branch 2 times, most recently from cf216ef to 44a3eb5 Compare July 14, 2022 02:10
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @blackkker and very sorry for the delay. i found a couple of pylint disables that we should not add and instead fix (i.e. the dangerous-default ones). i left a bunch of comments where things could be improved. otherwise this all looks pretty reasonable. can you fix these things and then we can merge?

tests/python/frontend/caffe/test_forward.py Show resolved Hide resolved
tests/python/frontend/oneflow/test_vision_models.py Outdated Show resolved Hide resolved
tests/python/frontend/pytorch/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tensorflow/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tensorflow/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/darknet/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/keras/test_forward.py Outdated Show resolved Hide resolved
@blackkker blackkker force-pushed the pylint_copy branch 3 times, most recently from 9aa7cec to 1684eb2 Compare July 27, 2022 02:39
@blackkker
Copy link
Contributor Author

@areusch basically completed, there are still a few questions that need to be confirmed with you.

@blackkker blackkker requested a review from areusch July 27, 2022 06:56
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @blackkker , not too much else to add, but just identified a couple of disables to look at

tests/python/frontend/caffe2/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/keras/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/keras/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/onnx/test_forward.py Outdated Show resolved Hide resolved
@blackkker blackkker force-pushed the pylint_copy branch 2 times, most recently from ca1cc61 to 9d83d45 Compare July 28, 2022 06:43
@@ -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
Copy link
Contributor Author

@blackkker blackkker Jul 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. import-outside-toplevel
    try:
    import tflite.Model
    tflite_model = tflite.Model.Model.GetRootAsModel(tflite_model_buf, 0)
    except AttributeError:
    import tflite
    tflite_model = tflite.Model.GetRootAsModel(tflite_model_buf, 0)
    except ImportError:
    raise ImportError("The tflite package must be installed")
  2. inconsistent-return-statements
    if quantized:
    tflite_model_quant = _unary_elewise_create_model(
    tf.math.abs, data, offset=1, int_quant_dtype=int_quant_dtype
    )
    tflite_output = run_tflite_graph(tflite_model_quant, data)
    # TFLite 2.6.x upgrade support
    if tf.__version__ < LooseVersion("2.6.1"):
    in_node = ["serving_default_input_int8"]
    else:
    in_node = (
    ["serving_default_input_int16"] if int_quant_dtype == tf.int16 else ["tfl.quantize"]
    )
    tvm_output = run_tvm_graph(tflite_model_quant, data, in_node)
    tvm.testing.assert_allclose(
    np.squeeze(tvm_output[0]), np.squeeze(tflite_output[0]), rtol=1e-5, atol=1e-2
    )
    else:
    return _test_unary_elemwise(math_ops.abs, data, quantized, int_quant_dtype=int_quant_dtype)

@blackkker
Copy link
Contributor Author

Got some problem when fixing dangerous-default-value in pytorch/test_forward.py.

def verify_model(
model_name, input_data=[], custom_convert_map={}, rtol=1e-5, atol=1e-5, expected_ops=[]
):

I've tried several ways but none of them fully solve the problem as 64 62 show. Any idea? @areusch @SebastianBoblest

@SebastianBoblest
Copy link
Contributor

Got some problem when fixing dangerous-default-value in pytorch/test_forward.py.

def verify_model(
model_name, input_data=[], custom_convert_map={}, rtol=1e-5, atol=1e-5, expected_ops=[]
):

I've tried several ways but none of them fully solve the problem as 64 62 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.

@blackkker
Copy link
Contributor Author

Not work casue it will result in RuntimeError: Boolean value of Tensor with more than one value is ambiguous as ci_60 show.

@SebastianBoblest
Copy link
Contributor

Not work casue it will result in RuntimeError: Boolean value of Tensor with more than one value is ambiguous as ci_60 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.

@blackkker
Copy link
Contributor Author

Not work casue it will result in RuntimeError: Boolean value of Tensor with more than one value is ambiguous as ci_60 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

@blackkker blackkker requested a review from areusch August 4, 2022 01:40
@blackkker
Copy link
Contributor Author

@areusch

@blackkker
Copy link
Contributor Author

cc @areusch

@areusch
Copy link
Contributor

areusch commented Aug 11, 2022

thanks @blackkker and sorry for yet another delay. i tried to resolve the merge conflicts myself here, hopefully it goes green and can merge.

@blackkker
Copy link
Contributor Author

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

@blackkker
Copy link
Contributor Author

@tvm-bot merge

@areusch areusch merged commit 1737308 into apache:main Aug 12, 2022
@areusch
Copy link
Contributor

areusch commented Aug 12, 2022

thanks @blackkker ! and so sorry for the delays here.

@blackkker blackkker changed the title [WIP][Pylint] Making frontend tests pylint compliant [Pylint] Making frontend tests pylint compliant [part 1] Aug 15, 2022
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* [CI] Apply linting rules to tf tests

* [CI] Apply linting rules to tflite tests

* [CI] Apply linting rules to coreml tests

* [CI] Apply linting rules to caffe tests

* [CI] Apply linting rules to caffe2 tests

* reformat by black

* reformat by black

* [CI] Apply linting rules to coreml tests

* [CI] Apply linting rules to darknet tests

* Update tests/python/frontend/coreml/test_forward.py

Co-authored-by: Sebastian Boblest <sebastian.boblest@etas.com>

* Update tests/python/frontend/tflite/test_forward.py

Co-authored-by: Sebastian Boblest <sebastian.boblest@etas.com>

* Update tests/python/frontend/coreml/test_forward.py

* Update tests/python/frontend/tflite/test_forward.py

* fix test errors

* fix ci test errors

* [CI] Apply linting rules to keras tests

* [CI] Apply linting rules to oneflow tests

* [CI] Apply linting rules to onnx tests

* replace with tvm.testing.main()

* fix conflict

* Update as @areusch suggest

* pylint pytorch/test_forward.py

* Update as @areusch suggest

* reformat by black

* fix redefined-outer-name lint errors

* remove rules

* remove rules & fix pylint errors

* Remove all unused_var and fix other pylint errors

* reformatted by black

* [CI] Apply linting rules to onnx tests

* Disable unused-argument is for some unused-argument in function can not be removed

* Fix ci errors

* reformatted by black

* Fix ci errors

* Fix invalid-name/unused-variable,redefined-builtin

* [CI] Apply linting rules to caffe tests

* [CI] Apply linting rules to darknet tests

* [CI] Apply linting rules to pytorch tests

* [CI] Apply linting rules to tflite tests

* reformat by black

* [Pylint] Making frontend tests pylint compliant Part 1 of N

* Fix ci errors

* Fix invalid-name pylint errors

* Fix invalid-name pylint errors

* Disabale temporarily

* Fix dangerous-default-value

* Fix typo errors

* Fix ci errors

* [CI] Apply linting rules to tensorflow tests

* Fix dangerous-default-value

* Fix ungrouped-imports

* [CI] Apply linting rules to tf tests

* [CI] Apply linting rules to tflite tests

* [CI] Apply linting rules to coreml tests

* [CI] Apply linting rules to caffe tests

* [CI] Apply linting rules to caffe2 tests

* reformat by black

* reformat by black

* [CI] Apply linting rules to coreml tests

* [CI] Apply linting rules to darknet tests

* Update tests/python/frontend/coreml/test_forward.py

Co-authored-by: Sebastian Boblest <sebastian.boblest@etas.com>

* Update tests/python/frontend/tflite/test_forward.py

Co-authored-by: Sebastian Boblest <sebastian.boblest@etas.com>

* Update tests/python/frontend/coreml/test_forward.py

* Update tests/python/frontend/tflite/test_forward.py

* fix test errors

* fix ci test errors

* [CI] Apply linting rules to keras tests

* [CI] Apply linting rules to oneflow tests

* [CI] Apply linting rules to onnx tests

* replace with tvm.testing.main()

* fix conflict

* Update as @areusch suggest

* pylint pytorch/test_forward.py

* Update as @areusch suggest

* reformat by black

* fix redefined-outer-name lint errors

* remove rules

* remove rules & fix pylint errors

* Remove all unused_var and fix other pylint errors

* reformatted by black

* [CI] Apply linting rules to onnx tests

* Disable unused-argument is for some unused-argument in function can not be removed

* Fix ci errors

* reformatted by black

* Fix ci errors

* Fix invalid-name/unused-variable,redefined-builtin

* [CI] Apply linting rules to caffe tests

* [CI] Apply linting rules to darknet tests

* [CI] Apply linting rules to pytorch tests

* [CI] Apply linting rules to tflite tests

* reformat by black

* [Pylint] Making frontend tests pylint compliant Part 1 of N

* Fix ci errors

* Fix invalid-name pylint errors

* Fix invalid-name pylint errors

* Disabale temporarily

* Fix dangerous-default-value

* Fix typo errors

* Fix ci errors

* [CI] Apply linting rules to tensorflow tests

* Fix dangerous-default-value

* Fix ungrouped-imports

* Fix boolean value of Tensor with more than one value is ambiguous

* Fix 'NoneType' object has no attribute 'shape'

* Fix boolean value of Tensor with more than one value is ambiguous

* reformatted by black

* [CI] Apply linting rules to caffe tests

* Fix dangerous-default-value

* Fix conflict & fix pylint error

* restore

Co-authored-by: Sebastian Boblest <sebastian.boblest@etas.com>
Co-authored-by: Andrew Reusch <areusch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants