areusch commented on a change in pull request #9074:
URL: https://github.com/apache/tvm/pull/9074#discussion_r728244569
##########
File path: python/tvm/driver/tvmc/frontends.py
##########
@@ -88,10 +96,54 @@ def import_keras():
from tensorflow import keras
return tf, keras
+ except ImportError:
Review comment:
aside from the sys.stderr redirection above and keras/tf dual-import
below, i almost think these `import_` functions are similar enough we should
consolidate them into one and remove `create_import_error_string` (e.g. just do
it from the one place). what do you think about doing this? i could be
persuaded either way, but it seems like `import_frontend_package("onnx")` could
be implemented like:
```
def import_frontend_package(pkg_name : str, from_pkg_name=None :
Optional[str], hide_stderr=False : bool) -> Module:
try:
if hide_stderr:
stderr = sys.stderr
sys.stderr = open(os.devnull, "w")
return importlib.import_module(pkg_name, from=from_pkg_naem)
except ImportError:
raise TVMCException(f"Error importing required frontend package
{pkg_name}")
finally:
if hide_stderr:
sys.stderr = stderr
```
another property i like about this is that it's very clear (once you know
the pattern) exactly what's being passed to "importlib.import_module()" whereas
the current structure could change between e.g. import_keras, import_onnx, etc.
##########
File path: python/tvm/driver/tvmc/frontends.py
##########
@@ -88,10 +96,54 @@ def import_keras():
from tensorflow import keras
return tf, keras
+ except ImportError:
+ raise TVMCException(create_import_error_string("Tensorflow",
"tensorflow"))
finally:
sys.stderr = stderr
+def import_onnx():
+ """Lazy import function for onnx"""
+ try:
+ # pylint: disable=C0415
Review comment:
i think (but am not 100% sure) that including this on its own line
disables that check for the whole file. possible to move it onto the next line?
```suggestion
import onnx as _onnx # pylint: disable=C0415
```
##########
File path: tests/python/driver/tvmc/test_frontends.py
##########
@@ -211,3 +225,49 @@ def
test_load_model___wrong_language__to_pytorch(tflite_mobilenet_v1_1_quant):
model_format="pytorch",
shape_dict={"input": [1, 3, 224, 224]},
)
+
+
+def test_import_keras_friendly_message(keras_resnet50, monkeypatch):
+ # some CI environments wont offer TFLite, so skip in case it is not present
Review comment:
can you fix the tflite part of the comment?
##########
File path: python/tvm/driver/tvmc/frontends.py
##########
@@ -88,10 +96,54 @@ def import_keras():
from tensorflow import keras
return tf, keras
+ except ImportError:
+ raise TVMCException(create_import_error_string("Tensorflow",
"tensorflow"))
finally:
sys.stderr = stderr
+def import_onnx():
+ """Lazy import function for onnx"""
+ try:
+ # pylint: disable=C0415
+ import onnx as _onnx
Review comment:
wondering if there is a reason to keep "as _onnx" (and other "as") here?
did it raise some linter issues?
--
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]
