Improve `instantiate` error message in case where `_target_` cannot be loaded #1864

Jasha10 · 2021-10-22T08:53:18Z

This PR improves the error handling of hydra.utils.instantiate for the case
where _target_ cannot be properly resolved.

Closes #1863

Below is a summary of the changed error handling (with tracebacks condensed).

The case where a module cannot be found:

Before:

>>> instantiate({"_target_": "foo"})
Traceback (most recent call last):
  ...
ValueError: Empty module name
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Error loading module 'foo'

Above, note the confusing ValueError: Empty module name.

After:

>>> instantiate({"_target_": "foo"})
Traceback (most recent call last):
  ...
ModuleNotFoundError: No module named 'foo'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Error loading module 'foo'

The case where an attribute cannot be found on an object:

Before:

>>> instantiate({"_target_": "pickle.dumps.missing_attribute"})
  ...
ModuleNotFoundError: No module named 'pickle.dumps'; 'pickle' is not a package
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Encountered error: `No module named 'pickle.dumps'; 'pickle' is not a package` when loading module 'pickle.dumps.missing_attribute'

Note above the misleading ModuleNotFoundError: No module named 'pickle.dumps'; 'pickle' is not a package.

After:

>>> instantiate({"_target_": "pickle.dumps.missing_attribute"})
Traceback (most recent call last):
  ...
AttributeError: 'builtin_function_or_method' object has no attribute 'missing_attribute'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Encountered AttributeError while loading 'pickle.dumps.missing_attribute': 'builtin_function_or_method' object has no attribute 'missing_attribute'

The case where an attribute cannot be found on a module (and no submodule can be found with the correct name):

Before:

>>> instantiate({"_target_": "pickle.typo"})
Traceback (most recent call last):
  ...
ModuleNotFoundError: No module named 'pickle.typo'; 'pickle' is not a package
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Encountered error: `No module named 'pickle.typo'; 'pickle' is not a package` when loading module 'pickle.typo'

After:

>>> instantiate({"_target_": "pickle.typo"})
Traceback (most recent call last):
  ...
AttributeError: module 'pickle' has no attribute 'typo'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Encountered AttributeError while loading 'pickle.typo': module 'pickle' has no attribute 'typo'

The case where a module contains a syntax error or other error (as in #1863)

# my_module.py
class Foo  # syntax error
    print("Hi")

Before:

>>> instantiate({"_target_": "my_module.Foo"})
Traceback (most recent call last):
  ...
ValueError: Empty module name
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Error loading module 'my_module.Foo'

After:

>>> instantiate({"_target_": "my_module.Foo"})
Traceback (most recent call last):
  ...
  File "/home/jasha10/hydra.git/my_module.py", line 2
    class Foo  # syntax error
             ^
SyntaxError: invalid syntax
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  ...
ImportError: Error loading module 'my_module.Foo'

robogast · 2021-10-22T10:14:49Z

hydra/_internal/utils.py

        except Exception as e:
-            if n == 0:
+            if n == 1:


What do you think of:

for n in range(len(parts), 0, -1): mod = ".".join(parts[:n]) try: obj = import_module(mod) except Exception as e: continue break else: raise ImportError(f"Error loading module '{path}'") from e

The reversed statement can be incorporated in range

The real try statement is around import_module; it's not intended for ".".join(parts[:n]) to raise an exception (that would be a bug). So you can move the mod = ... before the try.

You can replace if n == 1: with the use of the for/else statement https://book.pythontips.com/en/latest/for_-_else.html#else-clause

If you replace the if n == 1: with for/else you can even replace n in ... mod = ... with:

for mod in ( ".".join(parts[:n]) for n in range(len(parts), 0, -1) ):

Just commenting because I like the code puzzle :), feel free to ignore it if you want.

Edit:
Ah apparently the exception e gets cleared when exiting the except: https://docs.python.org/3.3/reference/compound_stmts.html#the-try-statement, so then in the else you have to do something like:

except Exception as e: _e = e ... else: raise ImportError(f"Error loading module '{path}'") from _e

But I think that's a bit ugly 😅

Thanks for your review @robogast!

Edit: Ah apparently the exception e gets cleared when exiting the except

I ran into this same issue when I was working on the PR 😆

1 & 4. Yes, but I think that reversed is slightly easier to understand for future readers -- There is already a lot going on with the indices here :)
2. Agreed! Updated in 5de3754.
3. I wonder why python clears the exception e after the except clause... Too bad :P

