[Pylint][microTVM] Applying Pylint rules to unittest folder #12063

alanmacd · 2022-07-12T00:11:17Z

Apply Pylint to the following files for issue #11414

tests/python/unittest/test_crt.py
tests/python/unittest/test_micro_model_library_format.py
tests/python/unittest/test_micro_project_api.py
tests/python/unittest/test_micro_transport.py
tests/python/unittest/test_node_reflection.py
tests/python/unittest/test_runtime_container.py
tests/python/unittest/test_runtime_error.py
tests/python/unittest/test_runtime_extension.py
tests/python/unittest/test_runtime_graph.py
tests/python/unittest/test_runtime_graph_cuda_graph.py
tests/python/unittest/test_runtime_graph_debug.py
tests/python/unittest/test_runtime_measure.py
tests/python/unittest/test_runtime_module_based_interface.py
tests/python/unittest/test_runtime_module_export.py
tests/python/unittest/test_runtime_module_load.py
tests/python/unittest/test_runtime_profiling.py
tests/python/unittest/test_runtime_rpc.py
tests/python/unittest/test_runtime_trace.py
tests/python/unittest/test_runtime_vm_profiler.py

Notes:

retained some variable names when it would either make code less readable or there were confusing redundant names that need deeper level of inspection or refactoring
some tvm.testing functions remain problematic
whitelisted variable names seem arbitrary, e.g. x & y are allowed but not z?
some of these files require much more effort than anticipated, (apologies for the monster-PR), I wonder if we should scan them all and rank from least to most issues to maximize efforts

@areusch @Mousius

cc @gromero @mehrdadh

tests/python/unittest/test_runtime_graph.py

areusch

thanks @alanmacd , left some comments

tests/lint/pylint.sh

areusch · 2022-07-18T18:09:56Z

tests/python/unittest/test_crt.py

-enable_usmp, expect_exception = tvm.testing.parameters((True, True), (False, False))
-
-
+@pytest.mark.parametrize("enable_usmp,expect_exception", [(True, True), (False, False)])


@Lunderberg here's an instance where we needed to undo tvm.testing.parameters. i'd love to find way around doing that. we could consider making tvm.testing.parameters mutate the scope of its caller...i don't love that but it does handily solve the lint problems.

areusch · 2022-07-18T18:10:55Z

tests/python/unittest/test_micro_transport.py

 import tvm.testing

+# Currently, there isn't a reasonable way to use @tvm.testing.fixture and not


possible to put this on line 36 to tie to just to transport?

tests/python/unittest/test_node_reflection.py

areusch · 2022-07-18T18:14:18Z

tests/python/unittest/test_runtime_extension.py

@@ -32,17 +34,18 @@ def _tvm_handle(self):


 def test_dltensor_compatible():
+    "Test DLTensor compatibility"


nit: i think taht should be """ """-style comment, could you see if we're missing a pylint rule?

good catch, yes that was a typo. I also confirmed pylint doesn't catch triple quote vs one quote strings, only catches if docstring is missing.

It may be only in a newer version of pylint?
https://pylint.pycqa.org/en/latest/user_guide/messages/convention/bad-docstring-quotes.html

areusch · 2022-07-18T18:18:04Z

tests/python/unittest/test_runtime_rpc.py

@@ -191,9 +204,9 @@ def check(remote):

        remote.cpu().sync()
        with pytest.raises(AttributeError):
-            f3 = remote.system_lib()["notexist"]
+            func3 = remote.system_lib()["notexist"]  # pylint: disable=unused-variable


can we just delete the func3 =?

I didn't know if we wanted to change functionality of test, i.e. why is this here, would that throw an exception if something was broken? or is it just abandoned code?

areusch · 2022-07-18T18:18:19Z

tests/python/unittest/test_runtime_rpc.py

@@ -302,15 +318,15 @@ def check_minrpc():
        a = tvm.nd.array(np.random.uniform(size=102).astype(A.dtype), dev)
        b = tvm.nd.array(np.zeros(102, dtype=A.dtype), dev)
        time_f = f1.time_evaluator("myadd", remote.cpu(0), number=1)
-        cost = time_f(a, b).mean
+        cost = time_f(a, b).mean  # pylint: disable=unused-variable


can we just rm cost =?

