Skip to content

Enable typechecking for torch.futures #41675

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.

Sign up for GitHub

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

Jump to bottom
Closed
malfet wants to merge 1 commit into from
Closed

Enable typechecking for torch.futures #41675

malfet wants to merge 1 commit into from

Conversation

@malfet
Copy link
Contributor

@malfet malfet commented Jul 20, 2020

Add typing declarations for torch._C.Future and torch._C._collect_all

@malfet malfet requested review from ezyang, rgommers and xush6528 July 20, 2020 14:45
malfet
malfet commented Jul 20, 2020
View reviewed changes
facebook-github-bot
facebook-github-bot reviewed Jul 20, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dr-ci
Copy link

dr-ci bot commented Jul 20, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 6dd7cda (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This 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.

See how this bot performed.

This comment has been revised 40 times.

mrshenli
mrshenli reviewed Jul 20, 2020
View reviewed changes
Copy link
Contributor

@mrshenli mrshenli left a comment

Choose a reason for hiding this comment

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

Thanks for adding type checking

@malfet malfet force-pushed the malfet/enable-futures-typing branch from 6a79061 to 4729ad8 Compare July 20, 2020 16:50
xush6528
xush6528 reviewed Jul 20, 2020
View reviewed changes
@malfet malfet force-pushed the malfet/enable-futures-typing branch 3 times, most recently from 1dbe803 to 5f8031b Compare July 21, 2020 00:04
facebook-github-bot
facebook-github-bot reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@malfet malfet requested review from mrshenli and xush6528 July 21, 2020 13:56
mrshenli
mrshenli reviewed Jul 21, 2020
View reviewed changes
torch/futures/__init__.py Outdated
Copy link
Contributor

@mrshenli mrshenli Jul 21, 2020

Choose a reason for hiding this comment

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

The first argument type should be Future[T] instead of T?

Copy link
Contributor

@mrshenli mrshenli Jul 21, 2020

Choose a reason for hiding this comment

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

Do we need new tests to verify types?

Copy link
Contributor

@ezyang ezyang Jul 22, 2020

Choose a reason for hiding this comment

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

Umm, so you seem to be right as far as the code is concerned... but why does callback take a future? That doesn't make any sense!

Copy link
Contributor Author

@malfet malfet Jul 22, 2020

Choose a reason for hiding this comment

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

@mrshenli Added test/test_futures.py to the list of type-checked code, so those types of typos are now detected. Also, added #41826 that actually enables running test_futures.py as part of CI system.
But receiving future in the callback looks a bit unusual.

Copy link
Contributor

@ezyang ezyang Jul 23, 2020

Choose a reason for hiding this comment

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

(unresolved the convo to make sure @mrshenli can see it)

@malfet malfet force-pushed the malfet/enable-futures-typing branch from 5f8031b to 49aa3d2 Compare July 22, 2020 21:20
@malfet malfet requested a review from mrshenli July 22, 2020 23:53
facebook-github-bot
facebook-github-bot reviewed Jul 23, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@malfet malfet force-pushed the malfet/enable-futures-typing branch from 49aa3d2 to 5a266ad Compare July 23, 2020 00:48
mrshenli
mrshenli approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@mrshenli mrshenli left a comment

Choose a reason for hiding this comment

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

Stamp to unblock, I just have a minor comment on collect_all/wait_all types.

@malfet malfet force-pushed the malfet/enable-futures-typing branch 2 times, most recently from 01ecc9f to 5b4a650 Compare July 23, 2020 22:00
facebook-github-bot
facebook-github-bot reviewed Jul 23, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@malfet
Enable typechecking for torch.futures
6dd7cda 
Add typing declarations for torch._C.Future and torch._C._collect_all
@malfet malfet force-pushed the malfet/enable-futures-typing branch from 5b4a650 to 6dd7cda Compare July 24, 2020 03:21
facebook-github-bot
facebook-github-bot reviewed Jul 24, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 24, 2020

@malfet merged this pull request in c0bfa45.

@malfet malfet deleted the malfet/enable-futures-typing branch July 24, 2020 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@mrshenli mrshenli mrshenli approved these changes

@ezyang ezyang Awaiting requested review from ezyang

@rgommers rgommers Awaiting requested review from rgommers

@xush6528 xush6528 Awaiting requested review from xush6528

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@malfet @facebook-github-bot @ezyang @xush6528 @mrshenli @mruberry