Repository: huggingface/huggingface_hub
Pull request #1666 (2023-09-15 05:17:38)

Change `TimeoutError` => `asyncio.TimeoutError`
# 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](https://docs.python.org/3.9/library/exceptions.html#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](https://docs.python.org/3/library/exceptions.html#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](https://github.com/matthewgrossman/huggingface_hub/blob/1214a2d7b545b27275f0b2d4060b5d70ccca8867/src/huggingface_hub/inference/_generated/_async_client.py#L232)):
```
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](https://github.com/huggingface/huggingface_hub/blob/main/CONTRIBUTING.md#submitting-a-new-issue-or-feature-request), 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 docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_1666). All of your documentation changes will be reflected on that endpoint.
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](https://github.com/huggingface/huggingface_hub/blob/main/tests/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 -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 see once you approve the workflow for CI). Feel free to give any feedback on the test I wrote!

Patch:
diff --git a/src/huggingface_hub/inference/_generated/_async_client.py b/src/huggingface_hub/inference/_generated/_async_client.py
index 93fe892f..ad068c6a 100644
--- a/src/huggingface_hub/inference/_generated/_async_client.py
+++ b/src/huggingface_hub/inference/_generated/_async_client.py
@@ -18,6 +18,7 @@
 # Any change in InferenceClient will be automatically reflected in AsyncInferenceClient.
 # To re-generate the code, run `make style` or `python ./utils/generate_async_inference_client.py --update`.
 # WARNING
+import asyncio
 import logging
 import time
 import warnings
@@ -227,7 +228,7 @@ class AsyncInferenceClient:
                         content = await response.read()
                         await client.close()
                         return content
-                except TimeoutError as error:
+                except asyncio.TimeoutError as error:
                     await client.close()
                     # Convert any `TimeoutError` to a `InferenceTimeoutError`
                     raise InferenceTimeoutError(f"Inference call timed out: {url}") from error
diff --git a/utils/generate_async_inference_client.py b/utils/generate_async_inference_client.py
index 3288a89d..f24ecc58 100644
--- a/utils/generate_async_inference_client.py
+++ b/utils/generate_async_inference_client.py
@@ -140,7 +140,10 @@ def _add_imports(code: str) -> str:
     code = re.sub(
         r"(\nimport .*?\n)",
         repl=(
-            r"\1" + "from .._common import _async_yield_from, _import_aiohttp\n" + "from typing import AsyncIterable\n"
+            r"\1"
+            + "from .._common import _async_yield_from, _import_aiohttp\n"
+            + "from typing import AsyncIterable\n"
+            + "import asyncio\n"
         ),
         string=code,
         count=1,
@@ -201,7 +204,7 @@ ASYNC_POST_CODE = """
                         content = await response.read()
                         await client.close()
                         return content
-                except TimeoutError as error:
+                except asyncio.TimeoutError as error:
                     await client.close()
                     # Convert any `TimeoutError` to a `InferenceTimeoutError`
                     raise InferenceTimeoutError(f"Inference call timed out: {url}") from error


Test patch:
diff --git a/tests/test_inference_async_client.py b/tests/test_inference_async_client.py
index ec34425a..1d827e5a 100644
--- a/tests/test_inference_async_client.py
+++ b/tests/test_inference_async_client.py
@@ -22,13 +22,14 @@ return types + uses streaming requests on demand). Tests are mostly duplicates f
 For completeness we also run a test on a simple task (`test_async_sentence_similarity`) and assume all other tasks
 work as well.
 """
+import asyncio
 import inspect
 
 import pytest
 from aiohttp import ClientResponseError
 
 import huggingface_hub.inference._common
-from huggingface_hub import AsyncInferenceClient, InferenceClient
+from huggingface_hub import AsyncInferenceClient, InferenceClient, InferenceTimeoutError
 from huggingface_hub.inference._common import _is_tgi_server
 from huggingface_hub.inference._text_generation import FinishReason, InputToken
 from huggingface_hub.inference._text_generation import ValidationError as TextGenerationValidationError
@@ -243,3 +244,13 @@ async def test_list_deployed_models_single_frameworks() -> None:
 
     assert "text-generation" in models_by_task
     assert "bigscience/bloom" in models_by_task["text-generation"]
+
+
+@pytest.mark.asyncio
+async def test_async_generate_timeout_error(monkeypatch: pytest.MonkeyPatch) -> None:
+    def _mock_aiohttp_client_timeout(*args, **kwargs):
+        raise asyncio.TimeoutError
+
+    monkeypatch.setattr("aiohttp.ClientSession.post", _mock_aiohttp_client_timeout)
+    with pytest.raises(InferenceTimeoutError):
+        await AsyncInferenceClient(timeout=1).text_generation("test")