guberti commented on code in PR #12207: URL: https://github.com/apache/tvm/pull/12207#discussion_r933564689
########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: + """Returns a dict mapping board to target model""" Review Comment: 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. ########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects("zephyr")) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def get_boards(platform: str) -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects(platform)) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def pytest_addoption(parser): + """Adds more pytest arguments""" + parser.addoption( + "--board", Review Comment: We should also document explicitly the difference between `platform`s and `board`s in our usage - it might not be obvious to others. ########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: Review Comment: Why are we treating `zephyr_boards` specially? I'd rather we use `get_boards("zephyr")` in all those cases. ########## 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 Review Comment: 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. ########## python/tvm/micro/testing/evaluation.py: ########## @@ -94,6 +95,15 @@ def create_aot_session( executor = tvm.relay.backend.Executor("aot") crt_runtime = tvm.relay.backend.Runtime("crt", {"system-lib": True}) + if use_cmsis_nn: + with tvm.transform.PassContext( + config={ + "tir.disable_vectorize": True, + "relay.ext.cmsisnn.options": {"mcpu": target.mcpu}, + } + ): + mod = cmsisnn.partition_for_cmsisnn(mod, params, mcpu=target.mcpu) Review Comment: 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 ########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects("zephyr")) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def get_boards(platform: str) -> dict: Review Comment: See above comment. This should be removed, and `get_supported_boards` inside `tvm/python/tvm/micro/testing/utils.py` should be used instead. ########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects("zephyr")) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def get_boards(platform: str) -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects(platform)) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def pytest_addoption(parser): + """Adds more pytest arguments""" + parser.addoption( + "--board", + required=True, + choices=list(get_boards("zephyr").keys()) + list(get_boards("arduino").keys()), + help="microTVM boards for tests", + ) + parser.addoption( + "--test-build-only", + action="store_true", + help="Only run tests that don't require physical hardware.", + ) + parser.addoption( + "--tvm-debug", + action="store_true", + default=False, + help="If set true, it will keep the project directory for debugging.", + ) + + [email protected](scope="module") +def board(request): + return request.config.getoption("--board") + + [email protected](scope="module") Review Comment: 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. ########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects("zephyr")) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def get_boards(platform: str) -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects(platform)) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def pytest_addoption(parser): + """Adds more pytest arguments""" + parser.addoption( + "--board", Review Comment: Love this change! It has been a long time in the workds. ########## tests/micro/arduino/test_arduino_workflow.py: ########## @@ -36,23 +36,22 @@ 6. Use serial connection to ensure model behaves correctly """ - # 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): Review Comment: Like this change! ########## tests/micro/arduino/conftest.py: ########## @@ -15,35 +15,19 @@ # specific language governing permissions and limitations # under the License. -import pytest +pytest_plugins = [ + "tvm.micro.testing.pytest_plugin", +] -from test_utils import ARDUINO_BOARDS +import pytest def pytest_addoption(parser): - parser.addoption( - "--arduino-board", - nargs="+", - required=True, - choices=ARDUINO_BOARDS.keys(), - help="Arduino board for tests.", - ) parser.addoption( "--arduino-cli-cmd", Review Comment: Is it worth abstracting this parameter to "build tool path"? I could be convinced either way. ########## python/tvm/micro/testing/pytest_plugin.py: ########## @@ -0,0 +1,126 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# 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 +""" microTVM testing fixtures used to deduce testing argument + values from testing parameters """ + +import pathlib +import json +import os +import datetime +import pytest + +import tvm +from tvm.contrib.utils import tempdir + + +def zephyr_boards() -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects("zephyr")) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def get_boards(platform: str) -> dict: + """Returns a dict mapping board to target model""" + with open( + pathlib.Path(tvm.micro.get_microtvm_template_projects(platform)) / "boards.json" + ) as f: + board_properties = json.load(f) + + boards_model = {board: info["model"] for board, info in board_properties.items()} + return boards_model + + +def pytest_addoption(parser): + """Adds more pytest arguments""" + parser.addoption( + "--board", + required=True, + choices=list(get_boards("zephyr").keys()) + list(get_boards("arduino").keys()), + help="microTVM boards for tests", + ) + parser.addoption( + "--test-build-only", + action="store_true", + help="Only run tests that don't require physical hardware.", + ) + parser.addoption( + "--tvm-debug", + action="store_true", + default=False, + help="If set true, it will keep the project directory for debugging.", + ) + + [email protected](scope="module") Review Comment: 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. ########## 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): Review Comment: 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`? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
