Skip to content

Adds functionality to save best model in CheckpointingCallback and fixes a typo #3

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

Open
dmsm wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Adds functionality to save best model in CheckpointingCallback and fixes a typo #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cad15fd
Fix typo
Jun 4, 2020
f6343dd
Add functionality to save best model in CheckpointingCallback
Jun 4, 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
33 changes: 29 additions & 4 deletions ttools/callbacks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def __init__(self, keys=None, val_keys=None, smoothing=0.999):
self.keys = keys

if val_keys is None:
self.val_keys = []
self.val_keys = []
else:
self.val_keys = val_keys

Expand All @@ -184,7 +184,7 @@ class VisdomLoggingCallback(KeyedCallback):
0.0 disables smoothing.

@mgharbi mgharbi Jun 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

maybe add a couple doc entries

"""

def __init__(self, keys=None, val_keys=None, frequency=100, server=None,
def __init__(self, keys=None, val_keys=None, frequency=100, server=None,
port=8097, base_url="/", env="main", log=False, smoothing=0.99):
super(VisdomLoggingCallback, self).__init__(
keys=keys, val_keys=val_keys, smoothing=smoothing)
Expand Down Expand Up @@ -460,14 +460,23 @@ class CheckpointingCallback(Callback):

PERIODIC_PREFIX = "periodic_"
EPOCH_PREFIX = "epoch_"
BEST_MODEL_FILENAME = "best"

def __init__(self, checkpointer, interval=600,
max_files=5, max_epochs=10):
max_files=5, max_epochs=10,
best_val_key=None, best_val_value=None):

@mgharbi mgharbi Jun 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

do we need to initialize best_val_value externally? could that be handled by the load/save mechanism so we only have one parameter?

super(CheckpointingCallback, self).__init__()
self.checkpointer = checkpointer
self.interval = interval
self.max_files = max_files
self.max_epochs = max_epochs
self.best_val_key = best_val_key

if best_val_value is not None:
LOG.info("Loaded best model ({}={})".format(best_val_key, best_val_value))

@mgharbi mgharbi Jun 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

"loaded" be misleading, since the load does not happen at init time

self.best_val_value = best_val_key
else:
self.best_val_value = float('inf')

self.last_checkpoint_time = time.time()

Expand Down Expand Up @@ -506,6 +515,22 @@ def batch_end(self, batch_data, train_step_data):
self.checkpointer.save(filename, extras={"epoch": self.epoch})

@mgharbi mgharbi Jun 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

here and elsewhere were save is called, we might want to always save the best_val_value for easy loading and treat the "None" case. It could be nice, but not required, to add a "load_best" method to the Checkpointer class.

self.__purge_old_files()

def validation_end(self, val_data):
"""Save a best model checkpoint if value for best_val_key is lowest so far."""

super(CheckpointingCallback, self).validation_end(val_data)

if self.best_val_key is None:
return
if val_data[self.best_val_key] > self.best_val_value:

@mgharbi mgharbi Jun 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

we should probably document that the "best" is implicitly a min value (e.g. if someone uses accuracy instead of loss)

return

self.best_val_value = val_data[self.best_val_key]

LOG.debug("Best model checkpoint ({}={})".format(self.best_val_key, self.best_val_value))
self.checkpointer.save(CheckpointingCallback.BEST_MODEL_FILENAME,
extras={"epoch": self.epoch, "best_val_value": self.best_val_value})

def __purge_old_files(self):
"""Delete checkpoints that are beyond the max to keep."""

Expand Down Expand Up @@ -635,7 +660,7 @@ def training_end(self):
print("end logging experiment", self.epoch, self.batch)

def _get_commit(self):
return subprocess.check_output(["git", "rev-parse", "HEAD"])
return subprocess.check_output(["git", "rev-parse", "HEAD"])


class CSVLoggingCallback(KeyedCallback):
Expand Down
4 changes: 2 additions & 2 deletions ttools/training.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def training_step(self, batch):

This should implement a forward pass of the model, compute gradients,
take an optimizer step and return useful metrics and tensors for
visualization and training callbacks.
visualization and training callbacks.

Args:
batch (dict): batch of data provided by a data pipeline.
Expand Down Expand Up @@ -168,7 +168,7 @@ def train(self, dataloader, starting_epoch=None, num_epochs=None,
if starting_epoch is None:
starting_epoch = 0

LOG.info("Starting taining from epoch %d", starting_epoch)
LOG.info("Starting training from epoch %d", starting_epoch)

epoch = starting_epoch

Expand Down