-
Notifications
You must be signed in to change notification settings - Fork 534
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
Fix log_config #1432
Fix log_config #1432
Conversation
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.
looks fine to me, given the changes around config logging would also like @b-chu to approve before merging.
Could you do a simple manual run off this branch to show the functionality working fine?
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.
Breaks BC, but I'll let @snarayan21 approve. Do you have a screenshot of wandb and mlflow before and after this change?
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.
chilling
Disabling automerge because I want to see how logging UI changed. Feel free to merge after posting
|
@josejg merge whenever you're ready |
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.
lgtm, just run name in description. Tests are failing though?
Foundry's train and eval scripts use the
log_config
helper to log the parameter config. However,log_config
breaks the logger abstraction by directly logging to wandb and/or mlflow.This PR uses the
logger.log_hyperparameter
method which relies on the same method calls for wandb and mlflow. In addition this PR allows custom loggers to be able to log the config which is not currently supported.EDIT: Run Name
210m-tpr100-adam-cool-linear-lr2d3-20pct-JvGeHi