-
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
remove redundant iterator call to data fetcher in loops #9117
Changes from all commits
5000f2e
4bfc7bd
37bbcff
f5f1b40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ | |
from pytorch_lightning.trainer.connectors.logger_connector.result import ResultCollection | ||
from pytorch_lightning.utilities.apply_func import apply_to_collection | ||
from pytorch_lightning.utilities.exceptions import MisconfigurationException | ||
from pytorch_lightning.utilities.fetching import DataLoaderIterDataFetcher | ||
from pytorch_lightning.utilities.fetching import AbstractDataFetcher, DataLoaderIterDataFetcher | ||
from pytorch_lightning.utilities.finite_checks import detect_nan_parameters | ||
from pytorch_lightning.utilities.types import STEP_OUTPUT | ||
|
||
|
@@ -105,9 +105,11 @@ def _process_training_step_output( | |
return results, hiddens | ||
|
||
|
||
def _prepare_dataloader_iter(dataloader_iter: Iterator, batch_idx: int) -> Iterator: | ||
def _prepare_dataloader_iter(data_fetcher: AbstractDataFetcher, batch_idx: int) -> Iterator: | ||
"""Attach the dataloader""" | ||
if not isinstance(dataloader_iter, DataLoaderIterDataFetcher): | ||
dataloader_iter = enumerate(dataloader_iter, batch_idx) | ||
# restore iteration | ||
if not isinstance(data_fetcher, DataLoaderIterDataFetcher): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not simply move this into the contract of this iterator? basically make the contract such that these data fetchers will always return the batch_idx as part of it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I suppose could be done like that :) |
||
# restore iteration | ||
dataloader_iter = enumerate(data_fetcher, batch_idx) | ||
else: | ||
dataloader_iter = iter(data_fetcher) | ||
return dataloader_iter |
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.
How much do we actually need to expose the "AbstractDataFetcher" here versus just an iterator? It seems to me that we're really not adding much value by exposing a new interface here.
The custom logic below really should be part of the iterator implementation itself as i comment below.
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.
yes, the iterator type annotation would have sufficed here