Skip to content

Exception formatting: handle case where module is None#2011

Merged
Jasha10 merged 1 commit intofacebookresearch:mainfrom
Jasha10:i2010
Oct 10, 2022
Merged

Exception formatting: handle case where module is None#2011
Jasha10 merged 1 commit intofacebookresearch:mainfrom
Jasha10:i2010

Conversation

@Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Feb 3, 2022

Fix an issue where Hydra's exception-handling logic could raise an AssertionError if the module from which in the exception's originates cannot be determined.

Closes #2010. Closes #2342.
See #2342 for a way to reproduce the bug.

Details:

Hydra's exception-handling logic automatically removes omegaconf-related stack frames before printing the handled traceback. This requires inspecting the stack frames (when an exception arises) to determine which stack frames belong to OmegaConf. Hydra uses the function inspect.getmodule to determine the module to which a given stack frame belongs.

In the case where the module from which a stack frame originated cannot be determined, inspect.getmodule returns None. Previously, this would cause Hydra to throw an AssertionError. This PR adds logic to handle the case where the module cannot be determined, avoiding the AssertionError.

@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 Feb 3, 2022
@Jasha10 Jasha10 marked this pull request as draft February 3, 2022 22:46
@Jasha10 Jasha10 marked this pull request as ready for review February 3, 2022 23:04
@jieru-hu
Copy link
Contributor

jieru-hu commented Feb 8, 2022

could you paste an example before and after? :)

@Jasha10 Jasha10 marked this pull request as draft March 11, 2022 19:21
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Apr 16, 2022

I'm closing this PR for now as I'm no longer able to reproduce the original issue.

Edit: see #2342 for a way to reproduce the bug.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Aug 16, 2022

I'm reopening this PR as per discussion in #2343.
The issue can be reproduced using the code from issue #2342.

@Jasha10 Jasha10 reopened this Aug 16, 2022
@Jasha10 Jasha10 marked this pull request as ready for review September 22, 2022 21:20
@Jasha10 Jasha10 merged commit 9ce6720 into facebookresearch:main Oct 10, 2022
@Jasha10 Jasha10 deleted the i2010 branch October 10, 2022 02:31
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.

[Bug] Hydra's exception formatting cannot handle exception raised in C extension [Bug] AssertionError in multithread context

4 participants