-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
update logic to inject FastForwardSampler / CaptureIterableDataset 2/n #8366
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8366 +/- ##
=======================================
+ Coverage 88% 93% +4%
=======================================
Files 168 167 -1
Lines 13985 13971 -14
=======================================
+ Hits 12355 12940 +585
+ Misses 1630 1031 -599 |
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
…ightning/pytorch-lightning into data_loading_extensions
# use current sampler | ||
sampler = dataloader.sampler | ||
|
||
dataloader = self.replace_sampler(dataloader, sampler, mode=mode) |
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.
Hi @tchaton, should we have replaced the sampler even when self.accelerator_connector.replace_sampler_ddp
is False
? I'm not familiar with the logic here, but a few of our tests failed because replace_sampler
is now invoked after this change. Specifically, the test uses a custom sampler, but _resolve_batch_sampler
expects to be able to re-construct the sampler following way:
if (batch_sampler is not None and type(batch_sampler) is not BatchSampler) or is_predicting:
batch_sampler = type(batch_sampler)(
sampler,
batch_size=batch_sampler.batch_size,
drop_last=(False if is_predicting else batch_sampler.drop_last),
)
However, the custom sampler in question doesn't have the expected fields.
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.
It won't actually replace it if replace_sampler_ddp
. But the code should be refactored as it does more than just replace the sampler.
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.
Is it fine if we only invoke replace_sampler
in the if-clause like before? I've confirmed that it would "fix" our test.
What does this PR do?
Should be merged after #8364
Does your PR introduce any breaking changes ? If yes, please list them.
Before submitting
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:
Did you have fun?
Make sure you had fun coding 🙃