Ignite: Subclasses of torch.utils.data.DataLoader are not supported

[jwindhager]

🐛 Bug description

When using the Ignite engine with a subclass of torch.utils.data.DataLoader, the engine re-instantiates the dataloader with its torch.utils.data.DataLoader base class, if the DataLoader's batch_sampler is not a ReproducibleBatchSampler. See engine.py, line 874 in my version (call to _update_dataloader).

This is super-dangerous, as people might not even notice that their custom DataLoader isn't being used at all. Only found the issue because my custom DataLoader has some additional public members that aren't parameters of the torch.utils.data.DataLoader constructor, which is being called in _update_dataloader.

Environment

• PyTorch Version: 1.4.0
• Ignite Version: 0.3.0
• OS: Linux
• How you installed Ignite: conda
• Python version: 3.7.6

[vfdev-5]

@jwindhager thanks for the report. Yes, we, probably, should have fixed that issue in the master : https://github.com/pytorch/ignite/blob/master/ignite/engine/utils.py#L16

This is super-dangerous, as people might not even notice that their custom DataLoader isn't being used at all.

I understand your point. This dataloader update is a price to pay to have reproducible dataflow.
Today, we are going to rethink this code, see https://github.com/pytorch/ignite/issues/795#issuecomment-589607610, such that to add this as an option: engine controls the dataflow...

[jwindhager]

@vfdev-5 Thanks for your quick reply and apologies, should've checked master first! In any case, in my opinion, re-initializing the DataLoader without a warning is not a good idea, as the constructor call may have side effects and doesn't necessarily produce an "identical" DataLoader instance.

Either way, I hope that this issue will soon be fixed with a future release!

[vfdev-5]

Yes, you are right about the warning, thanks for that.

In any case, in my opinion, re-initializing the DataLoader without a warning is not a good idea, as the constructor call may have side effects and doesn't necessarily produce an "identical" DataLoader instance.

If you have an idea on how to improve that, we are happy to discuss and implement that :)

Either way, I hope that this issue will soon be fixed with a future release!

You can also try our nightly releases with the latest features.

[jwindhager]

If you have an idea on how to improve that, we are happy to discuss and implement that :)

Unfortunately not really, sorry. I think for the way it's currently implemented, a safer option would be to just raise an exception at runtime in case the user provides a custom DataLoader without a (subclass of) ReproducibleBatchSampler as batch_sampler. Another option would probably be to wrap the entire DataLoader instead of just the BatchSampler and sample the batch indices within the DataLoader constructor, but I haven't really thought that through tbh...

[justusschock]

To jump into this: wrapping the whole dataloader would be difficult I think, because then we would probably bypass the user's custom dataloader and directly use the dataset, which is also not wanted here.

IMO raising an exception is also not a good idea, since there are some user's (like me) who carefully designed their custom dataloader to work and behave as expected with ignite. Raising a warning and improving the docs would be the way to go IMO.

If the user wishes to use a custom dataloader, we should expect him to make it work as he wishes and therefore simply making him aware of that problem should be sufficient