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.

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

Support IterableDatasets for validation and test, not just train set [blocked by #953] #948

Closed
Darktex opened this issue Feb 26, 2020 · 12 comments · Fixed by #1104
Closed
Assignees
Labels
feature Is an improvement or enhancement help wanted Open to be worked on

Comments

@Darktex
Copy link

Darktex commented Feb 26, 2020

🚀 Feature

Currently Lightning supports IterableDatasets only in the training set (see code). This makes them second-class citizens compared to the map-style datasets, and supporting them seems a low hanging fruit.

Motivation

This enables having larger test sets that may not fit into a machine's memory (they could be very large in production settings, or of modest size running in a student's cheap laptop). Moreover,
datasets are usually generated together (eg train, val, test can come from the same process). It is very likely that the same process has the same signature, so you may end up having IterableDatasets even when their size may not deem it strictly necessary.

Pitch

Changing a few lines of code by bringing in the checks we are doing for training should be enough unless I'm missing something.

Additional context

Are there any gotchas that make this harder than it looks?

@Darktex Darktex added feature Is an improvement or enhancement help wanted Open to be worked on labels Feb 26, 2020
@github-actions
Copy link
Contributor

Hey, thanks for your contribution! Great first issue!

@williamFalcon
Copy link
Contributor

@Darktex this looks straightforward! I can’t think if any gotchas right now. The only thing would be if you don’t have the length of a dataset up front but i think we’re refactoring to clear that up right now.

want to do a PR?

@ethanwharris @jeffling thoughts?

fyi @srush @luiscape

@ethanwharris
Copy link
Member

It seems there's an opportunity to clean stuff up a bit here. Really the only check we need is to see if len(dataloader) raises an error. If it does, then check if number of steps to run is set elsewhere and throw a warning if not (i.e. if not set elsewhere this will just run forever). That way you could get rid of the check for whether IterableDataset exists and the dependence on DataLoader.dataset, solving several issues.

@williamFalcon
Copy link
Contributor

maybe step 1 is to refactor the code to minimize the len(dataloader) calls? we likely only need them to:

  • figure out when to do validation checks (percent into epoch)
  • set the tqdm bar length

@ethanwharris
Copy link
Member

Agreed. Then it would be easier to see where the IterableDataset stuff will fall over, and just do something different when len is not available.

@williamFalcon
Copy link
Contributor

Ok, #953 is blocking this issue at the moment.

@williamFalcon williamFalcon changed the title Support IterableDatasets for validation and test, not just train set Support IterableDatasets for validation and test, not just train set [blocked by #953] Feb 26, 2020
@williamFalcon
Copy link
Contributor

@ethanwharris @Darktex i think 0.7.1 fixed this problem. Mind checking now?

@ethanwharris
Copy link
Member

@williamFalcon Not quite, still tires to call len on val / test dataloders - will PR in a bit

@ethanwharris ethanwharris self-assigned this Mar 9, 2020
@williamFalcon
Copy link
Contributor

is the easier thing to try catch for the len exception and set to inf if caught?

then when the epoch ends, set the length when we know it?

1 similar comment
@williamFalcon
Copy link
Contributor

is the easier thing to try catch for the len exception and set to inf if caught?

then when the epoch ends, set the length when we know it?

@ethanwharris
Copy link
Member

Yeah, that's the plan - currently have the is_infinite_dataloader method which tries to call len and catches the exception, just need to get the TQDM stuff to not do total=float('inf') as that raises an error

@ethanwharris
Copy link
Member

Not sure about setting the lenght once we know it - maybe in a seperate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement help wanted Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants