-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add Support for multiple train loaders #1959
Conversation
Hello @justusschock! Thanks for updating this PR.
Comment last updated at 2021-01-04 19:58:04 UTC |
in the eval loop we pass in a dataloader_idx, what are your thoughts on that? does it make sense or not to have that in training_step as well? |
I don't think so, because in the validation phase we will use the dataloaders sequentially and in the training we will use them in parallel |
yup, I like your idea. just wanted to raise this because in slack there was talk about making it consistent with eval loops. |
How do we handle different length datasets? Option 1: (L is a long dataset, 0,1,2,3,4 are cycles of the smaller datasets) Option 2: I personally think it should be option 1. @justusschock one very simple way to get this feature right now is just to add all trainining datasets to concat dataset for the user.
|
@williamFalcon I think it should be the minimum length, since this would be the most explicit version. Another option would be to sample from each loader as long as they did not yet run out of samples and to omit the loaders which are already exhausted. |
but seems weird to me that i wouldn’t use the full dataset if i had a smaller one. |
@justusschock any progress here? |
I'll get this done, once the metrics are finally finished :D Sorry for the delay on my side |
I could get around this problem using a custom pytorch BatchSampler which I pass to the dataloader. My dataset's get_item takes a tuple of integers as index, each integer gets a data item for corresponding dataset. The only drawback is that the data item needs to be same shape for all 3 datasets but the batch size can be different for each dataset's items. |
Okay, this is almost finished. Currently there still is a bug, if a loader has no length. Any ideas how we should proceed with this? Shall we set the overall length simply to inf? |
I guess that we made similar "hotfix" to valid dataloader and salt length to inf |
Hi, @omiita spotted this error...
|
Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Nicki Skafte <[email protected]>
length = all_lengths | ||
|
||
elif isinstance(all_lengths, Mapping): | ||
length = compute_func(all_lengths.values()) |
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.
What happens if something defines something like
{"a":{"b":...}}
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.
good point, this would currently fail. Is this something we want to enable?
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.
Not at this point I think. People will open an issue if needed.
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.
Awesome work !
@justusschock it seems that this break checks, mind check it ASAP 🐰 |
It doesn't look like the documentation for |
Before submitting
What does this PR do?
When this is finished it adds support for drawing batches from multiple train loaders at once. If the loaders are specified as a Mapping (dict), the resulting batch will consist of one batch per loader under the same keys as the loaders like this:
will result in a batch like this:
and loaders in a sequence will return in a sequence-batch built of the separate batches in the correct order:
will result in a batch like this:
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 🙃