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

Adding Model Data Init and Training Progress to MosaicMLLogger #2633

Merged
merged 32 commits into from
Oct 26, 2023

Conversation

jjanezhang
Copy link
Contributor

@jjanezhang jjanezhang commented Oct 11, 2023

Adding Model Data Init and Training Progress to MosaicMLLogger

  1. Logs timestamp when model data is downloaded and initialized
  2. Logs training progress metrics whenever we log to metadata

Testing
Started a run and confirmed that training metrics and data initialized are in run's metadata
image

@jjanezhang jjanezhang self-assigned this Oct 11, 2023
Copy link
Contributor

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

curious what @mvpatel2000 and @eracah think, but do we need a new callback for this? That is, do we actually want to log this to all logger destinations? or should we just have the MosaicMLLogger do this?

@mvpatel2000
Copy link
Contributor

I think it's fine to log to one or all logger destinations -- the others just won't really care since and the UX is clean on most of them. I've done one offs like this using the logger API and not had issues.

I'm not sure this belongs in Composer though. The callbacks in Composer should be foundational callbacks which are broadly applicable. This feels like a callback for a bespoke use case which probably better belongs in whatever repo is leveraging it? @jjanezhang feel free to start a conversation in the composer channel if you feel it's appropriate

@jjanezhang jjanezhang changed the title RunEvents Callback Adding Data Init to MosaicMLLogger Oct 13, 2023
@jjanezhang jjanezhang requested a review from eracah as a code owner October 13, 2023 18:56
@jjanezhang jjanezhang requested a review from dakinggg October 15, 2023 02:29
composer/loggers/mosaicml_logger.py Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
tests/loggers/test_mosaicml_logger.py Show resolved Hide resolved
tests/loggers/test_mosaicml_logger.py Outdated Show resolved Hide resolved
tests/loggers/test_mosaicml_logger.py Outdated Show resolved Hide resolved
@jjanezhang jjanezhang changed the title Adding Data Init to MosaicMLLogger Adding Model Data Init and Training Progress to MosaicMLLogger Oct 20, 2023
Copy link
Contributor

@irenedea irenedea left a comment

Choose a reason for hiding this comment

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

Nice! Had some comments and questions

composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
tests/loggers/test_mosaicml_logger.py Outdated Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
@jjanezhang jjanezhang requested a review from irenedea October 24, 2023 00:45
Copy link
Contributor

@irenedea irenedea left a comment

Choose a reason for hiding this comment

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

Looks good, awesome work! Just some nits and wanted to double check that moving to batch_end is the right call 😄

tests/loggers/test_mosaicml_logger.py Outdated Show resolved Hide resolved
tests/loggers/test_mosaicml_logger.py Outdated Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
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.

Requesting sign off from me for progress logic before this lands. Can review fast whenever Jane requests :)

@mvpatel2000
Copy link
Contributor

@jjanezhang will review when tests pass :)

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! Sorry this took a bit to review

composer/loggers/mosaicml_logger.py Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
@jjanezhang jjanezhang merged commit b361833 into dev Oct 26, 2023
15 checks passed
@jjanezhang jjanezhang deleted the jane/training-events branch October 26, 2023 22:18
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