-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Added SaveConfigCallback.save_config
#17475
Conversation
…saving the config to a logger.
SaveConfigCallback.extra_save_config
SaveConfigCallback.extra_save_config
SaveConfigCallback.save_config
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.
This looks better than before but still doesn't seem ideal to me. Left comments!
Co-authored-by: Carlos Mocholí <[email protected]>
class LoggerSaveConfigCallback(SaveConfigCallback): | ||
def save_config(self, trainer: Trainer, pl_module: LightningModule, stage: str) -> None: | ||
if isinstance(trainer.logger, Logger): | ||
config = self.parser.dump(self.config, skip_none=False) # Required for proper reproducibility | ||
trainer.logger.log_hyperparams({"config": config}) |
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.
@tommycwh I answer your comment since it is about this pull request.
However, I would want some more instructions on how to implement my save_config() function. For example, in my example above, there are some hyperparameters (like those numerical values) that can be saved by the "default" config saving mechanism and I would need to implement my own mechanism for the others that cannot be saved automaically (like the init_args).
I am not sure if I truly understand what you mean or what exactly is it that you want to achieve. This pull request adds save_config
such that it can be used however people want to use it. In principle the config is intended for reproducibility and I would save it in a logger as an artifact or a single yaml string which includes any init_args
. This is just three lines of code like shown in the example. Doing something different is perfectly fine, but it depends on what you want to do.
What does this PR do?
Makes it easy to subclass
SaveConfigCallback
to additionally save the config in other places, such as a logger. The main purpose of this feature is to provide a way to implement a custom save config without having to worry about ranks or race conditions.Motivated by #15340 (comment)
Might fix #14628, unless we consider that to be an out-of-the-box saving as artifact in logger.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist