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/04/18 20:04:59 UTC
[GitHub] [tvm] guberti opened a new pull request, #11043: Better version handling for Arduino
guberti opened a new pull request, #11043:
URL: https://github.com/apache/tvm/pull/11043
This pull request contains three small changes:
- Fix bug allowing `microTVM` to be used with Arduino version `v0.20` and above (see changes to `_parse_connected_boards`) and adds relevant unit tests.
- Only perform version check when calling `build` or `flash` (things that actually require `arduino-cli`), and adds relevant unit tests.
- Only raise a warning if the `arduino-cli` version present is **below** the min version (previously any version other than `v0.18` would cause an error).
--
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] gromero commented on a diff in pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #11043:
URL: https://github.com/apache/tvm/pull/11043#discussion_r853238754
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,11 +347,44 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
# Recursively change includes
self._convert_includes(project_dir, source_dir)
+ def _get_arduino_cli_cmd(self, options: dict):
+ arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+ assert arduino_cli_cmd, "'arduino_cli_cmd' command not passed and not found by default!"
+ return arduino_cli_cmd
+
+ def _get_platform_version(self, arduino_cli_path: str) -> float:
+ # sample output of this command:
+ # 'arduino-cli alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z\n'
+ version_output = subprocess.run(
+ [arduino_cli_path, "version"], check=True, stdout=subprocess.PIPE
+ ).stdout.decode("utf-8")
+
+ full_version = re.findall(r"version: ([\.0-9]*)", version_output.lower())
+ full_version = full_version[0].split(".")
+ version = float(f"{full_version[0]}.{full_version[1]}")
+ return version
+
+ # This will only be run for build and upload
+ def _check_platform_version(self, options):
+ if not self._version:
+ cli_command = self._get_arduino_cli_cmd(options)
+ self._version = self._get_platform_version(cli_command)
+
+ if self._version < MIN_ARDUINO_CLI_VERSION:
+ message = (
+ f"Arduino CLI version too old: found {self._version}, "
+ f"need at least {ARDUINO_CLI_VERSION}."
+ )
+ if options.get("warning_as_error") is not None and options["warning_as_error"]:
+ raise server.ServerError(message=message)
+ _LOG.warning(message)
+
def _get_fqbn(self, options):
o = BOARD_PROPERTIES[options["arduino_board"]]
return f"{o['package']}:{o['architecture']}:{o['board']}"
def build(self, options):
+ self._check_platform_version(options)
Review Comment:
Now that `warning_as_error` option has no effect on `generate_project` but on `build()` and `flash()`, could you please adapt "ProjectOption" in the server to list `build` and `flash` in the `optional` field instead of `generate_project`? This mostly useful for the CLI that use the Project API, like `tvmc`.
##########
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py:
##########
@@ -105,9 +106,8 @@ def test_auto_detect_port(self, sub_mock):
assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
# Test it raises an exception when no board is connected
- sub_mock.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V18, "utf-8")
- with pytest.raises(microtvm_api_server.BoardAutodetectFailed):
- handler._auto_detect_port(self.DEFAULT_OPTIONS)
+ sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")
Review Comment:
I think now comment "# Test it raises an exception when no board is connected" in line 108 must go to above line 112?
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,11 +347,44 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
# Recursively change includes
self._convert_includes(project_dir, source_dir)
+ def _get_arduino_cli_cmd(self, options: dict):
+ arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+ assert arduino_cli_cmd, "'arduino_cli_cmd' command not passed and not found by default!"
+ return arduino_cli_cmd
+
+ def _get_platform_version(self, arduino_cli_path: str) -> float:
+ # sample output of this command:
+ # 'arduino-cli alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z\n'
+ version_output = subprocess.run(
+ [arduino_cli_path, "version"], check=True, stdout=subprocess.PIPE
+ ).stdout.decode("utf-8")
+
+ full_version = re.findall(r"version: ([\.0-9]*)", version_output.lower())
+ full_version = full_version[0].split(".")
+ version = float(f"{full_version[0]}.{full_version[1]}")
+ return version
+
+ # This will only be run for build and upload
+ def _check_platform_version(self, options):
+ if not self._version:
+ cli_command = self._get_arduino_cli_cmd(options)
+ self._version = self._get_platform_version(cli_command)
+
+ if self._version < MIN_ARDUINO_CLI_VERSION:
+ message = (
+ f"Arduino CLI version too old: found {self._version}, "
+ f"need at least {ARDUINO_CLI_VERSION}."
Review Comment:
This now needs to be `MIN_ARDUINO_CLI_VERSION`
--
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] guberti commented on a diff in pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #11043:
URL: https://github.com/apache/tvm/pull/11043#discussion_r853437525
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,11 +347,44 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
# Recursively change includes
self._convert_includes(project_dir, source_dir)
+ def _get_arduino_cli_cmd(self, options: dict):
+ arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+ assert arduino_cli_cmd, "'arduino_cli_cmd' command not passed and not found by default!"
+ return arduino_cli_cmd
+
+ def _get_platform_version(self, arduino_cli_path: str) -> float:
+ # sample output of this command:
+ # 'arduino-cli alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z\n'
+ version_output = subprocess.run(
+ [arduino_cli_path, "version"], check=True, stdout=subprocess.PIPE
+ ).stdout.decode("utf-8")
+
+ full_version = re.findall(r"version: ([\.0-9]*)", version_output.lower())
+ full_version = full_version[0].split(".")
+ version = float(f"{full_version[0]}.{full_version[1]}")
+ return version
+
+ # This will only be run for build and upload
+ def _check_platform_version(self, options):
+ if not self._version:
+ cli_command = self._get_arduino_cli_cmd(options)
+ self._version = self._get_platform_version(cli_command)
+
+ if self._version < MIN_ARDUINO_CLI_VERSION:
+ message = (
+ f"Arduino CLI version too old: found {self._version}, "
+ f"need at least {ARDUINO_CLI_VERSION}."
+ )
+ if options.get("warning_as_error") is not None and options["warning_as_error"]:
+ raise server.ServerError(message=message)
+ _LOG.warning(message)
+
def _get_fqbn(self, options):
o = BOARD_PROPERTIES[options["arduino_board"]]
return f"{o['package']}:{o['architecture']}:{o['board']}"
def build(self, options):
+ self._check_platform_version(options)
Review Comment:
Fixed!
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,11 +347,44 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
# Recursively change includes
self._convert_includes(project_dir, source_dir)
+ def _get_arduino_cli_cmd(self, options: dict):
+ arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+ assert arduino_cli_cmd, "'arduino_cli_cmd' command not passed and not found by default!"
+ return arduino_cli_cmd
+
+ def _get_platform_version(self, arduino_cli_path: str) -> float:
+ # sample output of this command:
+ # 'arduino-cli alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z\n'
+ version_output = subprocess.run(
+ [arduino_cli_path, "version"], check=True, stdout=subprocess.PIPE
+ ).stdout.decode("utf-8")
+
+ full_version = re.findall(r"version: ([\.0-9]*)", version_output.lower())
+ full_version = full_version[0].split(".")
+ version = float(f"{full_version[0]}.{full_version[1]}")
+ return version
+
+ # This will only be run for build and upload
+ def _check_platform_version(self, options):
+ if not self._version:
+ cli_command = self._get_arduino_cli_cmd(options)
+ self._version = self._get_platform_version(cli_command)
+
+ if self._version < MIN_ARDUINO_CLI_VERSION:
+ message = (
+ f"Arduino CLI version too old: found {self._version}, "
+ f"need at least {ARDUINO_CLI_VERSION}."
Review Comment:
Fixed, and added a unit test
--
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] guberti commented on pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
guberti commented on PR #11043:
URL: https://github.com/apache/tvm/pull/11043#issuecomment-1103060463
Thanks for the comments @gromero! I've addressed them all, and made a small change to the way we compare versions (we now use `version.parse` like the rest of TVM).
--
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] gromero commented on a diff in pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #11043:
URL: https://github.com/apache/tvm/pull/11043#discussion_r853507006
##########
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py:
##########
@@ -98,36 +101,52 @@ def test_parse_connected_boards(self):
]
@mock.patch("subprocess.run")
- def test_auto_detect_port(self, sub_mock):
+ def test_auto_detect_port(self, mock_run):
process_mock = mock.Mock()
handler = microtvm_api_server.Handler()
# Test it returns the correct port when a board is connected
- sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
+ mock_run.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
- # Test it raises an exception when no board is connected
- sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")
+ # Should work with old or new arduino-cli version
+ mock_run.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")
assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
- # Should work with old or new arduino-cli version
- sub_mock.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V21, "utf-8")
+ # Test it raises an exception when no board is connected
+ mock_run.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V21, "utf-8")
with pytest.raises(microtvm_api_server.BoardAutodetectFailed):
handler._auto_detect_port(self.DEFAULT_OPTIONS)
# Test that the FQBN needs to match EXACTLY
handler._get_fqbn = mock.MagicMock(return_value="arduino:mbed_nano:nano33")
- sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
+ mock_run.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
assert (
handler._auto_detect_port({**self.DEFAULT_OPTIONS, "arduino_board": "nano33"})
== "/dev/ttyACM1"
)
- CLI_VERSION = "arduino-cli Version: 0.21.1 Commit: 9fcbb392 Date: 2022-02-24T15:41:45Z\n"
+ BAD_CLI_VERSION = "arduino-cli Version: 0.7.1 Commit: 7668c465 Date: 2019-12-31T18:24:32Z\n"
+ GOOD_CLI_VERSION = "arduino-cli Version: 0.21.1 Commit: 9fcbb392 Date: 2022-02-24T15:41:45Z\n"
+
+ @mock.patch("subprocess.run")
+ def test_auto_detect_port(self, mock_run):
+ handler = microtvm_api_server.Handler()
+ mock_run.return_value.stdout = bytes(self.GOOD_CLI_VERSION, "utf-8")
+ handler._check_platform_version(self.DEFAULT_OPTIONS)
+ assert handler._version == version.parse("0.21.1")
+
+ # Using too low a version should raise an error. Note that naively
Review Comment:
nit: the second part of this comment, about the naive comparison, seems to be a bit misplaced here, would it fit better in the test above for `GOOD_CLI_VERSION` instead (or even in `_get_platform_version` when `version.parse()` is used)?
--
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] gromero commented on pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
gromero commented on PR #11043:
URL: https://github.com/apache/tvm/pull/11043#issuecomment-1103214781
> > Finally, out of curiosity, do you know why these Arduino tests are kept in `template_project/tests` instead of in `tests/micro/arduino` with the other tests? Should we move everything into `tests/micro/arduino`?
>
> The original thinking was that unit tests would live in `template_project/tests`, while integration tests would live in `tests/micro/arduino`. I don't think this makes very much sense anymore, and I'd be open to moving everything to `/tests/micro/arduino`. That's probably OOS for this PR, though.
Got it! Yeah, that's OOS for this PR, I was just wondering about that organization. :+1:
--
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] gromero merged pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
gromero merged PR #11043:
URL: https://github.com/apache/tvm/pull/11043
--
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] guberti commented on a diff in pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #11043:
URL: https://github.com/apache/tvm/pull/11043#discussion_r853405740
##########
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py:
##########
@@ -105,9 +106,8 @@ def test_auto_detect_port(self, sub_mock):
assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
# Test it raises an exception when no board is connected
- sub_mock.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V18, "utf-8")
- with pytest.raises(microtvm_api_server.BoardAutodetectFailed):
- handler._auto_detect_port(self.DEFAULT_OPTIONS)
+ sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")
Review Comment:
Oops, fixed the comments
--
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] guberti commented on a diff in pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #11043:
URL: https://github.com/apache/tvm/pull/11043#discussion_r853523041
##########
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py:
##########
@@ -98,36 +101,52 @@ def test_parse_connected_boards(self):
]
@mock.patch("subprocess.run")
- def test_auto_detect_port(self, sub_mock):
+ def test_auto_detect_port(self, mock_run):
process_mock = mock.Mock()
handler = microtvm_api_server.Handler()
# Test it returns the correct port when a board is connected
- sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
+ mock_run.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
- # Test it raises an exception when no board is connected
- sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")
+ # Should work with old or new arduino-cli version
+ mock_run.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")
assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
- # Should work with old or new arduino-cli version
- sub_mock.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V21, "utf-8")
+ # Test it raises an exception when no board is connected
+ mock_run.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V21, "utf-8")
with pytest.raises(microtvm_api_server.BoardAutodetectFailed):
handler._auto_detect_port(self.DEFAULT_OPTIONS)
# Test that the FQBN needs to match EXACTLY
handler._get_fqbn = mock.MagicMock(return_value="arduino:mbed_nano:nano33")
- sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
+ mock_run.return_value.stdout = bytes(self.BOARD_CONNECTED_V18, "utf-8")
assert (
handler._auto_detect_port({**self.DEFAULT_OPTIONS, "arduino_board": "nano33"})
== "/dev/ttyACM1"
)
- CLI_VERSION = "arduino-cli Version: 0.21.1 Commit: 9fcbb392 Date: 2022-02-24T15:41:45Z\n"
+ BAD_CLI_VERSION = "arduino-cli Version: 0.7.1 Commit: 7668c465 Date: 2019-12-31T18:24:32Z\n"
+ GOOD_CLI_VERSION = "arduino-cli Version: 0.21.1 Commit: 9fcbb392 Date: 2022-02-24T15:41:45Z\n"
+
+ @mock.patch("subprocess.run")
+ def test_auto_detect_port(self, mock_run):
+ handler = microtvm_api_server.Handler()
+ mock_run.return_value.stdout = bytes(self.GOOD_CLI_VERSION, "utf-8")
+ handler._check_platform_version(self.DEFAULT_OPTIONS)
+ assert handler._version == version.parse("0.21.1")
+
+ # Using too low a version should raise an error. Note that naively
Review Comment:
Good point, it does make more sense in `_get_platform_version`. Moved.
--
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] guberti commented on pull request #11043: Better version handling for Arduino
Posted by GitBox <gi...@apache.org>.
guberti commented on PR #11043:
URL: https://github.com/apache/tvm/pull/11043#issuecomment-1103210666
> Finally, out of curiosity, do you know why these Arduino tests are kept in `template_project/tests` instead of in `tests/micro/arduino` with the other tests? Should we move everything into `tests/micro/arduino`?
The original thinking was that unit tests would live in `template_project/tests`, while integration tests would live in `tests/micro/arduino`. I don't think this makes very much sense anymore, and I'd be open to moving everything to `/tests/micro/arduino`. That's probably OOS for this PR, though.
--
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