Lunderberg commented on code in PR #13161:
URL: https://github.com/apache/tvm/pull/13161#discussion_r1002172526
##########
python/tvm/contrib/hexagon/build.py:
##########
@@ -102,14 +101,9 @@ class HexagonLauncherRPC(metaclass=abc.ABCMeta):
The basic flow of interaction with the launcher is
launcher = HexagonLauncher(...)
launcher.start_server()
- with launcher.start_session() as session:
+ with session.create_session() as session:
Review Comment:
Is there a missing line in this example? I don't see where the outer
`session` variable is defined in order to call `session.create_session()`.
##########
python/tvm/contrib/hexagon/meta_schedule.py:
##########
@@ -100,7 +101,7 @@ def run(self, runner_inputs: List[RunnerInput]) ->
List[RunnerFuture]:
def _worker_func(hexagon_launcher, evaluator_config, alloc_repeat,
artifact_path, args_info):
- with hexagon_launcher.start_session() as session:
+ with create_session(hexagon_launcher._workspace,
hexagon_launcher._rpc_info) as session:
Review Comment:
Why is `meta_schedule` accessing internal objects owned by the
`hexagon_launcher`?
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -393,3 +394,37 @@ def _aot_executor_from_factory(
remote_file_path = self.upload(binary_path, binary_name)
return self.get_aot_executor(remote_file_path)
+
+
+def create_session(
Review Comment:
Currently, all calls to `create_session` pass arguments owned by a
`HexagonLauncher` instance. The `_workspace` and `_rpc_info` objects are
marked as `_`, so these would be expected to be internal implementation
details, not part of the external API of the `HexagonLauncher`.
Do we expect to call `create_session` based on something in a workflow that
doesn't include `HexagonLauncher`? If yes, then both the free function
`hexagon.session.create_session` and the method
`HexagonLauncher.create_session` should exist, with
`HexagonLauncher.create_session` calling the free function instead of requiring
users to access private variables. If no, then only the `HexagonLauncher`
method should exist.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -393,3 +394,37 @@ def _aot_executor_from_factory(
remote_file_path = self.upload(binary_path, binary_name)
return self.get_aot_executor(remote_file_path)
+
+
+def create_session(
+ workspace: Union[str, pathlib.Path], rpc_info: dict, session_name: str =
"hexagon-rpc"
+) -> Session:
+ """Create an RPC session.
+
+ Parameters
+ ----------
+ workspace : Union[str, pathlib.Path]
+ Workspace path on remote device.
+
+ rpc_info : dict
+ RPC tracker information.
+
+ session_name : str
+ RPC session name.
+
+ Returns
+ -------
+ Session :
+ The session object.
+ """
+ assert "rpc_tracker_host" in rpc_info.keys()
+ assert "rpc_tracker_port" in rpc_info.keys()
+
+ hexagon_remote_kw = {
+ "host": rpc_info["rpc_tracker_host"],
+ "port": rpc_info["rpc_tracker_port"],
+ "priority": 0,
+ "timeout": 0,
+ "key": rpc_info["device_key"],
+ }
+ return Session(workspace, hexagon_remote_kw, session_name=session_name)
Review Comment:
Alternatively, since the only purpose of this function is to initialize a
`Session` object, why not move the free function into `Session.__init__`?
##########
python/tvm/contrib/hexagon/build.py:
##########
@@ -689,18 +556,19 @@ def start_server(self):
self._copy_to_remote(lib_dir / item, self._workspace / item)
# Copy libc++ from the toolchain to the workspace
self._copy_libcxx(self._workspace)
- self._device_key = self.HEXAGON_REMOTE_DEVICE_KEY + "." +
str(os.getpid())
+ self._rpc_info["device_key"] = HEXAGON_REMOTE_DEVICE_KEY + "." +
str(os.getpid())
Review Comment:
Is it allowed for multiple simultaneous servers to be started from the same
launcher? If so, this would overwrite the previous server that had been
started. If not, we should verify that no server is already running.
##########
python/tvm/contrib/hexagon/build.py:
##########
@@ -378,13 +239,16 @@ def __init__(
if not rpc_info.get("workspace_base"):
rpc_info["workspace_base"] = self.ANDROID_HEXAGON_TEST_BASE_DIR
self._serial_number = serial_number
+ assert self._serial_number != ""
+
adb_socket = rpc_info["adb_server_socket"] if
rpc_info["adb_server_socket"] else "tcp:5037"
self._adb_device_sub_cmd = ["adb", "-L", adb_socket, "-s",
self._serial_number]
self.forwarded_ports_ = []
self._hexagon_debug = hexagon_debug
self._clear_logcat = clear_logcat
self._sysmon_profile = sysmon_profile
self._sysmon_process = None
+ rpc_info["device_key"] = HEXAGON_REMOTE_DEVICE_KEY + "." +
self._serial_number
Review Comment:
Do we ever pass the same `rpc_info` dictionary passed into multiple
`HexagonLauncherAndroid` objects? (e.g. Launching kernels on each of N
devices?) If so, they would all share a single dictionary object, and the each
object would clobber the `rpc_info['device_key']` value of previous launchers.
This might be something better left for a standalone PR, but I'm wondering
if `rpc_info` be passed as individual arguments instead of a dictionary? That
way, even if we construct the same dictionary for convenience, it wouldn't be a
shared instance. It would also be clearer for a user to determine what the
arguments should be.
##########
tests/python/contrib/test_hexagon/test_meta_schedule.py:
##########
@@ -62,7 +64,7 @@ def main( # type: ignore # pylint: disable=no-self-argument
@tvm.testing.requires_hexagon
def test_builder_runner(hexagon_launcher):
- if hexagon_launcher._serial_number == "simulator":
+ if android_serial_number()[0] == "simulator":
Review Comment:
To a casual reader, there's nothing that connects the
`android_serial_number` function to either the `hexagon_launcher` or the
`hexagon_session`. The current implementation also has incorrect behavior when
running tests on multiple devices/simulators. If
`ANDROID_SERIAL_NUMBER=abcd1234,simulator`, the test would be erroneously run
on the simulator. If `ANDROID_SERIAL_NUMBER=simulator,abcd1234`, the test
would be erroneously skipped on the device.
Instead of requiring user code to call a free function, can we instead have
`HexagonLauncher.is_simulator()` and/or `HexagonSession.is_simulator()`
methods? That way, it's clear to the user that this is specific to the
connected device, and not something that is globally true.
--
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]
