-
-
Notifications
You must be signed in to change notification settings - Fork 988
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
Fix typing of poutine.trace(model).get_trace() #3334
Conversation
fn: Callable[_P, _T] = ..., | ||
fn: Callable[_P, _T], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove the default to avoid the mypy error:
pyro/poutine/handlers.py:469: error: Overloaded function signatures 1 and 2 overlap with incompatible return types [overload-overlap]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, seems like some weird behavior from mypy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes total sense. Should we also do this for the ReparamHandler
even though it doesn't have any methods defined?
fn: Callable[_P, _T] = ..., | ||
fn: Callable[_P, _T], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, seems like some weird behavior from mypy
Yes, I think we should be as precise as is practical. Beyond mypy correctness checking, the propagated type hints are used by vs code to show documentation on hover. So if we can narrow the type hint to a specific class, vs code will show the documentation of that variable. I've been using hover-docs more often than sphinx docs lately. I'll go ahead and make the remaining changes, then re-submit for review. Thanks for taking a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
Addresses #3332 #2550
This proposes to replace handler types with more explicit handlers:
so mypy can understand code like
poutine.trace(model).get_trace()
.If @ordabayevy approves of this change, I can similarly change all handlers in this PR.