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.

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

HenryJia: auto-move data decorator #1905

Merged
merged 39 commits into from
Jun 15, 2020
Merged

HenryJia: auto-move data decorator #1905

merged 39 commits into from
Jun 15, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented May 20, 2020 •
edited by Borda
Loading

What does this PR do?

Continuation of the work of @HenryJia from PR #1526, fixes #1412
This is currently based on PR #1756.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@mergify mergify bot requested a review from a team May 20, 2020 18:44
@awaelchli awaelchli force-pushed the feature/auto-move-data branch from ecb0dd3 to d33af6b Compare May 20, 2020 18:53
@Borda Borda changed the title HenryJia auto-move data decorator [blocked by #1756] HenryJia: auto-move data decorator May 22, 2020
@Borda Borda added duplicate This issue or pull request already exists feature Is an improvement or enhancement labels May 22, 2020
@awaelchli awaelchli mentioned this pull request May 25, 2020
Closed
5 tasks
@awaelchli awaelchli force-pushed the feature/auto-move-data branch from 06d5596 to fd658e6 Compare May 25, 2020 22:58
@Borda Borda changed the title [blocked by #1756] HenryJia: auto-move data decorator HenryJia: auto-move data decorator Jun 5, 2020
@Borda Borda marked this pull request as ready for review June 5, 2020 11:27
@mergify
Copy link
Contributor

mergify bot commented Jun 5, 2020

This pull request is now in conflict... :(

@Borda
Copy link
Member

Borda commented Jun 5, 2020

@awaelchli seems this is unblocked now, mind resolve conflicts? 🐰
@HenryJia mind check the update?

HenryJia
HenryJia approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@HenryJia HenryJia left a comment

Choose a reason for hiding this comment

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

Looks all good to me, though it may still be prudent to look into why the CircleCI build failed before merging

@awaelchli awaelchli force-pushed the feature/auto-move-data branch 2 times, most recently from 93a742e to 1bb4ed1 Compare June 6, 2020 23:36
@awaelchli awaelchli mentioned this pull request Jun 7, 2020
Closed
@awaelchli
Copy link
Contributor Author

Thanks for checking @HenryJia . I rebased and tests run except for one.
I added a doctest but it requires the GPU. It fails if the machine only has CPU.
I'm looking into that, I would really like to have that doctest. But otherwise PR is ready.

@Borda
Copy link
Member

Borda commented Jun 7, 2020

I'm looking into that, I would really like to have that doctest. But otherwise PR is ready.

I would like to have the doctest for it too, but maybe it would be easier to have the particular code just as an example and get this merge and later try to find a way to have it as doctest...

@Borda Borda removed the duplicate This issue or pull request already exists label Jun 7, 2020
@mergify mergify bot requested a review from a team June 7, 2020 09:36
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2020

This pull request is now in conflict... :(

@Borda
Copy link
Member

Borda commented Jun 8, 2020

@HenryJia @awaelchli could we get this done? 🦝

@awaelchli awaelchli force-pushed the feature/auto-move-data branch 2 times, most recently from 846ece4 to 1b2a56d Compare June 8, 2020 17:34
@awaelchli awaelchli force-pushed the feature/auto-move-data branch from 1b2a56d to cc4cb6b Compare June 8, 2020 17:37
@codecov
Copy link

codecov bot commented Jun 8, 2020 •
edited
Loading

Codecov Report

Merging #1905 into master will decrease coverage by 1%.
The diff coverage is 92%.

@@           Coverage Diff           @@
##           master   #1905    +/-   ##
=======================================
- Coverage      87%     86%    -1%     
=======================================
  Files          68      75     +7     
  Lines        5221    4798   -423     
=======================================
- Hits         4544    4140   -404     
+ Misses        677     658    -19

@awaelchli
Copy link
Contributor Author

@Borda If the decorator is enough, I think it is done module review. However this is probably not going to be merged very soon since there are these huge PRs on the top of the list.

@Borda Borda added this to the 0.9.0 milestone Jun 9, 2020
@mergify mergify bot requested a review from a team June 9, 2020 22:06
@Borda Borda requested review from ethanwharris, jeremyjordan and justusschock and removed request for a team June 9, 2020 22:09
@mergify mergify bot requested a review from a team June 9, 2020 22:10
SkafteNicki
SkafteNicki approved these changes Jun 15, 2020
View reviewed changes
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM, maybe we also could add a reference to the decorator in docs here:
https://pytorch-lightning.readthedocs.io/en/latest/trainer.html#deployment-prediction

@mergify mergify bot requested a review from a team June 15, 2020 12:51
@awaelchli
Copy link
Contributor Author

@SkafteNicki thanks, good suggestion. I added a short description to the deployment section.

@williamFalcon williamFalcon merged commit 22d9464 into master Jun 15, 2020
@Borda Borda deleted the feature/auto-move-data branch June 15, 2020 21:11
@awaelchli
Copy link
Contributor Author

Hey @HenryJia we made it!! good work ✋

@HenryJia
Copy link
Contributor

Awesome cheers <3 thanks for continuing it when I was too busy

@Borda Borda modified the milestones: 0.9.0, 0.8.0 Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

@HenryJia HenryJia HenryJia approved these changes

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@ethanwharris ethanwharris Awaiting requested review from ethanwharris ethanwharris is a code owner

@jeremyjordan jeremyjordan Awaiting requested review from jeremyjordan

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

Assignees
No one assigned
Labels
feature Is an improvement or enhancement
Projects
None yet
Milestone
pl:0.8
Development

Successfully merging this pull request may close these issues.

Auto move input to proper device for inference
5 participants
@awaelchli @Borda @HenryJia @SkafteNicki @williamFalcon