Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve instantiate handling of exceptions raised by user code #1916

Merged
merged 4 commits into from
Dec 15, 2021

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Dec 5, 2021

This PR fixes a bug that arises when instantiate is invoked on a callable that raises a custom exception that has a non-standard signature.

Background info about the bug:

This PR closes #1911.

A call to instantiate({"_target_": target_callable, **kwargs}) works by locating the target_callable and calling it on the given arguments **kwargs.
The call to target_callable is performed inside of the hydra._internal.instantiate._instantiate2._call_target function:

def _call_target(_target_: Callable, *args, **kwargs) -> Any:  # type: ignore
    """Call target (type) with args and kwargs."""
    try:
        ...
        return _target_(*args, **kwargs)
    except Exception as e:
        raise type(e)(
            f"Error instantiating '{_convert_target_to_string(_target_)}' : {e}"
        ).with_traceback(sys.exc_info()[2])

This _call_target function has a try/except block that catches exceptions that might be raisd by the target_callable.
In the _call_target function's except block, another exception is raised with an error message giving context about the exception. The exception raised in the except block is of the same type as the exception that was originally caught.
This is problematic because we must assume that the exception class type(e) can be called with one argument; we are making the assumption here that type(e).__init__ accepts a string error message as it's first and only argument.
This assumption fails if the user's code raises a custom exception whose __init__ function takes a non-standard number of arguments (as was the case in bug report #1911).

Proposed Changes

This PR fixes the reported bug by not raising an exception of type(e) if something goes wrong with the call to _target_.
Instead, an instance of hydra.errors.InstantiationException is raised from e (a lá exception chaining).

Since this PR changes the type of exception that will be raised if user code throws an exception, it should be considered an API change.

Commits:

  • 2505eda: Add failing tests
  • a2dbbf8: raise InstantiationException instead of type(e)
  • 247a3ed: add news fragment
  • 95494ee: update test regex patterns for python3.6 compatibility.

I'm not planning to merge this PR until after #1905 and #1915 are finished, as those PRs also touch code related to instantiate.

@facebook-github-bot 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 Dec 5, 2021
@Jasha10 Jasha10 marked this pull request as ready for review December 5, 2021 22:39
@rsokl
Copy link
Contributor

rsokl commented Dec 11, 2021

This will be very useful for improving compatibility with Hydra + pydantic! Currently, this pydantic-based example hits this exact issue, because the validation exception class that pydantic raises does not use a standard __init__ (although I don't draw attention to it in the example, since it would distract/confuse the reader).

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Dec 13, 2021

Nice!

Copy link
Contributor

@jieru-hu jieru-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! :)

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Dec 15, 2021

Rebased against main to resolve merge conflict.

@Jasha10 Jasha10 merged commit 5f605a8 into facebookresearch:main Dec 15, 2021
@Jasha10 Jasha10 deleted the closes1911 branch December 15, 2021 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instantiation exception also raises an exception sometimes
4 participants