Better error message in lazyconfig relative import

Summary:
Make the error messages in #4214 better

And add a few more tests for lazyconfig

Pull Request resolved: #4215

Reviewed By: tglik

Differential Revision: D39402746

fbshipit-source-id: 5d9cbfdbfd34b829e25ed1b27b6f2bf96e6ab1e9

Author: ppwwyyxx

INSTALL.md

@@ -247,8 +247,8 @@ whose version is closer to what's used by PyTorch (available in `torch.__config_
 "library not found for -lstdc++" on older version of MacOS
 </summary>
 <br/>
-See
-[this stackoverflow answer](https://stackoverflow.com/questions/56083725/macos-build-issues-lstdc-not-found-while-building-python-package).
+
+See [this stackoverflow answer](https://stackoverflow.com/questions/56083725/macos-build-issues-lstdc-not-found-while-building-python-package).
 
 </details>
 

detectron2/config/lazy.py

@@ -106,28 +106,41 @@ def _patch_import():
     1. locate files purely based on relative location, regardless of packages.
        e.g. you can import file without having __init__
     2. do not cache modules globally; modifications of module states has no side effect
-    3. support other storage system through PathManager
+    3. support other storage system through PathManager, so config files can be in the cloud
     4. imported dict are turned into omegaconf.DictConfig automatically
     """
     old_import = builtins.__import__
 
     def find_relative_file(original_file, relative_import_path, level):
+        # NOTE: "from . import x" is not handled. Because then it's unclear
+        # if such import should produce `x` as a python module or DictConfig.
+        # This can be discussed further if needed.
+        relative_import_err = """
+Relative import of directories is not allowed within config files.
+Within a config file, relative import can only import other config files.
+""".replace(
+            "\n", " "
+        )
+        if not len(relative_import_path):
+            raise ImportError(relative_import_err)
+
         cur_file = os.path.dirname(original_file)
         for _ in range(level - 1):
             cur_file = os.path.dirname(cur_file)
         cur_name = relative_import_path.lstrip(".")
         for part in cur_name.split("."):
             cur_file = os.path.join(cur_file, part)
-        # NOTE: directory import is not handled. Because then it's unclear
-        # if such import should produce python module or DictConfig. This can
-        # be discussed further if needed.
         if not cur_file.endswith(".py"):
             cur_file += ".py"
         if not PathManager.isfile(cur_file):
-            raise ImportError(
-                f"Cannot import name {relative_import_path} from "
-                f"{original_file}: {cur_file} has to exist."
-            )
+            cur_file_no_suffix = cur_file[: -len(".py")]
+            if PathManager.isdir(cur_file_no_suffix):
+                raise ImportError(f"Cannot import from {cur_file_no_suffix}." + relative_import_err)
+            else:
+                raise ImportError(
+                    f"Cannot import name {relative_import_path} from "
+                    f"{original_file}: {cur_file} does not exist."
+                )
         return cur_file
 
     def new_import(name, globals=None, locals=None, fromlist=(), level=0):

tests/config/dir1/bad_import.py

@@ -0,0 +1,2 @@
+# import from directory is not allowed
+from . import dir1a

tests/config/dir1/bad_import2.py

@@ -0,0 +1 @@
+from .does_not_exist import x

tests/config/dir1/load_rel.py

@@ -0,0 +1,5 @@
+# test that load_rel can work
+from detectron2.config import LazyConfig
+
+x = LazyConfig.load_rel("dir1_a.py", "dir1a_dict")
+assert x["a"] == 1

tests/config/test_lazy_config.py

@@ -10,7 +10,8 @@
 
 class TestLazyPythonConfig(unittest.TestCase):
     def setUp(self):
-        self.root_filename = os.path.join(os.path.dirname(__file__), "root_cfg.py")
+        self.curr_dir = os.path.dirname(__file__)
+        self.root_filename = os.path.join(self.curr_dir, "root_cfg.py")
 
     def test_load(self):
         cfg = LazyConfig.load(self.root_filename)
@@ -77,3 +78,18 @@ def test_to_py(self):
 cfg.list = ["a", 1, "b", 3.2]
 """
         self.assertEqual(py_str, expected)
+
+    def test_bad_import(self):
+        file = os.path.join(self.curr_dir, "dir1", "bad_import.py")
+        with self.assertRaisesRegex(ImportError, "relative import"):
+            LazyConfig.load(file)
+
+    def test_bad_import2(self):
+        file = os.path.join(self.curr_dir, "dir1", "bad_import2.py")
+        with self.assertRaisesRegex(ImportError, "not exist"):
+            LazyConfig.load(file)
+
+    def test_load_rel(self):
+        file = os.path.join(self.curr_dir, "dir1", "load_rel.py")
+        cfg = LazyConfig.load(file)
+        self.assertIn("x", cfg)