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

All data modules don't support providing batch size in the constructor. #417

Closed
senarvi opened this issue Dec 1, 2020 · 3 comments
Closed
Labels
bug Something isn't working datamodule Anything related to datamodules help wanted Extra attention is needed
Milestone

Comments

@senarvi
Copy link
Contributor

senarvi commented Dec 1, 2020

🐛 Bug

Half of the data loaders don't support providing batch size in the constructor, but only in the {train,val,test}_dataloader() methods. This means that if a data module is passed Trainer.fit(), the default batch size is used.

Cifar10DataModule, CityscapesDataModule, ImagenetDataModule, KittiDataModule, and STL10DataModule take batch size in constructor, which is used by the {train,val,test}_dataloader() methods.

MNISTDataModule takes batch size in constructor, but it's not used. {train,val,test}_dataloader() methods always use the default value 32.

BinaryMNISTDataModule, FashionMNISTDataModule, SklearnDataModule, SSLImagenetDataModule, and VOCDetectionDataModule don't take batch size in constructor. These have {train,val,test}_dataloader() methods that take a batch size argument, but a trainer always calls them without arguments.

To Reproduce

To illustrate the problem, the --batch_size argument has no effect in the the FasterRCNN model.

Steps to reproduce the behavior:

  1. Add print(batch_size) to VOCDetectionDataModule.train_dataloader().
  2. Run python faster_rcnn.py --batch_size 10.
  3. See that 1 is printed instead of 10.

Expected behavior

The batch size argument should be passed in the dataloader calls. This could be fixed by calling

trainer.fit(model, datamodule.train_dataloader(batch_size), datamodule.val_dataloader(batch_size))

instead of

trainer.fit(model, datamodule)

I guess it would be better to fix the data modules so that they take batch size in the constructor, though.

Environment

  • PyTorch Version (e.g., 1.0): 1.7.0
  • OS (e.g., Linux): Linux
  • How you installed PyTorch (conda, pip, source): conda
  • Python version: 3.8.5

Additional context

The DataModule documentation could be clearer regarding to how to set the batch size. The first MNISTDataModule example takes the batch size in constructor and saves it in the batch_size attribute, which is used by {train,val,test}_dataloader(). In the second example, the {train,val,test}_dataloader() methods take the batch size as an argument. However, if Trainer.fit() is called with a data module, the only way to set the batch size is in the data module constructor.

@senarvi senarvi added fix fixing issues... help wanted Extra attention is needed labels Dec 1, 2020
@github-actions
Copy link

github-actions bot commented Dec 1, 2020

Hi! thanks for your contribution!, great first issue!

@akihironitta akihironitta added the datamodule Anything related to datamodules label Dec 1, 2020
@akihironitta
Copy link
Contributor

@senarvi Thank you for reporting the issue! Actually, @hecoding has pointed it out in #334 and added batch_size argument to all datamodules in #344, and the fixes should be merged into the main branch very soon.

the --batch_size argument has no effect in the the FasterRCNN model.

You're right. We are trying to decouple datamodules from models in #207, and it seems we haven't removed those data-related arguments (e.g. --batch_size) from FasterRCNN as you pointed out.

In the second example, the {train,val,test}_dataloader() methods take the batch size as an argument.

Which example are you referring to?

@senarvi
Copy link
Contributor Author

senarvi commented Dec 1, 2020

@senarvi Thank you for reporting the issue! Actually, @hecoding has pointed it out in #334 and added batch_size argument to all datamodules in #344, and the fixes should be merged into the main branch very soon.

Great, then I'll close this issue!

In the second example, the {train,val,test}_dataloader() methods take the batch size as an argument.

Which example are you referring to?

Never mind, actually the second example ("Here’s a more realistic, complex DataModule") always uses batch size 32.

@senarvi senarvi closed this as completed Dec 1, 2020
@Borda Borda added this to the v0.3 milestone Jan 18, 2021
@Borda Borda added bug Something isn't working and removed fix fixing issues... labels Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datamodule Anything related to datamodules help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants