Skip to content

Conversation

@davidefiocco
Copy link
Contributor

@davidefiocco davidefiocco commented Oct 26, 2020

What does this PR do?

I propose this PR to allow transformers to call AzureML logging using https://docs.microsoft.com/en-us/python/api/azureml-core/azureml.core.run(class)?view=azure-ml-py

The intended behaviour is to enable transformers users to track metrics in the AzureML UI in this fashion

image

Contributors to https://github.com/microsoft/AzureML-BERT and folks @microsoft may well come up with a better implementation though!

I am glad to improve the following if reviewers like the idea, and update docs and tests if needed.
@reviewers feel free to add any suggestions as my contributions to transformers have been very limited so far :)

Before submitting

Who can review?

@julien-c is aware of this and @sgugger participated in the thread on forums above and implemented callbacks with #7596

@davidefiocco davidefiocco mentioned this pull request Oct 26, 2020
@davidefiocco davidefiocco changed the title Add AzureML logging in integrations via dedicated callback Add AzureML in integrations via dedicated callback Oct 26, 2020
Copy link
Collaborator

@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.

Looks great!
There is just a conflict to solve (probably with MLFlow merged this morning) and the CI tools to make happy (your import might need some comment to be ignored by black/isort).

@davidefiocco davidefiocco marked this pull request as draft October 26, 2020 22:44
@davidefiocco
Copy link
Contributor Author

Code changes pass tests @sgugger ! Thanks @alvarobartt for having a look at those also.

I guess I need to take care of docs/source/main_classes/callback.rst as well to complete the checklist though?

@davidefiocco davidefiocco marked this pull request as ready for review October 27, 2020 12:26
Copy link
Member

@alvarobartt alvarobartt left a comment

Choose a reason for hiding this comment

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

Everything keeps the standards and passes all the checks (black, isort) 👍🏻

@sgugger
Copy link
Collaborator

sgugger commented Oct 27, 2020

Yes, if you could add a line to the rst file, that would be great!

@davidefiocco davidefiocco marked this pull request as draft October 27, 2020 13:39
@sgugger
Copy link
Collaborator

sgugger commented Oct 27, 2020

You might need to rebase to have the latest script for doc formatting, which should do everything to make the CI happy with just make style. Let me know if you need help.

@davidefiocco
Copy link
Contributor Author

davidefiocco commented Oct 27, 2020

Sorry for the somewhat messy hacktoberfest @sgugger !
I wasn't so aware of rst idiosyncrasies, and ways to diagnose issues, so I struggled a bit. Should be in order now (bonus, I fixed a typo on the way 286f20c)

@davidefiocco davidefiocco marked this pull request as ready for review October 27, 2020 16:08
@sgugger sgugger requested a review from LysandreJik October 27, 2020 16:10
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

As you mention hacktoberfest, we had not thought of adding the label but we just did. I hope this PR will be counted towards your goal!

@LysandreJik LysandreJik merged commit 995006e into huggingface:master Oct 27, 2020
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