-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add support for httpx as backend #1085
base: master
Are you sure you want to change the base?
Conversation
aiobotocore/httpsession.py
Outdated
|
||
# previously data was wrapped in _IOBaseWrapper | ||
# github.com/aio-libs/aiohttp/issues/1907 | ||
# I haven't researched whether that's relevant with httpx. |
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.
ya silly decision of aiohttp, they took over the stream. Most likely httpx does the right thing. I think to get around the sync/async thing we can just make a stream wrapper that hides the relevant methods...I think I did this somewhere...will try to remember
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.
Would the current tests catch if httpx didn't do the right thing?
I started wondering whether The way that |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1085 +/- ##
==========================================
- Coverage 89.84% 89.06% -0.78%
==========================================
Files 67 67
Lines 6086 6349 +263
==========================================
+ Hits 5468 5655 +187
- Misses 618 694 +76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Whooooo, all tests are passing!!!! current TODOs:
codecov is very sad, but most of that is due to me duplicating code that wasn't covered to start with, or extending tests that aren't run in CI. I'll try to make it very slightly less sad, but making it completely unsad is very much out of scope for this PR. Likewise RTD is failing ... and I think that's unrelated to the PR? |
Add no-httpx run to CI on 3.12 Tests can now run without httpx installed. Exclude `if TYPE_CHECKING` blocks from coverage. various code cleanup
…ive errors on connector args not compatible with httpx. Remove proxy code and raise NotImplementedError. fix/add tests
@thejcannon @aneeshusa if you wanna do a review pass |
Hey @thehesiod what's the feeling on this? It is turning out to be a messier and more disruptive change than initially thought in #749. I can pull out some of the changes to a separate PR to make this a bit smaller at least |
hey sorry been down with a cold, will look asap. I don't mind big PRs |
if httpx and isinstance(aio_session, httpx.AsyncClient): | ||
async with aio_session.stream("GET", presigned_url) as resp: | ||
data = await resp.aread() |
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.
what do you think of making an adapter class so no test changes are necessary. We can expose the raw stream via a new property if needed
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.
no test changes is not going to be possible - see #1085 (comment). But I could try to make one that implements a bunch of the basic functionality which would minimize test changes.
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.
well, translating between await resp.read()
and await resp.aread()
with an adapter class is trivial.
But having an adapter class turn calls to resp['Body'].close()
into await resp['Body'].aclose
...
I guess it's possible in theory to hook into the currently running async framework and run .aclose()
from a sync .close()
in an adapter class... but that feels like a very bad idea. Especially as we're looking to support anyio and structured concurrency.
I suppose I could write a wrapper class that .. only translates read
->aread
, and gives specific errors for the other ones? It could maybe help transitioning code currently written, but I think perhaps more appropriate is to if/when dropping aiohttp we make the adapter class raise DeprecationError
if calling .read()
or .close()
.
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 the issue is you didn't wrap the body in the StreamingBody, that will solve these issues. For the close
I think aiohttp did a bad decision, it should have been an async method. I think we should have a sync close method on the StreamingBody that creates a task around aclose and returns that task. That way if you call it sync it will be fine because eventually it will get closed, and if you want you can await it to wait for it to actually get closed.
tests/test_basic_s3.py
Outdated
if current_http_backend == 'httpx': | ||
assert key == 'key./name' | ||
else: | ||
assert key == 'key./././name' |
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.
hmm, ya this needs to be fixed. We can't have the response changing. This should match whatever botocore does, if current way is incorrect this is fine, otherwise needs to be fixed
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.
The current behaviour is indeed how botocore handles it:
https://github.com/boto/botocore/blob/970d577087d404b0927cc27dc57178e01a3371cd/tests/integration/test_s3.py#L599-L606
so I'll have to do some digging
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.
digging success!... but looks like it needs changes in upstream httpx to get fixed.
When we call self._session.build_request
in
aiobotocore/aiobotocore/httpsession.py
Lines 451 to 456 in b19bc09
httpx_request = self._session.build_request( | |
method=request.method, | |
url=url, | |
headers=headers, | |
content=content, | |
) |
httpx turns the string url into a httpx.URL
object, which explicitly normalizes the path https://github.com/encode/httpx/blob/392dbe45f086d0877bd288c5d68abf860653b680/httpx/_urlparse.py#L387
We can manually create a httpx.URL
object to be passed into httpx.build_request
, but there's no parameter that control normalization, so at that point we'd have to create a subclass of httpx.URL
or something to customize the behaviour.
I can open an issue for it in the httpx repo, but I'll first try to figure out why botocore seems to explicitly not want to normalize the path.
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.
Okay so the reason these slashes aren't normalized is because having slashes (and periods) in a key name is allowed
But I think if we can replace the slashes in the key name with %2F
somewhere along the chain then I think it'll be handled correctly.
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.
the current percent encoding happens deep within the guts of botocore, where they explicitly mark /
as safe for some reason: https://github.com/boto/botocore/blob/970d577087d404b0927cc27dc57178e01a3371cd/botocore/serialize.py#L520
Why? no clue. Can it be marked unsafe? I tried and ran unit tests and some functional tests in botocore/, and some tests did start to fail - though unclear if they're bad errors or just defensive asserts.
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.
This has been requested in httpx since forever: encode/httpx#1805
let's see if me offering to write a PR can give it some traction.
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.
ya main thing is downstream consumers may be doing string matching and could break logic
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.
appreciate the digging!
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.
turns out httpx does have a way to do it now! encode/httpx#1805 (reply in thread)
btw check out: https://awslabs.github.io/aws-crt-python/api/http.html#awscrt.http.HttpClientConnection perhaps we should go in that direction so it will be complete AWS impl |
discussion here: #1106 |
back from my trip, will look asap, also need to add Jacob as a helper to review these, so much to do, so little time ;) |
CI failure is/was pre-commit/pre-commit-hooks#1101 |
I could make some small improvements to the codecov (but a lot of it is because I've duplicated previously uncovered code), but otherwise I'm still mostly just waiting for review :) |
sorry for taking so long, mind updating this pr? looks pretty good, just needs the in-depth review I promised (and have failed to provide yet, sorry!) |
since switching over to uv I'm not quite sure how to properly do a run in CI where httpx isn't installed, but otherwise I think this should be good. I've definitely forgotten .. a lot of details though 😅 |
@@ -88,6 +88,15 @@ jobs: | |||
COLOR: 'yes' | |||
run: | | |||
uv run make mototest | |||
# TODO: I don't know how to do this properly with uv-managed venv |
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.
You could move httpx
from dependency group dev
into a new one, such as httpx
, and also make that dependency group a default one. Then it would be as simple as uv run --no-group httpx
.
DEFAULT_HTTP_SESSION_CLS = AIOHTTPSession | ||
|
||
|
||
async def convert_to_response_dict(http_response, operation_model): |
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.
While I personally like type annotations a lot, this code is based on upstream (botocore) and we strive to keep it closely aligned to simplify version bumps. Please do not add type annotations as part of this PR. Thanks!
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.
ya fact of life :(
@@ -272,7 +287,7 @@ async def _needs_retry( | |||
return False | |||
else: | |||
# Request needs to be retried, and we need to sleep | |||
# for the specified number of times. |
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.
The original comment was not a typo. Please revert.
thank you for the quick update! |
headers = http_response.headers | ||
response_dict: dict[str, Any] = { | ||
'headers': headers, |
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.
ya these type of code changes should be to a minimum, we compare this to: https://github.com/boto/botocore/blob/develop/botocore/endpoint.py#L42
if httpx and isinstance(http_response.raw, httpx.Response): | ||
response_dict['body'] = http_response.raw |
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 keep the StreamingBody
as this is our own version so we can adapt as necessary
@@ -59,11 +71,13 @@ def __init__( | |||
|
|||
# TODO: handle socket_options | |||
# keep track of sessions by proxy url (if any) | |||
self._sessions: Dict[Optional[str], aiohttp.ClientSession] = {} | |||
self._sessions: dict[str | None, aiohttp.ClientSession] = {} |
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 believe dict
is 3.10+, we still support 3.8 so needs to be Dict
conn_timeout: float | None | ||
read_timeout: float | None |
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.
nop?
@@ -280,3 +294,273 @@ async def send(self, request): | |||
message = 'Exception received when sending urllib3 HTTP request' | |||
logger.debug(message, exc_info=True) | |||
raise HTTPClientError(error=e) | |||
|
|||
|
|||
class HttpxSession: |
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.
probably worth moving this to httpxsession.py
@@ -286,6 +287,10 @@ async def test_sso_token_provider_refresh(test_case): | |||
cache_key = "d033e22ae348aeb5660fc2140aec35850c4da997" | |||
token_cache = {} | |||
|
|||
# deepcopy the test case so the test can be parametrized against the same | |||
# test case w/ aiohttp & httpx | |||
test_case = deepcopy(test_case) |
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.
oh man, parametrize
should be returning a frozendict. That's crazy of botocore changing global state in a test, we inherited this code from botocore: https://github.com/boto/botocore/blob/develop/tests/unit/test_tokens.py#L26
@pytest.fixture | ||
def skip_httpx(current_http_backend: str) -> None: | ||
if current_http_backend == 'httpx': | ||
pytest.skip('proxy support not implemented for httpx') |
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.
better put this in conftest.py
# TODO: think about better api and make behavior like in aiohttp | ||
resp['Body'].close() | ||
if httpx and isinstance(resp['Body'], httpx.Response): | ||
data = await resp['Body'].aread() |
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.
given body is our own StreamResponse wrapper, i prefer we expose a more sane read
method to match botocore, their whole thing of read
vs aread
is very strange design decision. This will also reduce the diff
@@ -4,6 +4,8 @@ | |||
|
|||
from .mock_server import AIOServer | |||
|
|||
# these tests don't currently care about aiohttp vs httpx |
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.
no need for cmt
ok did a first pass, after StreamingBody fix I can do another pass for exceptions |
awesome work btw! |
First step of #749 as described in #749 (comment)
I was tasked with implementing this, but it's been a bit of a struggle not being very familiar with aiohttp, httpx or aiobotocore - and there being ~zero in-line types. But I think I've fixed enough of the major problems that it's probably useful to share my progress.
There's a bunch of random types added. I can split those off into a separate PR or remove if requested. Likewise for
from __future__ import annotations
.TODO:
aiobotocore/aiobotocore/httpsession.py
Lines 478 to 534 in b19bc09
but AFAICT you can only configure proxies per-client in httpx. So need to move the logic for it, and cannot usebotocore.httpsession.ProxyConfiguration.proxy_[url,headers]_for(request.url)
BOTO_EXPERIMENTAL__ADD_PROXY_HOST_HEADERseems not possible to do when configuring proxies per-client?No longer TODOs after changing the scope to implement httpx alongside aiohttp:
test_patches
previously cared about aiohttp. That can probably be retired?tests.mock_server.AIOServer
?NotImplementedError
:use_dns_cache
: did not find any mentions of dns caches on a quick skim of httpx docsforce_close
: same. Can maybe find out more by digging into docs on what this option does in aiohttp.resolver
: this is anaiohttp.abc.AbstractResolver
which is obviously a no-go.yarl.URL(url, encoding=True)
. httpx does not support yarl. I don't know what this achieved (maybe the non-normalization??), so skipping it for now.Some extra tests would probably also be good, but not super critical when we're just implementing httpx alongside aiohttp.