Lunderberg commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855492678
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
Review Comment:
Also, I think this default is required for some use cases. The
`load_module()` function doesn't call `set_device_type`, so I expect the
TE-based tests to fail. It is also legal to upload a module, and later refer
to the uploaded module by its name. Since this can occur on an entirely
different instance of TVM, there wouldn't be a way to determine the device
type, and we can't avoid needing a default.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
Review Comment:
I'd recommend either removing the `hasattr(self, "_device")` from this
condition, or removing `self._device = None` from the initializer. As it is,
there are two different ways to represent an uninitialized session, which could
cause confusion in the future.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -226,6 +244,28 @@ def get_executor_from_factory(self, module:
ExecutorFactoryModule):
raise TypeError(f"Unsupported executor type: {type(module)}")
+ def set_device_type(self, module: Union[str, pathlib.Path,
GraphExecutorFactoryModule]):
Review Comment:
Should `set_device_type` be a private function? If it is only needed from
the AOT path, and should only be called from within
`_get_aot_executor_from_factory`, we should rename to `_set_device_type`.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
+ assert (
+ False
+ ), "Device type is not set. 'set_device_type' should be called
before accessing device."
Review Comment:
This error message instructs users to call `set_device_type`, which we may
want to be an internal function. The concern would be for future breakage, if
code outside this class calls `hexagon_session.set_device_type`, we wouldn't be
able to remove `set_device_type` without breaking that code. I think there are
some plans to remove the `kDLHexagon` type and instead use options within the
`kDLCPU`, at which point we'd want to remove the device choice without breaking
external usage.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
Review Comment:
Do we want to supply a default for `_requires_cpu_device`, rather than
requiring it to be explicitly set? I think general case is that we return a
"hexagon" device, and that AOT is the special case that would require a CPU
device. Rather than requiring it to be set for both paths, can we set
`self._requires_cpu_device = False` in the initializer, and only set it to True
in the AOT path. That way, the default is to return a "hexagon" device.
Also, as a general nitpick, I'd recommend `assert(condition)` instead of `if
not condition: assert(False)`.
--
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]
