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

Make default tqdm dict overridable #749

Merged

Conversation

kuynzereb
Copy link
Contributor

Resolves #629.

This PR moves default tqdm_dict definition from Trainer to LightningModule, so it can be overridden by the user. Now you just need to overwrite get_tqdm_dict() on the module to define your own default tqdm items.

Also I have removed batch_idx and gpu from the default tqdm dict and changed all batch/s to it/s as was suggested by @CarloLucibello.

@tullie, @williamFalcon, @Borda

@williamFalcon
Copy link
Contributor

williamFalcon commented Jan 26, 2020

@kuynzereb fails on GPU tests...


self = LightningDataParallel(
  (module): LightningTestModel(
    (c_d1): Linear(in_features=784, out_features=1000, bias=Tru...)
    (c_d1_drop): Dropout(p=0.2, inplace=False)
    (c_d2): Linear(in_features=1000, out_features=10, bias=True)
  )
)
name = 'get_tqdm_dict'

    def __getattr__(self, name):
        if '_parameters' in self.__dict__:
            _parameters = self.__dict__['_parameters']
            if name in _parameters:
                return _parameters[name]
        if '_buffers' in self.__dict__:
            _buffers = self.__dict__['_buffers']
            if name in _buffers:
                return _buffers[name]
        if '_modules' in self.__dict__:
            modules = self.__dict__['_modules']
            if name in modules:
                return modules[name]
        raise AttributeError("'{}' object has no attribute '{}'".format(
>           type(self).__name__, name))
E       AttributeError: 'LightningDataParallel' object has no attribute 'get_tqdm_dict'



-- Docs: https://docs.pytest.org/en/latest/warnings.html
========================================================================= 11 failed, 102 passed, 23 warnings in 161.23s (0:02:41) ===

@kuynzereb
Copy link
Contributor Author

I think I have fixed it. But actually I don't understand how did you get these fails, because travis shows no errors and I was not able to reproduce it locally...

@williamFalcon
Copy link
Contributor

these are GPU tests. Travis doesn't run GPU tests

@kuynzereb
Copy link
Contributor Author

Ah, okay, got it. Moreover several tests require more than 1 gpu, so it turned out that I can't run the full test-suite because now I have only 1 gpu on my machine :(

@Borda
Copy link
Member

Borda commented Jan 30, 2020

tackling #486

tqdm_dict['gpu'] = '{}'.format(torch.cuda.current_device())

return tqdm_dict
return dict(**ref_model.get_tqdm_dict(), **self.tqdm_metrics)
Copy link

@CarloLucibello CarloLucibello Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with

if self.tqdm_metrics == None:
    return ref_model.get_tqdm_dict()
else:
    return self.tqdm_metrics

so that that the default dict can be completely overloaded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is already can be completely overloaded :)
self.tqdm_metrics is not the default, it is totally determined by progress_bar dict from the training_end() and validation_end(). And model.get_tqdm_dict() adds some additional information like version number, gpu, etc. With this PR it is also totally controlled by the user.

So if you return an empty dict in model.get_tqdm_dict() and have no progress_bar dicts you will have a tqdm progress bar without any additional info.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, sorry for the noise, I thought that get_tqdm_dict was not exposed. Is it discoverable? Should something be added somewhere in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, now it can be found as one of the methods of LightningModule. Maybe we can add some information about how progress bar is composed, but I don't understand where it can be inserted in the current docs.

@williamFalcon
Copy link
Contributor

@kuynzereb @CarloLucibello what's the verdict? @Borda

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -1211,6 +1211,25 @@ def on_save_checkpoint(self, checkpoint):
"""
pass

def get_tqdm_dict(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it as property like a tqdm_params?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, at first I made it property. But then I accidentally found out that it will cause misleading error messages, if there is an attribute error in the user code. What I mean, if you make

@property
def tqdm_dict(self):
    _ = self.trainer.some_non_existing_attribute
    return {}

Then after

return dict(**ref_model.tqdm_dict, **self.tqdm_metrics)

you will get

AttributeError: 'CoolSystem' object has no attribute 'tqdm_dict'

I don't know how to fix it, but it seems to be the known issue: https://stackoverflow.com/questions/15348331/why-does-property-decorator-show-object-has-no-attribute

Overall, I decided to stay without property, so the user gets the clear error message if there is some error in his code.

@CarloLucibello
Copy link

looks fine

@williamFalcon williamFalcon merged commit 5035ce5 into Lightning-AI:master Feb 5, 2020
@kuynzereb kuynzereb deleted the make_tqdm_dict_overridable branch February 5, 2020 11:49
@Borda Borda mentioned this pull request Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slim down progress bar default printing
4 participants