[microTVM] Refactor pytest fixtures #12207

mehrdadh · 2022-07-27T19:49:02Z

Refactoring pytest fixtures between multiple directories. Using this change, we can reuse the same fixtures in external repositories
cc @alanmacd @gromero @guberti

guberti

I like these changes - especially the work to reduce redundancy between Arduino and Zephyr. However, I'm skeptical about the choice to move our testing infrastructure to a pytest plugin. I could be convinced, but I'd love to better understand the justification.

guberti · 2022-07-29T19:27:05Z

python/tvm/micro/testing/evaluation.py

+                "relay.ext.cmsisnn.options": {"mcpu": target.mcpu},
+            }
+        ):
+            mod = cmsisnn.partition_for_cmsisnn(mod, params, mcpu=target.mcpu)


Let's put this into the ExitStack() block below - remove likes 98-106 and insert:

mod = cmsisnn.partition_for_cmsisnn(mod, params, mcpu=target.mcpu)

in the if use_cmsiss_nn block

guberti · 2022-07-29T19:30:11Z

python/tvm/micro/testing/evaluation.py

@@ -153,4 +163,4 @@ def evaluate_model_accuracy(session, aot_executor, input_data, true_labels, runs
    num_correct = sum(u == v for u, v in zip(true_labels, predicted_labels))
    average_time = sum(aot_runtimes) / len(aot_runtimes)
    accuracy = num_correct / len(predicted_labels)
-    return average_time, accuracy
+    return average_time, accuracy, predicted_labels


Why are we changing this to return predicted_labels? There are a few cases where we'll want to override evaluate_model_accuracy (say, with the MLPerf Tiny model for anomaly detection) but there won't be a 1:1 correspondence of samples to predicted_labels (because anomaly detection uses an area of the curve metric). I'd prefer to keep it as is.

I made this change since we were using the predicted labels outside of this function for validity check in some tests. If you think it's out of the scope of this function, I can replicated the whole function in the other repo. thoughts?

Ah, I'd forgotten we need the labels for some hardware in the loop tests. That seems fine - for anomaly detection and other AOC metrics using the same "template", we could use confidence values (or even just None) in place of predicted_labels. This LGTM now.

guberti · 2022-07-29T19:30:42Z

python/tvm/micro/testing/pytest_plugin.py

+from tvm.contrib.utils import tempdir
+
+
+def zephyr_boards() -> dict:


Why are we treating zephyr_boards specially? I'd rather we use get_boards("zephyr") in all those cases.

that was left over, thanks for catching!

guberti · 2022-07-29T19:32:24Z

python/tvm/micro/testing/pytest_plugin.py

+
+
+def zephyr_boards() -> dict:
+    """Returns a dict mapping board to target model"""


We already have a function for this purpose - get_supported_boards in tvm/python/tvm/micro/testing/utils.py. Let's either move that or use it instead.

removed get_boards

guberti · 2022-07-29T19:33:23Z

python/tvm/micro/testing/pytest_plugin.py

+    return boards_model
+
+
+def get_boards(platform: str) -> dict:


See above comment. This should be removed, and get_supported_boards inside tvm/python/tvm/micro/testing/utils.py should be used instead.

removed get_boards

guberti · 2022-07-29T19:44:27Z

python/tvm/micro/testing/pytest_plugin.py

+    )
+
+
+@pytest.fixture(scope="module")


I'd rather if all the microTVM fixtures lived in tests/micro, where they have existed previously. I don't think they can be reused very easily - if someone wanted to do additional testing elsewhere, they would't need test-build-only or similar fixtures.

guberti · 2022-07-29T19:45:20Z

tests/micro/arduino/conftest.py

