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.

Sign up for GitHub

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

Check early stopping metric in the beginning of the training #542

Merged
merged 11 commits into from
Jan 23, 2020
Merged

Check early stopping metric in the beginning of the training #542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4c635ad
Early stopping fix
kuynzereb Nov 25, 2019
c06bad6
Update trainer.py
williamFalcon Nov 30, 2019
acda2b7
Don't force validation sanity check
kuynzereb Dec 1, 2019
10e76f6
fix tests
kuynzereb Dec 2, 2019
aeb8bbd
Resolved conflicts
kuynzereb Jan 14, 2020
25e1bda
update
kuynzereb Jan 14, 2020
748640b
Added early_stopping check_metrics
kuynzereb Jan 14, 2020
706d84f
Merge branch 'master' into early_stopping_callback_fix
kuynzereb Jan 22, 2020
0f83b1a
Updated docs
kuynzereb Jan 22, 2020
4e35bee
Update docs
kuynzereb Jan 22, 2020
320e4c1
Do not call early stopping when validation is disabled
kuynzereb Jan 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pytorch_lightning/callbacks/pt_callbacks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,10 @@ def on_epoch_end(self, epoch, logs=None):
if current is None:
warnings.warn(
f'Early stopping conditioned on metric `{self.monitor}`'
f' which is not available. Available metrics are: {",".join(list(logs.keys()))}',
f' which is not available, so early stopping will not work.'
f' Available metrics are: {",".join(list(logs.keys()))}',
RuntimeWarning)
stop_training = True

Copy link
Member

Choose a reason for hiding this comment

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

then you should return True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly. Return True was before and it caused the interruption of the training if the required metric was not found. And now it just gives a warning and training just proceeds as though without early stopping. The point is that the callback should not stop the training if it can't find the metrics.

Actually, in the current implementation this branch is not reachable because we check for the availability of the metric in the trainer initialization. But my idea was that if we decide to set early_stopping to True by default, then it can be used to give a warning but not to stop the training.

You can also look at #524 for better understanding.

return stop_training

if self.monitor_op(current - self.min_delta, self.best):
Expand Down
16 changes: 14 additions & 2 deletions pytorch_lightning/trainer/trainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class Trainer(TrainerIOMixin,
def __init__(self,
logger=True,
checkpoint_callback=True,
early_stop_callback=True,
early_stop_callback=False,
Copy link
Member

Choose a reason for hiding this comment

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

why changing the default config?

Copy link
Contributor Author

@kuynzereb kuynzereb Nov 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As I have written in the description we should discuss whether early stopping should be turned on or off by the default. I think that it is better to be turned off. Again, please look at #524

Copy link
Contributor

@williamFalcon williamFalcon Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Early stopping should default to True - this is the most common use case.
This is a best practice for research with patience of 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that it is conditioned on the val_loss and it will work only if the user guess right the name. For this reason it seems to me that the better option is when the user deliberately enables early stopping. It is also very easy (just set True).

But if you insist I can suggest the following: we enable early stopping by default, but if there is no val_loss we will just warn the user that early stopping will not work and training will proceed with disabled early stopping. I just don't like that there will be some warnings when you run the trainer with the default settings :)

default_save_path=None,
gradient_clip_val=0,
gradient_clip=None, # backward compatible
Expand Down Expand Up @@ -185,6 +185,8 @@ def __init__(self,
# creates a default one if none passed in
self.early_stop_callback = None
self.configure_early_stopping(early_stop_callback, logger)
if self.enable_early_stop:
self.nb_sanity_val_steps = max(1, self.nb_sanity_val_steps)
Copy link
Member

Choose a reason for hiding this comment

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

maybe max(1, nb_sanity_val_steps) since earlier you have

if self.fast_dev_run:
    self.nb_sanity_val_steps = 1

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 exactly by that reason it should be max(1, self.nb_sanity_val_steps) :)

We just take the previously defined final self.nb_sanity_val_steps and set it to 1 if it is less than 1.

If we made as you have suggested then self.nb_sanity_val_steps will be equal to the user defined value in fast dev run mode, but it should be 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not do this. People need to have the option of turning sanity_val_check off

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 how then we will check that early stopping will work correctly? (Note that we force this check only if early stopping is turned on.)

Copy link
Contributor

@williamFalcon williamFalcon Nov 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I understand what you're saying, but restricting EVERYONE to force sanity check will certainly block some esoteric research or production cases, so we can't do this.

But I think this is on the user at this point. If they turned off sanity check then it's on them at that point and are willingly exposing themselves to these kinds of issues... but for people who keep it on, then we use what you suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


# configure checkpoint callback
self.checkpoint_callback = checkpoint_callback
Expand Down Expand Up @@ -444,6 +446,7 @@ def run_pretrain_routine(self, model):
# run tiny validation (if validation defined)
# to make sure program won't crash during val
ref_model.on_sanity_check_start()
callback_metrics = {}
if self.get_val_dataloaders() is not None and self.nb_sanity_val_steps > 0:
# init progress bars for validation sanity check
pbar = tqdm.tqdm(desc='Validation sanity check', total=self.nb_sanity_val_steps,
Expand All @@ -453,12 +456,21 @@ def run_pretrain_routine(self, model):
# dummy validation progress bar
self.val_progress_bar = tqdm.tqdm(disable=True)

self.evaluate(model, self.get_val_dataloaders(), self.nb_sanity_val_steps, self.testing)
eval_results = self.evaluate(model, self.get_val_dataloaders(),
self.nb_sanity_val_steps, False)
_, _, _, callback_metrics, _ = self.process_output(eval_results)

# close progress bars
self.main_progress_bar.close()
self.val_progress_bar.close()

if (self.enable_early_stop and
callback_metrics.get(self.early_stop_callback.monitor) is None):
raise RuntimeError(f"Early stopping was configured to monitor "
f"{self.early_stop_callback.monitor} but it is not available "
f"after validation_end. Available metrics are: "
f"{','.join(list(callback_metrics.keys()))}")

# init progress bar
pbar = tqdm.tqdm(leave=True, position=2 * self.process_position,
disable=not self.show_progress_bar, dynamic_ncols=True, unit='batch',
Expand Down