-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Add typing annotations to hub.py and _jit_internal.py #42252
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
Conversation
💊 CI failures summary and remediationsAs of commit 0cf851e (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 19 times. |
hameerabbasi
left a comment
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.
One minor, optional comment.
| frame = inspect.currentframe() | ||
| i = 0 | ||
| while i < frames_up + 1: | ||
| assert frame is not None |
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.
Does this not work if you put the assert on the line right below inspect.currentframe()? (it wouldn't surprise me if not, mypy is fragile with this kind of type narrowing)
Technically assert's are used for asserting things that must be true. In this case that is the case (because the reason for None is the below), so the change is correct - just unfortunate the same assert must be used twice and inside a for-loop.
CPython implementation detail: This function relies on Python stack frame support in the interpreter, which isn’t guaranteed to exist in all implementations of Python. If running in an implementation without Python stack frame support this function returns None.
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.
Yeah, unfortunately it doesn't work. If I move the assertion to the line below current_frame = inspect.currentframe(), mypy raises an error.
rgommers
left a comment
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, thanks @guilhermeleobas
|
Good stuff. Do you want to handle the CR comments before I merge? |
Yes, I will address them later today. |
|
Okay all comments addressed, this should be good to land. |
facebook-github-bot
left a comment
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.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
xref: https://github.com/pytorch/pytorch/wiki/Guide-for-adding-type-annotations-to-PyTorch