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

Display correct error message and cleanup _PYRO_STACK when error happens #818

Merged
merged 3 commits into from
Nov 19, 2020

Conversation

fehiepsi
Copy link
Member

Fixes #815.

Currently, if an error happens inside an message handler, sometimes we got the error

~/miniconda3/lib/python3.7/site-packages/numpyro/primitives.py in __exit__(self, *args, **kwargs)
     56     def __exit__(self, *args, **kwargs):
---> 57         assert _PYRO_STACK[-1] is self
     58         _PYRO_STACK.pop()

FilteredStackTrace: AssertionError

which is not meaningful and _PYRO_STACK is not cleared when it happens. After incorporating Pyro's exit mechanism, the error becomes

~/numpyro/numpyro/contrib/funsor/enum_messenger.py in _pyro_post_to_data(self, msg)
    392         if msg["kwargs"]["dim_type"] in (DimType.GLOBAL, DimType.VISIBLE):
    393             for name in msg["args"][0].inputs:
--> 394                 self._saved_globals += ((name, _DIM_STACK.global_frame.name_to_dim[name]),)
    395 
    396 

KeyError: 'p'

which is more meaningful and PYRO_STACK is cleared after the error.

@eb8680
Copy link
Member

eb8680 commented Nov 19, 2020

Can you add a regression test that would have caught this using the example from #815?

monkeypatch.setattr(numpyro.infer.util, "_validate_model", lambda model_trace: None)
with pytest.raises(Exception):
mcmc.run(random.PRNGKey(2), data)
assert len(_PYRO_STACK) == 0
Copy link
Member Author

Choose a reason for hiding this comment

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

@eb8680 Before this PR, assert len(_PYRO_STACK) == 0 fails (7 vs 0).

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

LGTM

@fehiepsi
Copy link
Member Author

Thanks for reviewing, @eb8680!

@fehiepsi fehiepsi merged commit 970dc48 into pyro-ppl:master Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatible shapes error after stack error
2 participants