-        required=True,
-        choices=ARDUINO_BOARDS.keys(),
-        help="Arduino board for tests.",
-    )
    parser.addoption(
        "--arduino-cli-cmd",


Is it worth abstracting this parameter to "build tool path"? I could be convinced either way.

I think this way is more clear, specially when used in tvmc

yea you’re right, build tool is just too abstract.

guberti · 2022-07-29T19:45:56Z

tests/micro/arduino/test_arduino_workflow.py

 # Since these tests are sequential, we'll use the same project/workspace
 # directory for all tests in this file
 @pytest.fixture(scope="module")
-def workspace_dir(request, board):
+def workflow_workspace_dir(request, board):


Like this change!

guberti · 2022-07-29T19:47:06Z

tests/micro/zephyr/test_zephyr.py

@@ -89,7 +89,7 @@ def _make_add_sess(temp_dir, model, zephyr_board, west_cmd, build_config, dtype=
 # The same test code can be executed on both the QEMU simulation and on real hardware.
 @tvm.testing.requires_micro
 @pytest.mark.skip_boards(["mps2_an521"])
-def test_add_uint(temp_dir, board, west_cmd, tvm_debug):
+def test_add_uint(workspace_dir, board, west_cmd, tvm_debug):


For this and all the other tests - why do we have to use workspace_dir? If it's going to be reused every time, it would probably be cleaner to use the Python builtin fixutre temp_dir?

workspace_dir is a custom path directory where it is in the tvm workspace, so user can easily access them even when they are testing in docker. Also, we have tvm-debug which we can use to keep the project for debugging purposes. I think it's a useful tool for developers.

Huh, I’d forgotten that workspace_dir is kept when debug is passed. That sounds super useful - good call with this!

guberti · 2022-07-29T19:50:03Z

python/tvm/micro/testing/pytest_plugin.py

+    return request.config.getoption("--board")
+
+
+@pytest.fixture(scope="module")


Also, any fixtures that are only based on command line arguments should be session scoped IMO. They can't change during the session, so I'd argue it is more appropriate.

I think that makes sense. I changed them

mehrdadh

@guberti thanks for the review!
I addressed your comments. Regarding moving the pytest plugin into python package, I think they are useful for reusing in Hardware in the loop testing for microtvm. Duplications are always a pain to maintain and this PR tries to remove those duplications. In addition, the pytest features that we included here are mostly generic for microtvm testing in any environment.

mehrdadh · 2022-07-29T21:31:50Z

python/tvm/micro/testing/evaluation.py

@@ -153,4 +163,4 @@ def evaluate_model_accuracy(session, aot_executor, input_data, true_labels, runs
    num_correct = sum(u == v for u, v in zip(true_labels, predicted_labels))
    average_time = sum(aot_runtimes) / len(aot_runtimes)
    accuracy = num_correct / len(predicted_labels)
-    return average_time, accuracy
+    return average_time, accuracy, predicted_labels


I made this change since we were using the predicted labels outside of this function for validity check in some tests. If you think it's out of the scope of this function, I can replicated the whole function in the other repo. thoughts?

mehrdadh · 2022-07-29T21:32:21Z

python/tvm/micro/testing/pytest_plugin.py

+from tvm.contrib.utils import tempdir
+
+
+def zephyr_boards() -> dict:


that was left over, thanks for catching!

mehrdadh · 2022-07-29T21:35:33Z

python/tvm/micro/testing/pytest_plugin.py

+
+
+def zephyr_boards() -> dict:
+    """Returns a dict mapping board to target model"""


removed get_boards

mehrdadh · 2022-07-29T21:39:00Z

python/tvm/micro/testing/pytest_plugin.py

+    return boards_model
+
+
+def get_boards(platform: str) -> dict:


removed get_boards

mehrdadh · 2022-07-29T21:41:10Z

python/tvm/micro/testing/pytest_plugin.py

+def pytest_addoption(parser):
+    """Adds more pytest arguments"""
+    parser.addoption(
+        "--board",


added more details

mehrdadh · 2022-07-29T21:42:09Z

python/tvm/micro/testing/pytest_plugin.py

+    return request.config.getoption("--board")
+
+
+@pytest.fixture(scope="module")


I think that makes sense. I changed them

mehrdadh · 2022-07-29T21:44:25Z

tests/micro/arduino/conftest.py

-        required=True,
-        choices=ARDUINO_BOARDS.keys(),
-        help="Arduino board for tests.",
-    )
    parser.addoption(
        "--arduino-cli-cmd",


I think this way is more clear, specially when used in tvmc

mehrdadh · 2022-07-29T21:46:24Z

tests/micro/zephyr/test_zephyr.py

@@ -89,7 +89,7 @@ def _make_add_sess(temp_dir, model, zephyr_board, west_cmd, build_config, dtype=
 # The same test code can be executed on both the QEMU simulation and on real hardware.
 @tvm.testing.requires_micro
 @pytest.mark.skip_boards(["mps2_an521"])
-def test_add_uint(temp_dir, board, west_cmd, tvm_debug):
+def test_add_uint(workspace_dir, board, west_cmd, tvm_debug):


workspace_dir is a custom path directory where it is in the tvm workspace, so user can easily access them even when they are testing in docker. Also, we have tvm-debug which we can use to keep the project for debugging purposes. I think it's a useful tool for developers.

guberti

Your reasoning for moving to a pytest plugin makes sense to me - this approach now makes sense to me. LGTM!

guberti · 2022-07-30T12:40:27Z

tests/micro/arduino/conftest.py

-        required=True,
-        choices=ARDUINO_BOARDS.keys(),
-        help="Arduino board for tests.",
-    )
    parser.addoption(
        "--arduino-cli-cmd",


yea you’re right, build tool is just too abstract.

guberti · 2022-07-30T12:41:24Z

tests/micro/zephyr/test_zephyr.py

@@ -89,7 +89,7 @@ def _make_add_sess(temp_dir, model, zephyr_board, west_cmd, build_config, dtype=
 # The same test code can be executed on both the QEMU simulation and on real hardware.
 @tvm.testing.requires_micro
 @pytest.mark.skip_boards(["mps2_an521"])
-def test_add_uint(temp_dir, board, west_cmd, tvm_debug):
+def test_add_uint(workspace_dir, board, west_cmd, tvm_debug):


Huh, I’d forgotten that workspace_dir is kept when debug is passed. That sounds super useful - good call with this!

guberti · 2022-07-30T15:08:07Z

python/tvm/micro/testing/evaluation.py

@@ -153,4 +163,4 @@ def evaluate_model_accuracy(session, aot_executor, input_data, true_labels, runs
    num_correct = sum(u == v for u, v in zip(true_labels, predicted_labels))
    average_time = sum(aot_runtimes) / len(aot_runtimes)
    accuracy = num_correct / len(predicted_labels)
-    return average_time, accuracy
+    return average_time, accuracy, predicted_labels


Ah, I'd forgotten we need the labels for some hardware in the loop tests. That seems fine - for anomaly detection and other AOC metrics using the same "template", we could use confidence values (or even just None) in place of predicted_labels. This LGTM now.

guberti · 2022-07-30T15:13:24Z

python/tvm/micro/testing/pytest_plugin.py

+
+
+@pytest.fixture(scope="session")
+def tvm_debug(request):


nit: I don't love the name tvm_debug if all this flag does is keep the project directory - IMO --keep-project-dir or --preserve-project makes more sense. If it does things besides this, we should document them in the help string.

it used to do other things, but not anymore. I will change the name.

let me correct myself, it also used in project generation config to show more logs in the build/test process. So I would change the description.

areusch

overall lgtm, left a couple of comments around naming

python/tvm/micro/testing/pytest_plugin.py

@guberti

* Refactor micro test fixtures * fix error * fix scope * address @guberti comments * fix help message * rename tvm_debug and added .gitignore * fix bug * fix bug

github-actions bot requested a review from gromero July 27, 2022 19:50

mehrdadh force-pushed the micro/refactor_pytest branch from cdbdc8d to 12fcaae Compare July 27, 2022 21:18

Refactor micro test fixtures

0b6e33a

mehrdadh force-pushed the micro/refactor_pytest branch from 12fcaae to 0b6e33a Compare July 28, 2022 16:07

mehrdadh added 2 commits July 28, 2022 17:21

fix error

2cfd878

fix scope

b416bd3

guberti requested changes Jul 29, 2022

View reviewed changes

mehrdadh commented Jul 29, 2022

View reviewed changes

address @guberti comments

d9c774c

mehrdadh force-pushed the micro/refactor_pytest branch from 87cd1bb to d9c774c Compare July 29, 2022 21:55

guberti approved these changes Jul 30, 2022

View reviewed changes

fix help message

989facd

areusch reviewed Aug 1, 2022

View reviewed changes

python/tvm/micro/testing/pytest_plugin.py Show resolved Hide resolved

python/tvm/micro/testing/pytest_plugin.py Show resolved Hide resolved

python/tvm/micro/testing/pytest_plugin.py Show resolved Hide resolved

mehrdadh added 3 commits August 1, 2022 09:51

rename tvm_debug and added .gitignore

d5c0ba5

fix bug

1c51152

fix bug

4493112

areusch approved these changes Aug 3, 2022

View reviewed changes

areusch merged commit 6f83113 into apache:main Aug 3, 2022

mehrdadh deleted the micro/refactor_pytest branch August 3, 2022 18:10

guberti mentioned this pull request Aug 22, 2022

[microTVM] Replace static fixtures with parameterization #12530

Merged

AndrewZhaoLuo mentioned this pull request Oct 4, 2022

TVM v0.10.0.rc0 Release Candidate Notes #12979

Closed

		from tvm.contrib.utils import tempdir


		def zephyr_boards() -> dict:



		def zephyr_boards() -> dict:
		"""Returns a dict mapping board to target model"""

		return request.config.getoption("--board")


		@pytest.fixture(scope="module")

[microTVM] Refactor pytest fixtures #12207

[microTVM] Refactor pytest fixtures #12207

Conversation

mehrdadh commented Jul 27, 2022 • edited Loading

guberti 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

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

mehrdadh 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

guberti 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

areusch left a comment

Choose a reason for hiding this comment

mehrdadh commented Jul 27, 2022 •

edited

Loading