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

MLflow improvements for metric performance - batching #23

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

MLflow improvements for metric performance - batching #23

wants to merge 5 commits into from

Conversation

wamartin-aml
Copy link

@wamartin-aml wamartin-aml commented Oct 15, 2021

Signed-off-by: Walter Martin wamartin@microsoft.com

What does this PR do?

This PR simplifies which integrations are needed in the case of both AzureML and MLflow being available and it batches metric logging for improved performance.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Signed-off-by: Walter Martin <wamartin@microsoft.com>
@wamartin-aml
Copy link
Author

@noise-field, @sgugger, I think you two are the best set up to review this PR

Signed-off-by: Walter Martin <wamartin@microsoft.com>
Copy link

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

This looks okay to me, though I'm not an expert in MlFlow.
If the change helps, it would be great to upstream it into the main Transformers repo! :-)

@wamartin-aml
Copy link
Author

This looks okay to me, though I'm not an expert in MlFlow. If the change helps, it would be great to upstream it into the main Transformers repo! :-)

that's the plan :)

Signed-off-by: Walter Martin <wamartin@microsoft.com>
Signed-off-by: Walter Martin <wamartin@microsoft.com>
Signed-off-by: Walter Martin <wamartin@microsoft.com>
@wamartin-aml
Copy link
Author

@jingyanwangms or @harshithapv could I get a review?

@harshithapv
Copy link

harshithapv commented Oct 27, 2021

@jingyanwangms or @harshithapv could I get a review?

@wamartin-aml this change looks good but if you plan to upstream to transformers repo, I would suggest you make the PR there. We sync with transformers master periodically. So your change will be taken from there anyways.

@wamartin-aml
Copy link
Author

Ok, will do!

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.

3 participants