dmlc / dmlc/dgl

[bug] dgl.dataloading.DataLoader forwards args to torch.utils.data.DataLoader but many are not valid

Open
#4,507 4 comments 2 reactions 0 assignees View on GitHub
bug:confirmed
Dominant language
Python
Stars
14.3k
Forks
3.1k
PR merge metrics
No merged PRs in 30d

Description

## 🐛 Bug

Currently, the DGL's DataLoader is a child of PyTorch's DataLoader, and passes `kwargs` to it, however DGL's dataloader does several things under the hood which breaks many of these options, and no indication is given to the user what is valid and what is not. Further more, CI covers a fraction of possible options users my pass in.

Current problematic options:
* `sampler` and `batch_sampler`: Specifying either of these results in errors about `IterableDataset`. Our documentation explicitly references `sampler` inside the description for the `use_ddp` argument.
* `collate_fn`: This results in a duplicate keyword error.
* `pin_memory`: This results in a duplicate keyword error, as the super is called with `pin_memory=self.pin_prefetcher`.
* `pin_memory_device` (new in 1.12): This will only have effect on the input to the _PrefetchingIterator, which may not affect the data as returned to the user.
* `generator`: Has no effect.
* `timeout`: While this will work relatively correctly with respect to the workers, when a prefetching thread is active, it is possible to block on the dataloader for longer than the specified timeout, which would be unexpected from the user's perspective.
* ~~`worker_init_fn`: This results in a duplicate keyword error, however this appears just to be a bug as we attempt to correctly wrap the passed in function, but fail to delete it from the `kwargs`.~~
* `batch_size=None`: While it would be odd for a user to do this given we don't provide users a way to manually batch data, our documentation does address it when it is valid for PyTorch's DataLoader.

We also have the issue of when PyTorch's DataLoader is updated, how to maintain compatibility. For example, the option `pin_memory_device` was just added in 1.12.

## Proposed fix

I think the best way to fix this, is to remove the `kwargs` parameter to our DataLoader, and instead capture every argument explicitly. This way we can limit, verify, and test the different argument combinations passed in. It will also make reading our documentation easier.

## Alternative fix

We could try to keep documentation on what parameters are valid, but we may need separate documentation per PyTorch version.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at dgl.dataloading.DataLoader and compare its accepted arguments with torch.utils.data.DataLoader. Trace how kwargs are forwarded, including sampler, batch_sampler, collate_fn, pin_memory, generator, timeout, and batch_size. Done means the supported argument set is explicit, incompatible options are handled consistently, and the combinations are covered by tests and documentation.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data, machine-learning
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.