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 logging and force flushing for run events #2703

Merged
merged 7 commits into from
Nov 9, 2023
Merged

Conversation

jjanezhang
Copy link
Contributor

@jjanezhang jjanezhang commented Nov 9, 2023

Adding logging and force flushing for run events

We are seeing run events inconsistently sent to run metadata for finetuning runs. Adding logs to help debug and force flushing metadata when run event related metadata is being written to the run.

Testing

image image

@jjanezhang jjanezhang self-assigned this Nov 9, 2023
@jjanezhang jjanezhang requested a review from j316chuck November 9, 2023 00:55
Copy link
Contributor

@j316chuck j316chuck left a comment

Choose a reason for hiding this comment

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

QQ: are these dditional logs generally helpful for future debugging or are they just useful for debugging the issues we are seeing in the current run?

composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
composer/loggers/mosaicml_logger.py Outdated Show resolved Hide resolved
@jjanezhang
Copy link
Contributor Author

QQ: are these dditional logs generally helpful for future debugging or are they just useful for debugging the issues we are seeing in the current run?

I think if we get logging to occur consistently, then we can remove these.

@jjanezhang jjanezhang requested a review from j316chuck November 9, 2023 01:34
@j316chuck
Copy link
Contributor

j316chuck commented Nov 9, 2023

@jjanezhang should we not merge this change then and simply set the integration in the RunConfig when debugging the issue?

- integration_type: git_repo
  git_repo: mosaicml/composer
  git_branch: jane/re-logs
  pip_install: '--upgrade -e .[all]'

Alternatively if you think this improves the mosaicml_logger generally then we should merge it.

@dakinggg
Copy link
Contributor

dakinggg commented Nov 9, 2023

@j316chuck not a lot of harm in adding debug logs :)

@jjanezhang
Copy link
Contributor Author

jjanezhang commented Nov 9, 2023

@jjanezhang should we just dev/test on this branch and not merge this then by setting the

- integration_type: git_repo
  git_repo: mosaicml/composer
  git_branch: jane/re-logs
  pip_install: '--upgrade -e .[all]'

I can't add integrations to a finetune yaml so the best I can do is submit a normal run with a ft generated yaml and then add the integrations. If this is really a concern I can just test with the later, but I would prefer to be able to check the logs from a finetune run.

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.

Approved with one minor comment

composer/loggers/mosaicml_logger.py Outdated 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.

LGTM -- agree with Daniel to make sure logs look OK and not spammy. Spammy logs = no one reads anything

@jjanezhang jjanezhang enabled auto-merge (squash) November 9, 2023 22:38
@jjanezhang jjanezhang merged commit 39c3a99 into dev Nov 9, 2023
16 checks passed
@jjanezhang jjanezhang deleted the jane/re-logs branch November 9, 2023 22:48
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