Skip to content
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

Merged
merged 7 commits into from
Aug 17, 2024
Merged

Fix log_config #1432

merged 7 commits into from
Aug 17, 2024

Conversation

josejg
Copy link
Contributor

@josejg josejg commented Aug 6, 2024

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

@josejg josejg requested a review from a team as a code owner August 6, 2024 06:33
Copy link
Contributor

@snarayan21 snarayan21 left a 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?

@snarayan21 snarayan21 requested a review from b-chu August 7, 2024 19:55
Copy link
Contributor

@b-chu b-chu left a 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?

llmfoundry/utils/config_utils.py Outdated Show resolved Hide resolved
@snarayan21 snarayan21 requested a review from dakinggg August 7, 2024 20:08
Copy link
Contributor

@snarayan21 snarayan21 left a comment

Choose a reason for hiding this comment

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

chilling

@snarayan21 snarayan21 enabled auto-merge (squash) August 7, 2024 20:27
@b-chu b-chu disabled auto-merge August 7, 2024 21:05
@b-chu
Copy link
Contributor

b-chu commented Aug 7, 2024

Disabling automerge because I want to see how logging UI changed. Feel free to merge after posting

Do you have a screenshot of wandb and mlflow before and after this change?

@snarayan21 snarayan21 enabled auto-merge (squash) August 7, 2024 21:41
@snarayan21 snarayan21 disabled auto-merge August 7, 2024 21:41
@snarayan21
Copy link
Contributor

@josejg merge whenever you're ready

Copy link
Contributor

@snarayan21 snarayan21 left a 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?

@josejg josejg merged commit d5ed9e7 into mosaicml:main Aug 17, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants