-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Fix RequestCounter to make it more future-proof #27406
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.
Thanks for fixing!
The documentation is not available anymore as the PR was closed or merged. |
self.assertEqual(counter.other_request_count, 0) | ||
self.assertEqual(counter["GET"], 0) | ||
self.assertEqual(counter["HEAD"], 1) | ||
self.assertEqual(counter.total_calls, 1) |
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.
IIRC, this means no other request
, so we are good as before, right (i.e. no extra requests)
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.
Yep exactly! I'm counting all request types separately :)
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 need to learn more python, but don't want to block this PR. Thank you @Wauplin for the fix. Just a tiny question
I will merge once something on main is fixed. (should be soon) |
* Fix RequestCounter to make it more future-proof * code quality
Related to #27389 and #27393 (cc @amyeroberts @ydshieh).
The problem came from the fact that
transformers
test suite was trying to mockhuggingface_hub.utils.get_session
which is an internal method that I sometimes move/adapt for my needs. It shouldn't change much in the future but to make it more future-proof I chose to intercept theurllib3
debug logs directly. It's not perfect as urllib3 might change their logs + we could usepytest.caplog
instead but at this stage I think we are fine.I also unskipped the 2 tests that were previously failing.