Previously (before this PR), an ImportError was raised if a submodule mod_a.mod_b could not be found and module and also mod_a has no attribute named `mod_b`. An earlier commit in this PR changed this ImportError behavior to raise an AttributeError instead. This commit reverts that behavior -- now once again an ImportError is raised.

robogast · 2021-10-25T11:44:59Z

hydra/_internal/utils.py

-            if n == 0:
-                raise ImportError(f"Error loading module '{path}'") from e
+            obj = import_module(mod)
+        except Exception as exc_import:


Also; as far as I can find, import_module should only throw an ImportError, so maybe change the generic Exception to ImportError?

This except clause also catches errors raised by the module that's being imported (e.g. if the imported module contains a line that says assert False).

Another option here would be to just catch ImportError and let the exceptions from mod bubble up to the user, but that would be potentially breaking change to Hydra's API.

A change for Hydra 2.0 then :)

Good idea :)

robogast · 2021-10-25T11:46:36Z

hydra/_internal/utils.py

+                    import_module(mod)
+                except ModuleNotFoundError:
+                    pass
+                except Exception as exc_import:


Same comment as above, and as ModuleNotFoundError is already handled above, catching the more generic ImportError here should be OK, as import_module should only throw an ImportError.

Same comment as above -- exceptions raised by mod itself (rather than by import_module) are caught here.

jieru-hu

lgtm! thanks for improving this. only one small nit.

jieru-hu · 2021-10-26T00:22:19Z

tests/instantiate/test_helpers.py

+        param(
+            "datetime",
+            raises(
+                ValueError,
+                match=re.escape("Invalid type (<class 'module'>) found for datetime"),
+            ),
+            id="top_level_module",
+        ),


nice - we can probably remove/update the comment in line 600 in hydra/_internal/utils.py

(github won't let me comment on that line)

Good catch! Updated in e716b17.

Jasha10 added 4 commits October 22, 2021 03:18

improvements to _locate

1e8ca7c

retain behavior from main: don't instantiate builtins

729a04b

improve AttributeError handling

72d7ed7

refactor

34f8b3b

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2021

Jasha10 added 2 commits October 22, 2021 03:54

add news fragment

ffd0935

lint

a314db0

robogast reviewed Oct 22, 2021

View reviewed changes

Jasha10 marked this pull request as draft October 22, 2021 11:21

Jasha10 added 2 commits October 22, 2021 06:29

further polish error messages

0c3df6b

refactor

5de3754

Jasha10 marked this pull request as ready for review October 22, 2021 11:49

Jasha10 added 2 commits October 25, 2021 06:36

lint

5510c79

robogast reviewed Oct 25, 2021

View reviewed changes

jieru-hu approved these changes Oct 26, 2021

View reviewed changes

update comment in _locate

e716b17

Jasha10 merged commit 043ec9b into facebookresearch:main Nov 1, 2021

Jasha10 deleted the closes1863 branch November 1, 2021 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `instantiate` error message in case where `_target_` cannot be loaded #1864

Improve `instantiate` error message in case where `_target_` cannot be loaded #1864

Jasha10 commented Oct 22, 2021 •

edited

Loading

robogast Oct 22, 2021 •

edited

Loading

Jasha10 Oct 22, 2021

Jasha10 Oct 22, 2021

robogast Oct 25, 2021

Jasha10 Oct 25, 2021 •

edited

Loading

robogast Oct 26, 2021

Jasha10 Oct 28, 2021

robogast Oct 25, 2021

Jasha10 Oct 25, 2021

jieru-hu left a comment

jieru-hu Oct 26, 2021

Jasha10 Oct 28, 2021

Improve instantiate error message in case where _target_ cannot be loaded #1864

Improve instantiate error message in case where _target_ cannot be loaded #1864

Conversation

Jasha10 commented Oct 22, 2021 • edited Loading

The case where a module cannot be found:

The case where an attribute cannot be found on an object:

The case where an attribute cannot be found on a module (and no submodule can be found with the correct name):

The case where a module contains a syntax error or other error (as in #1863)

robogast Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasha10 Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieru-hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve `instantiate` error message in case where `_target_` cannot be loaded #1864

Improve `instantiate` error message in case where `_target_` cannot be loaded #1864

Jasha10 commented Oct 22, 2021 •

edited

Loading

robogast Oct 22, 2021 •

edited

Loading

Jasha10 Oct 25, 2021 •

edited

Loading