Fix bugs in scale_batch_size

[ghost]

Fixes #2484

• Set model.batch_size based on model.hparams.batch_size if not defined
• Changed init_val for batch_size to 0 so that it starts with user defined batch_size instead of 2 all the time

[pep8speaks]

Hello @x2-l! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-06 13:07:55 UTC

[codecov]

Codecov Report

Merging #2523 into master will increase coverage by 1%.
The diff coverage is 33%.

@@          Coverage Diff           @@
##           master   #2523   +/-   ##
======================================
+ Coverage      88%     89%   +1%     
======================================
  Files          69      69           
  Lines        5628    5641   +13     
======================================
+ Hits         4963    5017   +54     
+ Misses        665     624   -41     

[Borda]

LGTM
@SkafteNicki mind check?

[SkafteNicki]

I am not sure this will solve your problem. Since it is only model.batch_size that gets updated during the scaling, if you have defined your dataloaders using Dataloader(dataset, batch_size=self.hparams.batch_size) then this will have no effect.

[ghost]

this should be handled by user no? its the same as enabling auto_lr_find=True but the user sets Optimizer(self.parameters(), lr=self.hparams.learning_rate). or maybe scale_batch_size should consider overwritting DataLoader.batch_size if it is called?

[justusschock]

Maybe we also need to check, if self.hparams is a mapping, because then you can't use setattr

[SkafteNicki]

@x2-l DataLoader.batch_size cannot be overwritten after it is initialized, already tried it because that would be the easiest solution.
@justusschock agree, in PR #1998 I have implemented three functions as counterparts to setattr, hasattr and getattr that basically search for attribute in this order:

1. attribute of model i.e. model.batch_size
2. key of model.hparams i.e. `model.hparams['batch_size']
3. attribute of model.hparams i.e. model.hparams.batch_size

[ghost]

@SkafteNicki what bout reinitializing DataLoader with the new batch_size like

def replace_batch_size(self, dataloader, batch_size):
    skip_keys = ['batch_size']
    dl_args = {
        k: v for k, v in dataloader.__dict__.items() if not k.startswith('_') and k not in skip_keys
    }

    dl_args['batch_size'] = batch_size
    dataloader = type(dataloader)(**dl_args)
    return dataloader

[SkafteNicki]

@x2-l agree that should work

[awaelchli]

@x2-l I think there is already code like this for replacing the sampler/shuffle attributes. One could factor this out and create a general function
replace_dataloader_attr(dataloader: DataLoader, name: str, new_value: Any) like you proposed and avoid duplicated code.

[SkafteNicki]

Here is the function we already have: https://github.com/PyTorchLightning/pytorch-lightning/blob/927f305f7e556828b5cdd45e3977c67f3c54b8fc/pytorch_lightning/trainer/data_loading.py#L169-L178

[Borda]

I agree that this is quite a monkey patch and it would be better to support 'hparams.batch_size' as batch_arg_name

[ghost]

@SkafteNicki @Borda should I still update this? or can be updated together at PR #1998?

[ghost]

on second thought, why not just raise an warning that model.batch_size will be used when both model.batch_size and model.hparams.batch_size are set? if only model.hparams.batch_size is set, the attribute batch_size will be created for model.

the scale_batch_size function will then update both params so that it'll be consistent no matter what is being used in the rest of the code. same can be done for the learning_rate. when these functions are called the original values are just the initial values that will be replaced after the search. this can also avoid the undesired scenarios when self.hparams.batch_size and self.hparams.lr are used in defining the data loaders and optimizers and the values are not updated with the searched values.

@Borda @SkafteNicki @awaelchli thoughts?

[justusschock]

While I see your point, we should be careful about that. E.g. when you reinstantiate your model and overwrite the wrong parameter, you introduce unwanted behaviour somewhere else.

I think I'd prefer not to implicitly introduce concurring new arguments of not necessary

[Borda]

I would say, do this patch/fix here because it may take some time before PR #1998 lands...
also, this may be 0.8.x release compare to PR #1998 is aimed for 0.9

[ghost]

I enabled scaling of batch size using model.hparams.batch_arg_name and removed setting the .batch_arg_name attribute on the model. If both are specified, a warning is raised telling user that model.batch_arg_name will be used as initial value for the search. If it's undesired, user can stop and change accordingly. I think that'll be enough for the bug fix while not introducing anything new?

[Borda]

seems taking parts from #2223 so let's merge this after it...

[mdgoldberg]

Hello! Will this and/or #2223 also fix auto_lr_find? It has the same issue.

[mergify]

This pull request is now in conflict... :(

[Borda]

@ghost mind finish this one as #2223 have been merged...

[awaelchli]

Hey! recently a similar PR/issue was fixed by @SkafteNicki in #2821. Could we use the lightning_attr api he added there to implement these checks here? They look very similar.
And we need to keep in mind the compatibility with datamodules, but perhaps this should be done in a different PR :)

[SkafteNicki]

Agree with @awaelchli that the first issue addressed in this PR is the same issue the learning rate finder had. If you could exchange all hasattr, getattr, setattr with the corresponding function from this file https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/utilities/parsing.py#L145-L195 then it would be great :)

[awaelchli]

It looks like this user and their repository completely vanished from github. Should we copy these changes over into a new PR and continue as we discussed?

[Borda]

It looks like this user and their repository completely vanished from github. Should we copy these changes over into a new PR and continue as we discussed?

he behaves as his nick says :D
I would say, check if we can push/rebase this PR, if yes, let's continue if not let's make the same branch in our repo, and merge this PR into the new branch and continue there...