Lunderberg commented on code in PR #11547:
URL: https://github.com/apache/tvm/pull/11547#discussion_r890215211
##########
python/tvm/contrib/hexagon/build.py:
##########
@@ -217,10 +233,7 @@ def load_module(self, module: Union[str, pathlib.Path,
tvm.runtime.Module], sess
session and loaded.
If the object passed is a string or pathlib.Path, it must
- be either a bare file name (without any path components),
- or a full path in the remote system. If it is a file name,
- the file must already have been uploaded to the remote,
- and be placed in the remote workspace.
+ be a full path in the remote system.
Review Comment:
I like the symmetry of loading a file using the same path as returned from
`upload`, but are there cases where we'd want to work within the existing
workspace, without requiring the full path? The use cases coming to mind would
be uploading a model once, then running several independent tests on it.
Within a single process, the model to run should be passed to each independent
target, so this change wouldn't affect it. If the upload happens in one
process, then the independent tests happen in subsequent processes, it might
cause an issue, but then the workspace itself would need to be passed through.
So I don't think I see any major concerns, but reducing previously
functional behavior does give me pause.
##########
python/tvm/contrib/hexagon/build.py:
##########
@@ -58,7 +62,9 @@ def _get_hexagon_rpc_lib_dir() -> pathlib.Path:
def _get_test_directory_name() -> str:
"""Generate a time-stamped name for use as a test directory name."""
- return datetime.datetime.now().strftime("%Y-%m-%dT%H-%M-%S")
+ date_str = datetime.datetime.now().strftime("%Y-%m-%dT%H-%M-%S")
Review Comment:
While I think this is a good change to add to ensure unique folders even if
a test concludes in less than a second, I think we should separate it out into
an independent 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:
I think we should add a test for the `get_aot_executor` function, rather
than removing it entirely. The current tests use `get_executor_from_factory`,
which calls into `_aot_executor_from_factory`. This is exactly what we need
for CI, where each model is uploaded once and run once, but isn't as useful
when a model is uploaded once and run many times, potentially across multiple
executions.
The `get_aot_executor` path is intended to support that use case, where an
AOT module has been compiled and uploaded, and in each future occurrence only
needs to be loaded.
##########
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:
Is this related to the change from allowing just filenames to requiring a
full remote path, so that the caller would have somewhere from which they could
learn the full remove path?
--
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]
