[GitHub] [tvm] Mousius commented on a change in pull request #9074: Improve tvmc error message from lazy-loading frontend imports

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

Mousius commented on a change in pull request #9074:
URL: https://github.com/apache/tvm/pull/9074#discussion_r756018356



##########
File path: python/tvm/driver/tvmc/main.py
##########
@@ -91,6 +92,11 @@ def _main(argv):
 
     try:
         return args.func(args)
+    except TVMCImportError as err:
+        sys.stderr.write(
+            f'Package "{err.message}" is not installed. ' f'Hint: "pip install tlcpack[tvmc]".'

Review comment:
       I got a final error of `AttributeError: 'TVMCImportError' object has no attribute 'message'` when I ran this, so I think it should just be: 
   ```suggestion
               f'Package "{err}" is not installed. ' f'Hint: "pip install tlcpack[tvmc]".'
   ```

##########
File path: tests/python/driver/tvmc/test_frontends.py
##########
@@ -367,3 +383,39 @@ def _is_layout_transform(node):
     tvm.relay.analysis.post_order_visit(after["main"], _is_layout_transform)
 
     assert not any(layout_transform_calls), "Unexpected 'layout_transform' call"
+
+
+def test_import_keras_friendly_message(keras_resnet50, monkeypatch):
+    # keras is part of tensorflow
+    monkeypatch.setattr("importlib.import_module", mock_error_on_name("tensorflow"))
+
+    with pytest.raises(TVMCImportError) as e:
+        _ = tvmc.frontends.load_model(keras_resnet50, model_format="keras")

Review comment:
       We should check the error message is correct using the `match` parameter but also the variables aren't necessary:
   ```suggestion
       with pytest.raises(TVMCImportError, match='Package "keras" is not installed. Hint: "pip install tlcpack[tvmc]"'):
           vmc.frontends.load_model(keras_resnet50, model_format="keras")
   ```

##########
File path: python/tvm/driver/tvmc/frontends.py
##########
@@ -76,20 +78,19 @@ def load(self, path, shape_dict=None, **kwargs):
         """
 
 
-def import_keras():
-    """Lazy import function for Keras"""
-    # Keras writes the message "Using TensorFlow backend." to stderr
-    # Redirect stderr during the import to disable this
-    stderr = sys.stderr
-    sys.stderr = open(os.devnull, "w")
+def lazy_import(pkg_name, from_pkg_name=None, hide_stderr=False):
+    """Lazy import a frontend package or subpackage"""
     try:
-        # pylint: disable=C0415
-        import tensorflow as tf
-        from tensorflow import keras
+        if hide_stderr:
+            stderr = sys.stderr
+            sys.stderr = open(os.devnull, "w")
 
-        return tf, keras
+        return importlib.import_module(pkg_name, package=from_pkg_name)
+    except ImportError:
+        raise TVMCImportError({pkg_name})

Review comment:
       To avoid Python complaining about nested exceptions you have to explicitly state it, and I don't think this needs squiggles?
   ```suggestion
       except ImportError as error:
           raise TVMCImportError(pkg_name) from error
   ```

##########
File path: python/tvm/driver/tvmc/frontends.py
##########
@@ -76,20 +78,19 @@ def load(self, path, shape_dict=None, **kwargs):
         """
 
 
-def import_keras():
-    """Lazy import function for Keras"""
-    # Keras writes the message "Using TensorFlow backend." to stderr
-    # Redirect stderr during the import to disable this
-    stderr = sys.stderr
-    sys.stderr = open(os.devnull, "w")
+def lazy_import(pkg_name, from_pkg_name=None, hide_stderr=False):
+    """Lazy import a frontend package or subpackage"""
     try:
-        # pylint: disable=C0415
-        import tensorflow as tf
-        from tensorflow import keras
+        if hide_stderr:
+            stderr = sys.stderr
+            sys.stderr = open(os.devnull, "w")

Review comment:
       What does this suppress? When I run the command I still get errors:
   ```
   2021-11-24 12:27:41.514375: W tensorflow/stream_executor/platform/default/dso_loader.cc:60] Could not load dynamic library 'libcudart.so.11.0'; dlerror: libcudart.so.11.0: cannot open shared object file: No such file or directory
   2021-11-24 12:27:41.514406: I tensorflow/stream_executor/cuda/cudart_stub.cc:29] Ignore above cudart dlerror if you do not have a GPU set up on your machine.
   ```
   
   We also seem to always set `hide_stderr` to `True` so there's no need to branch?




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