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.

Sign up for GitHub

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

Jump to bottom

Add CometML Support #1490

Merged
merged 20 commits into from
Sep 8, 2022
Merged

Add CometML Support #1490

merged 20 commits into from
Sep 8, 2022

Conversation

eracah
Copy link
Contributor

@eracah eracah commented Sep 1, 2022 •
edited
Loading

CO-1027

  • Add CometMLLogger
  • Along for the ride:
    • log hyperparameters for all loggers in entrypoint (not just wandb)

Manual/E2E Tests:

Local Custom Entrypoint

Comet results
WandB results for comparison

Local YAHP Entrypoint

CometML
WandB

Remote YAHP Entrypoint

CometML
WandB

Remote Custom Entrypoint

Remote Bert Run Results with Comet
Corresponding WandB run

  • note: hyperparameters will be different because we are using Moin's custom entrypoint which explicitly logs hyperparameters to wandb

@eracah eracah marked this pull request as ready for review September 1, 2022 02:24
@eracah eracah requested a review from a team as a code owner September 1, 2022 02:24
mvpatel2000
mvpatel2000 reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

Are integration tests here feasible? Ideally we'd have something set up

composer/loggers/cometml_logger.py Show resolved Hide resolved
composer/loggers/cometml_logger.py Show resolved Hide resolved
docs/source/notes/run_name.md Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
karan6181
karan6181 reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@karan6181 karan6181 left a comment

Choose a reason for hiding this comment

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

Thank you for your Contribution. Left some comments. Also, do you have a plan to add a unit test for this change in tests/loggers/test_cometml_logger.py ?

composer/loggers/cometml_logger.py Outdated Show resolved Hide resolved
composer/loggers/cometml_logger.py Outdated Show resolved Hide resolved
composer/loggers/cometml_logger.py Outdated Show resolved Hide resolved
docs/source/notes/run_name.md Show resolved Hide resolved
hanlint
hanlint reviewed Sep 2, 2022
View reviewed changes
.ci/Jenkinsfile Outdated Show resolved Hide resolved
@eracah
Copy link
Contributor Author

eracah commented Sep 8, 2022

Ok, @karan6181, @mvpatel2000, I added a unit test. Let me know how it looks and if this PR is ready for merge.

mvpatel2000
mvpatel2000 approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

LGTM! Ideally we get integration tests at some point soon, but that's a later issue

karan6181
karan6181 approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@karan6181 karan6181 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@eracah eracah merged commit edb4949 into mosaicml:dev Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karan6181 karan6181 karan6181 approved these changes

@mvpatel2000 mvpatel2000 mvpatel2000 approved these changes

@hanlint hanlint Awaiting requested review from hanlint

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eracah @karan6181 @hanlint @mvpatel2000