Add support for httpx as backend #1085
Conversation
|
I started wondering whether The way that |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1085 +/- ##
==========================================
- Coverage 91.12% 90.59% -0.53%
==========================================
Files 66 67 +1
Lines 6994 7303 +309
==========================================
+ Hits 6373 6616 +243
- Misses 621 687 +66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jakkdl
changed the title
jakkdl
changed the title
|
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? |
|
@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 |
|
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 😅 |
.github/workflows/reusable-test.yml
Outdated
| if: ${{ ! inputs.no-httpx }} | ||
| env: | ||
| COLOR: 'yes' | ||
| run: | | ||
| uv run -v --all-extras make mototest | ||
| - name: Run unittests without httpx installed | ||
| if: ${{ inputs.no-httpx }} |
There was a problem hiding this comment.
should the normal tests run with boto3 and awscli? I don't think they did previously but uv is still confusing me.
There was a problem hiding this comment.
No, they are not installed as part of CI.
There was a problem hiding this comment.
Why don't we run three sets of tests to cover everything?
- without httpx installed - corresponds to current test suite
- with httpx installed, but not configured to be used by aiobotocore - to protect against regressions from changes introduced as part of this PR
- with httpx installed and configured to be used by aiobotocore - test experimental functionality introduced by this PR
httpx is specified as an optional dependency and is easily installed by using uv run --extra httpx ..., see uv docs.
I'm not very familiar with pytest configuration, but it looks like you have figured that part out already.
There was a problem hiding this comment.
the second part is effectively tested through the way pytest is currently parametrizing tests, --http-backend=all runs each test twice
There was a problem hiding this comment.
cool @jakob-keller you satisfied with where we're at with this?
There was a problem hiding this comment.
I've stated how I would set up the test suite:
Why don't we run three sets of tests to cover everything?
- without httpx installed - corresponds to current test suite
- with httpx installed, but not configured to be used by aiobotocore - to protect against regressions from changes introduced as part of this PR
- with httpx installed and configured to be used by aiobotocore - test experimental functionality introduced by this PR
As I understand the proposed test suite, two jobs are run which cover parts of that proposed strategy:
- The job
Run unittestsmixes 2. and 3. I don't feel qualified to judge if that is properly implemented. - The job
Run unittests without httpx installedcovers 1 - at least partially.
It seems to work and CI passes. I'm really not in a position to go beyond that.
There was a problem hiding this comment.
yeah, we have one CI job on each python version with --http-backend=all that tests both the aiohttp and httpx backend. This is not perfect currently, as I noticed some tests that directly create internal classes to test them (instead of using e.g. the s3_client fixture). I suspect some minor bugs in the httpx backend are sneaking through because of this, but the tests will need bigger overhauls with #749 so I don't see much reason to delay this PR for now. There should be no risk that issues with the aiohttp backend are sneaking through.
We only run a single CI job for checking without httpx installed, which is mostly just to catch ImportError. I don't see any reason to extend this to testing on all python versions, and this is what I've done in e.g. pytest. (I'm assuming this is what you mean by "partially").
tl;dr no this isn't perfectly testing httpx, but we're explicitly calling it an experimental release and I plan on continuing work once this PR gets merged.
.github/workflows/reusable-test.yml
Outdated
| if: ${{ ! inputs.no-httpx }} | ||
| env: | ||
| COLOR: 'yes' | ||
| run: | | ||
| uv run -v --all-extras make mototest | ||
| - name: Run unittests without httpx installed | ||
| if: ${{ inputs.no-httpx }} |
There was a problem hiding this comment.
Why don't we run three sets of tests to cover everything?
- without httpx installed - corresponds to current test suite
- with httpx installed, but not configured to be used by aiobotocore - to protect against regressions from changes introduced as part of this PR
- with httpx installed and configured to be used by aiobotocore - test experimental functionality introduced by this PR
httpx is specified as an optional dependency and is easily installed by using uv run --extra httpx ..., see uv docs.
I'm not very familiar with pytest configuration, but it looks like you have figured that part out already.
There was a problem hiding this comment.
Thanks again for pushing this forward!
All my comments are in and many have been addressed. I do not feel familiar enough with the test suite to give qualified feedback there beyond what I already provided. From my perspective, there are no blocking issues remaining.
From a technical perspective, the PR needs to be rebased unto main and all commit need verified signatures.
As soon as that is completed, I would like to ask @thehesiod and/or @webknjaz to perform a final review and their approval.
|
squashed to get it all into a verified commit, and merged main. @jakob-keller you'll need to approve to unblock the CI as well since you requested changes in a previous review. |
Can you dismiss the review? I don't know, how to do that. |
I don't think I can.
Files changed -> Review changes [green button top right] -> tick the |
Sorry, but I don't feel qualified to approve this PR in its entirety. |
no yeah I get that, but I think you need to do that at some point to unblock CI even if it gets approved by thehesiod/webknjaz |
I think it's only the "conversations" that need to be marked as resolved. This is independent of any approvals. |
|
cool where do we stand, I can re-review. I was unsure as to the build changes as that was recently revamped. if those are good I can look at the rest |
|
ah just caught up. will re-review later today |
|
@jakkdl we have some build failures, 3.14 |
These are experimental tests and are currently expected to fail. Don't worry about it. |
|
awesome work guys, sorry for delay, crazy as usual here :) |
|
aaand published ;). @jakob-keller I had to manually approve the pypi deploy on master, is that normal process now? seems odd |
|
another weird thing is it then had a 15m wait timer for "protection rules" |
|
Yes, we've had this for a while now. I believe @webknjaz put that in last year when we redid CI/CD. |
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_patchespreviously 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.AbstractResolverwhich 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.