-
Notifications
You must be signed in to change notification settings - Fork 110
Add log_train_loss_on_step toggle to EasySyntax #886
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
470d077
d4e3508
1152aa7
4dd59b7
07b2c72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,8 +36,23 @@ def __init__( | |||||||||
| scheduler_class: Optional[type] = None, | ||||||||||
| scheduler_kwargs: Optional[Dict] = None, | ||||||||||
| scheduler_config: Optional[Dict] = None, | ||||||||||
| also_log_train_loss_per_step: bool = False, | ||||||||||
| ) -> None: | ||||||||||
| """Construct `StandardModel`.""" | ||||||||||
| """Construct `StandardModel`. | ||||||||||
|
|
||||||||||
| Args: | ||||||||||
| tasks: Task(s) appended as the head(s) of the model, defining | ||||||||||
| the prediction target(s) and loss(es). | ||||||||||
| optimizer_class: Optimizer class used during training. | ||||||||||
| optimizer_kwargs: Keyword arguments passed to `optimizer_class`. | ||||||||||
| scheduler_class: Learning-rate scheduler class. If `None`, no | ||||||||||
| scheduler is used. | ||||||||||
| scheduler_kwargs: Keyword arguments passed to `scheduler_class`. | ||||||||||
| scheduler_config: Additional configuration for how the scheduler | ||||||||||
| is invoked by PyTorch Lightning (e.g. `interval`, `frequency`). | ||||||||||
| also_log_train_loss_per_step: If `True`, additionally logs the | ||||||||||
| per-batch training loss under `train_loss_step`. | ||||||||||
| """ | ||||||||||
| # Base class constructor | ||||||||||
| super().__init__(name=__name__, class_name=self.__class__.__name__) | ||||||||||
|
|
||||||||||
|
|
@@ -52,6 +67,7 @@ def __init__( | |||||||||
| self._scheduler_class = scheduler_class | ||||||||||
| self._scheduler_kwargs = scheduler_kwargs or dict() | ||||||||||
| self._scheduler_config = scheduler_config or dict() | ||||||||||
| self._also_log_train_loss_per_step = also_log_train_loss_per_step | ||||||||||
|
|
||||||||||
| self.validate_tasks() | ||||||||||
|
|
||||||||||
|
|
@@ -243,15 +259,26 @@ def training_step( | |||||||||
| if isinstance(train_batch, Data): | ||||||||||
| train_batch = [train_batch] | ||||||||||
| loss = self.shared_step(train_batch, batch_idx) | ||||||||||
| batch_size = self._get_batch_size(train_batch) | ||||||||||
| self.log( | ||||||||||
| "train_loss", | ||||||||||
| loss, | ||||||||||
| batch_size=self._get_batch_size(train_batch), | ||||||||||
| batch_size=batch_size, | ||||||||||
| prog_bar=True, | ||||||||||
| on_epoch=True, | ||||||||||
| on_step=False, | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And then expose the logging setting to the class. This removes duplicate code and then we don't have to define the batch_size.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, but how do we want to handle the logging of the val loss?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is fine to have the logging of the validation and train loss behave in the same way. In principle we could separate the arguments for validation and training, but I think that is a little too many arguments, and moving towards instances where people should just create their own torch-lightning callbacks. |
||||||||||
| sync_dist=True, | ||||||||||
| ) | ||||||||||
| if self._also_log_train_loss_per_step: | ||||||||||
| self.log( | ||||||||||
| "train_loss_step", | ||||||||||
| loss, | ||||||||||
| batch_size=batch_size, | ||||||||||
| prog_bar=False, | ||||||||||
| on_epoch=False, | ||||||||||
| on_step=True, | ||||||||||
| sync_dist=False, | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| current_lr = self.trainer.optimizers[0].param_groups[0]["lr"] | ||||||||||
| self.log("lr", current_lr, prog_bar=True, on_step=True) | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry for being a little bit annoying here but I think it is better to just expose the
on_epochandon_stepfrom the lightning library then the user can decide themselves whether they want just on step, or just on epoch, on both or on neither. The standard values should follow current behavior.