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

Add dataloader_idx to batch transfer hooks #6241

Merged
merged 38 commits into from
May 13, 2021
Merged

Add dataloader_idx to batch transfer hooks #6241

merged 38 commits into from
May 13, 2021

Conversation

rohitgr7
Copy link
Contributor

@rohitgr7 rohitgr7 commented Feb 27, 2021 •
edited
Loading

What does this PR do?

Fixes #6058
Also updated the internal flow from args to kwargs (OrderedDict) to simplify things a bit and better accessibility and kwargs values are passed as args to the user-specific hooks for BC.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

rohitgr7
rohitgr7 commented Feb 27, 2021
View reviewed changes
pytorch_lightning/core/lightning.py Outdated
def _apply_batch_transfer_handler(self, batch: Any, device: Optional[torch.device] = None, dataloader_idx: int = 0):
def _apply_batch_transfer_handler(
self, batch: Any, device: Optional[torch.device] = None, dataloader_idx: Optional[int] = None
):
batch = self.on_before_batch_transfer(batch, dataloader_idx)
batch = self.transfer_batch_to_device(batch, device)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should remove the device argument from transfer_batch_to_device since this can easily be accessed using self.device (device property of LightningModule).

@codecov
Copy link

codecov bot commented Feb 27, 2021 •
edited
Loading

Codecov Report

Merging #6241 (65152d3) into master (b9a52fa) will decrease coverage by 0%.
The diff coverage is 97%.

@@          Coverage Diff           @@
##           master   #6241   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files         199     199           
  Lines       13066   13073    +7     
======================================
- Hits        12021   11990   -31     
- Misses       1045    1083   +38

@rohitgr7 rohitgr7 force-pushed the enhance/batch_transfer_dl_idx branch from 9470d61 to dc46a6c Compare March 6, 2021 20:35
@rohitgr7
Copy link
Contributor Author

rohitgr7 commented Mar 6, 2021

Hi @SeanNaren some deepspeed/sharded tests are failing here. Mind check the reason for it. Can't figure out what's causing these failures. Thanks :)

@rohitgr7 rohitgr7 added design Includes a design discussion data handling Generic data-related topic feature Is an improvement or enhancement labels Mar 6, 2021
@rohitgr7 rohitgr7 added this to the 1.2.x milestone Mar 6, 2021
@rohitgr7 rohitgr7 force-pushed the enhance/batch_transfer_dl_idx branch from 9d0994c to 04e4ee3 Compare March 13, 2021 13:50
@pep8speaks
Copy link

pep8speaks commented Mar 13, 2021 •
edited
Loading

Hello @rohitgr7! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-11 18:26:45 UTC

@rohitgr7 rohitgr7 marked this pull request as ready for review March 13, 2021 15:29
@rohitgr7 rohitgr7 changed the title Add dataloader_idx to batch transfer hooks [WIP] Add dataloader_idx to batch transfer hooks Mar 16, 2021
@mergify mergify bot removed the has conflicts label May 6, 2021
@Borda Borda added the _Will label May 6, 2021
@mergify mergify bot added the has conflicts label May 6, 2021
@mergify mergify bot removed the has conflicts label May 8, 2021
@mergify mergify bot added the has conflicts label May 8, 2021
@mergify mergify bot removed the has conflicts label May 9, 2021
Borda
Borda reviewed May 11, 2021
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

is the args >> kwargs really needed to be done here as the R title does not mention it, I feel it would be much easier to review split it to two PRs

Borda
Borda approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

overall the kwargs seems much safer than just args :]

@carmocca carmocca removed design Includes a design discussion _Will labels May 12, 2021
@carmocca
Copy link
Contributor

carmocca commented May 12, 2021 •
edited
Loading

Unlabeled API/design as this does not modify the API, dataloader_idx is there already from #6059

@carmocca
Copy link
Contributor

cc @awaelchli as accelerator CODEOWNER

@awaelchli awaelchli disabled auto-merge May 12, 2021 12:29
@rohitgr7
Copy link
Contributor Author

Unlabeled API/design as this does not modify the API, dataloader_idx is there already from #6059

transfer_batch_to_device API is updated.

@carmocca
Copy link
Contributor

transfer_batch_to_device API is updated.

In any case, this was previously approved by William so no need for his explicit approval again.

Feel free to merge

@carmocca carmocca changed the title Add dataloader_idx to batch transfer hooks Add dataloader_idx to batch transfer hooks May 12, 2021
@rohitgr7 rohitgr7 merged commit 7ca4173 into master May 13, 2021
@rohitgr7 rohitgr7 deleted the enhance/batch_transfer_dl_idx branch May 13, 2021 17:33
@awaelchli awaelchli mentioned this pull request May 24, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carmocca carmocca carmocca approved these changes

@ananthsub ananthsub ananthsub approved these changes

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda approved these changes

@edenlightning edenlightning Awaiting requested review from edenlightning

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

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

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

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

Assignees
No one assigned
Labels
data handling Generic data-related topic feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.4
Development

Successfully merging this pull request may close these issues.

on_*_batch_transfer hooks should include a dataloader_index parameter
7 participants
@rohitgr7 @pep8speaks @tchaton @carmocca @Borda @ananthsub @awaelchli