-
Notifications
You must be signed in to change notification settings - Fork 530
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
Point to composer.callback.Generate #631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please manually test this? both with old args and new args. Also I know there isn't a unit test for this callback right now...but could you please add one?
Yep, will do! Planning to test with https://github.com/mosaicml/mcloud/pull/2529, and I'll add some unit tests too Although since the |
There are unit tests for Generate in composer that test for functionality. Is there something else we should be unit testing in foundry for this callback? I think the builder tests that are in this PR may be sufficient. cc: @dakinggg |
Co-authored-by: Daniel King <43149077+dakinggg@users.noreply.github.com>
Co-authored-by: Daniel King <43149077+dakinggg@users.noreply.github.com>
This removes the llm-foundry
Generate
callback in favor ofcomposer.callback.Generate
, which has newer features like logging to MLflow. I've deleted the existingGenerate
callback logic but kept it around just in case someone is directly importing from llm-foundry. The builder callback also supports convertingbatch_log_interval
tointerval
for backwards compatibilityIn a future llmfoundry release, should remove this callback entirely
example of logging to mlflow & wandb