Change TimeoutError => asyncio.TimeoutError

[matthewgrossman]

Problem

I'm currently running python 3.9 with the huggingface_hub AsyncInferenceClient library.

When a request times out, the library is supposed to catch the TimeoutError and reraise a custom InferenceTimeoutError: https://github.com/huggingface/huggingface_hub/blob/3734d74/src/huggingface_hub/inference/_generated/_async_client.py#L230-L233

However, when these exceptions occurred for me, I'd never catch the custom error; I'd get an asyncio.TimeoutError.

The reason this is happening is because a TimeoutError and asyncio.TimeoutError are different exceptions in python3.9:

• py39 docs: https://docs.python.org/3.9/library/asyncio-exceptions.html

  Important This exception is different from the builtin TimeoutError exception.

• py311 docs: https://docs.python.org/3/library/asyncio-exceptions.html

  Changed in version 3.11: This class was made an alias of TimeoutError.

Because this library never catches the actual error, it never calls await client.close(). This doesn't have huge consequence, but it means I get noisy error logs about unclosed sessions (source):

Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7fcee450ff10>

Solution

I think the easiest fix here is to simply catch asyncio.TimeoutError in the AsyncInferenceClient timeout-path. Because newer versions of python alias this library, I see no reasons why it wouldn't be forward-compatible either

Commentary

I can add some testcases / reproducible steps to this, but want to make sure the repo owners are amenable to this change. I did read the contributing docs, but I just want to make sure the repo doesn't have a policy around "only support python version 3.X+" or something

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[Wauplin]

Hi @matthewgrossman thanks for opening this PR and for the clear explanation of the problem. I've never encountered this problem as I was testing async TimeoutError only locally and on Python 3.11. The fix you propose looks good to me.

I just want to make sure the repo doesn't have a policy around "only support python version 3.X+" or something

We want to support Python3.8+ so this PR is definitely helpful!

[Wauplin]

Tests are flaky at the moment. I'll merge this PR right now as the tests are not related to your PR. Thanks for your contribution :)

[matthewgrossman]

Great, glad it's helpful! Let me know if you want anything else in the PR (I see now that CI does run py3.8 tests; could a unit test have caught this?)

otherwise it's ready to merge away 🎉

[Wauplin]

I see now that CI does run py3.8 tests; could a unit test have caught this?

@matthewgrossman Thanks for suggesting! Indeed adding a unit test would be good to as well. I would add it to test_inference_async_client.py. Let me know if you need help/guidance/review for that :)

[matthewgrossman]

I locally removed the fix, just to confirm the test validates the behavior:

$ git diff

diff --git a/src/huggingface_hub/inference/_generated/_async_client.py b/src/huggingface_hub/inference/_generated/_async_client.py
index ad068c6..546847f 100644
--- a/src/huggingface_hub/inference/_generated/_async_client.py
+++ b/src/huggingface_hub/inference/_generated/_async_client.py
@@ -228,7 +228,7 @@ class AsyncInferenceClient:
                         content = await response.read()
                         await client.close()
                         return content
-                except asyncio.TimeoutError as error:
+                except TimeoutError as error:
                     await client.close()
                     # Convert any `TimeoutError` to a `InferenceTimeoutError`
                     raise InferenceTimeoutError(f"Inference call timed out: {url}") from error

 ~/src/huggingface_hub  mg/fix-asyncio-timeouterror  » python3 --version                                                                                  
Python 3.9.16

 ~/src/huggingface_hub  mg/fix-asyncio-timeouterror  » python3 -m pytest tests/test_inference_async_client.py::test_async_generate_timeout_error
...                                                                                                                                                                                    
tests/test_inference_async_client.py::test_async_generate_timeout_error FAILED

When I remove the local changes, it starts to pass (which we should of course see once you approve the workflow for CI as well). Feel free to give any feedback on the test I wrote!

[Wauplin]

That's a very clean and complete PR, thanks @matthewgrossman! 🎉 Test looks good to me. Fast and simple as I like them ❤️

I've triggered the CI. Let's wait until it's ✔️ and I'll merge :)

EDIT: all green!