Skip to content
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

The timeout parameter of aiohttp.ClientSession does work for types other than Union[ClientTimeout, _SENTINEL, None] #8602

Closed
1 task done
aucampia opened this issue Aug 5, 2024 · 5 comments

Comments

@aucampia
Copy link

aucampia commented Aug 5, 2024

Describe the bug

The typing for the timeout parameter of aiohttp.ClientSession is Union[ClientTimeout, _SENTINEL, None]

timeout: "Union[ClientTimeout, _SENTINEL, None]"

but the function is written to also accommodate values for timeout that is of float type:

aiohttp/aiohttp/client.py

Lines 1116 to 1124 in f1e4213

def get(
self, url: StrOrURL, *, allow_redirects: bool = True, **kwargs: Any
) -> "_RequestContextManager":
"""Perform HTTP GET request."""
return _RequestContextManager(
self._request(
hdrs.METH_GET, url, allow_redirects=allow_redirects, **kwargs
)
)

aiohttp/aiohttp/client.py

Lines 523 to 529 in f1e4213

if timeout is sentinel:
real_timeout: ClientTimeout = self._timeout
else:
if not isinstance(timeout, ClientTimeout):
real_timeout = ClientTimeout(total=timeout)
else:
real_timeout = timeout

aiohttp/aiohttp/client.py

Lines 185 to 191 in f1e4213

@attr.s(auto_attribs=True, frozen=True, slots=True)
class ClientTimeout:
total: Optional[float] = None
connect: Optional[float] = None
sock_read: Optional[float] = None
sock_connect: Optional[float] = None
ceil_threshold: float = 5

To Reproduce

Write some python code that passes a float/int value to aiohttp.ClientSession.get(...,timeout=...) and run mypy on it.

Expected behavior

No type error as the code will run correctly.

Logs/tracebacks

kron/clients/open_exchange_rates/client.py: note: In member "historical" of class "OpenExchangeRatesApiClient":
  kron/clients/open_exchange_rates/client.py:62: error: Argument "timeout" to "get" of "ClientSession" has incompatible type "int"; expected "ClientTimeout | _SENTINEL | None"  [arg-type]
  Found 1 error in 1 file (checked 2577 source files)

Python Version

$ python --version
Python 3.12.4

aiohttp Version

$ poetry run python -m pip show aiohttp

Name: aiohttp
Version: 3.10.1
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache 2
Location: /home/iwana/.cache/pypoetry/virtualenvs/kron-sw1LGez6-py3.12/lib64/python3.12/site-packages
Requires: aiohappyeyeballs, aiosignal, attrs, frozenlist, multidict, yarl
Required-by: gcloud-aio-auth, pytest-aiohttp

multidict Version

$ poetry run python -m pip show multidict

Name: multidict
Version: 6.0.5
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /home/iwana/.cache/pypoetry/virtualenvs/kron-sw1LGez6-py3.12/lib64/python3.12/site-packages
Requires: 
Required-by: aiohttp, yarl

yarl Version

$ poetry run python -m pip show yarl
Name: yarl
Version: 1.9.4
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: /home/iwana/.cache/pypoetry/virtualenvs/kron-sw1LGez6-py3.12/lib64/python3.12/site-packages
Requires: idna, multidict
Required-by: aiohttp

OS

Fedora Linux 40 (Xfce)

Related component

Server

Additional context

$ poetry run python -m pip show mypy
Name: mypy
Version: 1.10.1
Summary: Optional static typing for Python
Home-page: https://www.mypy-lang.org/
Author: Jukka Lehtosalo
Author-email: [email protected]
License: MIT
Location: /home/iwana/.cache/pypoetry/virtualenvs/kron-sw1LGez6-py3.12/lib64/python3.12/site-packages
Requires: mypy-extensions, typing-extensions
Required-by: 

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@aucampia aucampia added the bug label Aug 5, 2024
@bdraco bdraco added the regression Something that used to work stopped working "as before" after upgrade label Aug 5, 2024
@bdraco bdraco added this to the 3.10.2 milestone Aug 5, 2024
@Dreamsorcerer
Copy link
Member

It's there for backwards compatibility, but shouldn't be used to ensure forward compatibility with v4. Therefore, it's not in the typing information.

@Dreamsorcerer Dreamsorcerer closed this as not planned Won't fix, can't repro, duplicate, stale Aug 5, 2024
@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Aug 5, 2024

The documentation also has not mentioned float for many years. Here's the documentation from 3.7.2 (the oldest I can see on RTD):
https://docs.aiohttp.org/en/v3.7.2/client_reference.html#aiohttp.ClientSession

@aucampia
Copy link
Author

aucampia commented Aug 5, 2024

The documentation also has not mentioned float for many years. Here's the documentation from 3.7.2 (the oldest I can see on RTD): https://docs.aiohttp.org/en/v3.7.2/client_reference.html#aiohttp.ClientSession

Thanks for the feedback @Dreamsorcerer, we will just change our code to not pass a float/int then.

Technically, and somewhat pedantically, if you are following SEMVER, then float should still be valid, and the type hints should accept that, but alas just not passing int/float seems sensible due to the forward compatibility benefits.

@Dreamsorcerer
Copy link
Member

I think we prefer the typing information to help nudge people away from deprecated things. The only reason you didn't have a type error before is because it was **kwargs: Any. 3.10 just added Unpack support to get all the kwargs typed properly.

@bdraco bdraco removed this from the 3.10.2 milestone Aug 5, 2024
@bdraco bdraco removed bug regression Something that used to work stopped working "as before" after upgrade labels Aug 5, 2024
@gandhis1
Copy link

gandhis1 commented Aug 6, 2024

I encountered this issue as well upon a recent upgrade. I understand the motivation behind it, but I'm going to guess that there are a LOT of people who are using it with a float/int not knowing any better, because comparable libraries like requests and httpx support numeric values. i.e. such a change is somewhat disruptive

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

No branches or pull requests

4 participants