-
Notifications
You must be signed in to change notification settings - Fork 570
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
Logging fix for hf_api
, logging documentation
#748
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.
LG2M, should we also include a note on how to expose the logging functionality when running tests?
E.g.
pytest -sv tests/ --log-cli-level=INFO
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.
LG!
``` | ||
|
||
## Controlling the logging of `huggingface_hub` |
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.
Should this information be in README? How are we going to create the documentation for this library? We probably need quite a few user guides and I think this should go in one of those, which probably should be located in the /doc
folder.
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.
I'm putting this here for now, but this should definitely be in a separate guide as outlined in https://github.com/huggingface/huggingface_hub/issues/754
src/huggingface_hub/hf_api.py
Outdated
logging.error( | ||
"HfApi.login: This method is deprecated in favor of `set_access_token`." | ||
warnings.warn( | ||
"HfApi.login: This method is deprecated in favor of `set_access_token`.", |
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.
should we mention when it will be removed? Do we have a deprecation cycle policy?
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.
I'm adding a mention to v0.7
: we should remove this as soon as possible, but there are downstream libraries using it still, so it's important to give them time to switch.
There's no written deprecation cycle policy, but I agree it would be welcome.
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.
Thanks @LysandreJik , other than the formatting LGTM.
match=r"HfApi.login: This method is deprecated in favor of " | ||
r"`set_access_token` and will be removed in v0.7.", |
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.
I think this is not black formatted.
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.
Actually, it is! I've split it in two to respect the 88 line char limit, black seems okay with it
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.
Black unindented it, the second line was previously indented at the same level as the |
in match=|
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.
OK
This fixes an issue with the logging of
hf_api
, replaceslogger.error
for deprecation warnings by aFutureWarning
, and adds a small document regarding how to use logging withhuggingface_hub
.cc @FrancescoSaverioZuppichini as you have raised the issue with
hf_api
this morning.