yes, I wasn't sure whether to change functionality of test, although in this case probably fine to do so, I figured marking it will flag it for future test developers who are more familiar with code to decide. This one's probably pretty safe to delete though.

areusch · 2022-07-18T18:18:39Z

tests/python/unittest/test_runtime_rpc.py

    # start server
    server = rpc.Server(key="x1")
    client = rpc.connect("127.0.0.1", server.port, key="x1")

    m = client.get_function("rpc.test.remote_return_nd")
    get_arr = m("get_arr")
-    ref_count = m("ref_count")
+    ref_count = m("ref_count")  # pylint: disable=unused-variable


consider removing ref_count =

tests/python/unittest/test_runtime_trace.py

…ower-case single-letter entries; remove related pylint disables;

github-actions bot requested review from gromero and mehrdadh July 12, 2022 00:14

alanmacd added 21 commits July 11, 2022 17:23

pylint test_crt.py

d87c0ad

pylint test_micro_model_library_format.py

23e483d

pylint test_micro_project_api.py

eab24cd

pylint test_micro_transport.py

87e231c

pylint test_node_reflection.py

aaf4923

pylint test_runtime_container.py

e999b58

pylint test_runtime_error.py

7e35939

pylint test_runtime_extension.py

54e5701

pylint test_runtime_graph.py

fd6ad0e

pylint test_runtime_graph_cuda_graph.py

aaa65f6

pylint test_runtime_measure.py

7ea8061

pylint test_runtime_module_based_interface.py

a411983

pylint test_runtime_module_export.py

33a95ec

pylint test_runtime_module_load.py

f188f1b

pylint test_runtime_profiling.py

2f451f7

pylint test_runtime_rpc.py

e303957

pylint test_runtime_trace.py

22cfd7b

pylint test_runtime_vm_profiler.py

9d08280

pylint test_runtime_graph_debug.py

37c4c30

update pylint.sh with new filenames

f8f7c72

pytest.mark.parametrize instead of tvm.testing.parameters

3f4487f

alanmacd force-pushed the pylint-unitest-runtime branch from c3a31ca to 3f4487f Compare July 12, 2022 00:25

alanmacd commented Jul 12, 2022

View reviewed changes

tests/python/unittest/test_runtime_graph.py Outdated Show resolved Hide resolved

fix pylint disable

41b6c5e

alanmacd force-pushed the pylint-unitest-runtime branch from 777ac36 to eeef2b6 Compare July 12, 2022 17:35

fix tmv.rpc

032271c

alanmacd force-pushed the pylint-unitest-runtime branch from eeef2b6 to 032271c Compare July 12, 2022 20:59

areusch reviewed Jul 18, 2022

View reviewed changes

alanmacd and others added 4 commits July 18, 2022 11:42

address PR feedback for pylint.sh

0a048dc

fix docstring

e9277da

updated pylintrc good-names to be alphabetized and add other common l…

0d7077d

…ower-case single-letter entries; remove related pylint disables;

Merge branch 'main' into pylint-unitest-runtime

a7628d7

areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pylint][microTVM] Applying Pylint rules to unittest folder #12063

[Pylint][microTVM] Applying Pylint rules to unittest folder #12063

alanmacd commented Jul 12, 2022 •

edited

areusch left a comment

areusch Jul 18, 2022

areusch Jul 18, 2022

areusch Jul 18, 2022

alanmacd Jul 18, 2022

alanmacd Jul 18, 2022

areusch Jul 18, 2022

alanmacd Jul 18, 2022

areusch Jul 18, 2022

alanmacd Jul 18, 2022

areusch Jul 18, 2022

		enable_usmp, expect_exception = tvm.testing.parameters((True, True), (False, False))


		@pytest.mark.parametrize("enable_usmp,expect_exception", [(True, True), (False, False)])

		import tvm.testing

		# Currently, there isn't a reasonable way to use @tvm.testing.fixture and not

		@@ -32,17 +34,18 @@ def _tvm_handle(self):


		def test_dltensor_compatible():
		"Test DLTensor compatibility"

[Pylint][microTVM] Applying Pylint rules to unittest folder #12063

Are you sure you want to change the base?

[Pylint][microTVM] Applying Pylint rules to unittest folder #12063

Conversation

alanmacd commented Jul 12, 2022 • edited

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanmacd commented Jul 12, 2022 •

edited