Reduce duplicated logging code, standardize logs#777
Conversation
jpdunc23
left a comment
jpdunc23
left a comment
There was a problem hiding this comment.
Thanks for the nice cleanup. I noticed a couple of minor inconsistencies and an unused arg. Otherwise, LGTM.
| config=config, env_vars=env_vars, resumable=resumable, **kwargs | ||
| config = to_flat_dict(dataclasses.asdict(self)) | ||
| self.logging.configure_logging( | ||
| self.experiment_dir, log_filename, config=config, resumable=True |
There was a problem hiding this comment.
| self.experiment_dir, log_filename, config=config, resumable=True | |
| self.experiment_dir, log_filename, config=config, resumable=False |
There was a problem hiding this comment.
While the default to this helper was False, there was a hard-code resumable=True where it's called on generation_config.configure_wandb below, so I think resumable=True is no change in behavior.
Can you check and make sure you agree?
There was a problem hiding this comment.
Same comment as https://github.com/ai2cm/ace/pull/777/changes#r2835488269
| self.logging.configure_wandb( | ||
| config=config, env_vars=env_vars, resumable=resumable, **kwargs | ||
| self.logging.configure_logging( | ||
| self.experiment_dir, log_filename, config=config, resumable=True |
There was a problem hiding this comment.
| self.experiment_dir, log_filename, config=config, resumable=True | |
| self.experiment_dir, log_filename, config=config, resumable=False |
There was a problem hiding this comment.
Similarly, resumable=True is hard-coded where this is called in practice. Not sure if this was intended, strange to have it for an inference-like entrypoint.
There was a problem hiding this comment.
Yes, I see from the diff that in main() we had evaluator_config.configure_wandb(resumable=True, notes=beaker_url). So I understand you're preserving that behavior, but why not add a resumable: bool = False default arg to EvaluatorConfig.configure_logging and pass resumable=True when calling evaluator_config.configure_logging below in main()? This would preserve the existing behavior of EvaluatorConfig.
There was a problem hiding this comment.
When I passed through this file the existing code was quite confusing - why is a default set that never gets used? I expected we run this with resumable=False based on the config method, but then saw it's actually resumable=True from the code below. I updated it to the style used in the training scripts as a cleanup.
I could split this out into a separate PR? I'll do that on Monday.
| experiment_dir: str, | ||
| log_filename: str, | ||
| config: Mapping[str, Any], | ||
| resumable: bool = False, |
There was a problem hiding this comment.
The default value for resumable was changed from True to False. Please double check that resumable=True is explicitly passed everywhere configure_wandb was previously called without the resumable argument.
There was a problem hiding this comment.
Reverted the change back to default True.
| def configure_wandb( | ||
| self, env_vars: dict | None = None, resumable: bool = False, **kwargs | ||
| ): | ||
| config = to_flat_dict(dataclasses.asdict(self)) |
There was a problem hiding this comment.
Should remove to_flat_dict here (and other inference entrypoints) as was done in the training scripts?
There was a problem hiding this comment.
This was actually a bug - I hadn't added to_flat_dict to the wandb helper. But it's a good idea, I added it and removed these calls.
| experiment_dir=experiment_dir, | ||
| resumable=resumable, | ||
| dir=wandb_dir, | ||
| **kwargs, |
There was a problem hiding this comment.
**kwargs is not longer used. Do we need it?
This PR moves low-level logging logic into the configure_logging helper, which now configures all types of logging including wandb. All entrypoints now log all of the information we log at the start of runs. Changes: - `LoggingConfig.configure_logging` now takes in `config` and `resumable` arguments for configuring WandB. - Reduced duplicated logging code in train and inference entrypoints. - [ ] Tests added Resolves ai2cm#536
2 participants
This PR moves low-level logging logic into the configure_logging helper, which now configures all types of logging including wandb.
All entrypoints now log all of the information we log at the start of runs.
Changes:
LoggingConfig.configure_loggingnow takes inconfigandresumablearguments for configuring WandB.Reduced duplicated logging code in train and inference entrypoints.
Tests added
Resolves #536