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

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

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 requested a review from mvpatel2000 September 1, 2022 02:24
@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
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
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
.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.

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants