[GitHub] [tvm] mehrdadh commented on a diff in pull request #11547: [Hexagon] Run single RPC server on Android in each testing session

[GitBox <gi...@apache.org>]

mehrdadh commented on code in PR #11547:
URL: https://github.com/apache/tvm/pull/11547#discussion_r889353770


##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -194,33 +196,36 @@ def get_graph_executor(
         self._set_device_type(graph_mod)
         return tvm.contrib.graph_executor.create(graph_json, graph_mod, self.device)
 
-    def get_aot_executor(
+    def get_graph_debug_executor(

Review Comment:
   Unfortunately github didn't present this correctly. I removed `get_aot_executor` function because it didn't work correctly and also it was not used anywhere in the codebase. Therefore it will remain broken if we keep it and others might try to use it.



##########
python/tvm/contrib/hexagon/pytest_plugin.py:
##########
@@ -173,19 +180,51 @@ def hexagon_launcher(
         rpc_info = {
             "rpc_tracker_host": tvm_tracker_host,
             "rpc_tracker_port": tvm_tracker_port,
-            "rpc_server_port": rpc_server_port,
+            "rpc_server_port": rpc_server_port_for_session,
             "adb_server_socket": adb_server_socket,
         }
         launcher = HexagonLauncher(serial_number=android_serial_number, rpc_info=rpc_info)
-        launcher.start_server()
+
         try:
+            launcher.start_server()
             yield launcher
         finally:
             launcher.stop_server()
 
 
-@tvm.testing.fixture
-def hexagon_session(hexagon_launcher) -> Session:
+@pytest.fixture
+def hexagon_launcher(
+    hexagon_server_process,
+    rpc_server_port,
+    tvm_tracker_host,
+    tvm_tracker_port,
+    adb_server_socket,
+    android_serial_number,
+) -> HexagonLauncherRPC:
+    """Initials and returns hexagon launcher which reuses RPC info and Android serial number."""
+    if hexagon_server_process._serial_number != "simulator":

Review Comment:
   I think simulator is more dependent to have all the execution files in the same workspace and since it was already working fine, I tried not to change it. We could also change that in this PR or a follow up PR.



##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -194,33 +196,36 @@ def get_graph_executor(
         self._set_device_type(graph_mod)
         return tvm.contrib.graph_executor.create(graph_json, graph_mod, self.device)
 
-    def get_aot_executor(
+    def get_graph_debug_executor(

Review Comment:
   Also I changed `get_graph_debug_executor` to work with the current approach. Just realized we are missing a test for that as well. I suggest to add a test for it since this api is useful for debugging.



##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -109,7 +110,7 @@ def device(self):
 
         return self._device
 
-    def upload(self, local_path: Union[str, pathlib.Path], remote_filename: str):
+    def upload(self, local_path: Union[str, pathlib.Path], remote_filename: str) -> pathlib.Path:

Review Comment:
   I need this change to make this approach work. It is used in `load_module` function.



-- 
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