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

AioAWSResponse and AioAWSRequest #934

Merged
merged 12 commits into from
May 5, 2022
Merged

AioAWSResponse and AioAWSRequest #934

merged 12 commits into from
May 5, 2022

Conversation

thehesiod
Copy link
Collaborator

@thehesiod thehesiod commented May 5, 2022

There are special decoding operations performed by these classes that we need in certain circumstances. This moves us closer to botocore, however requires us to override even more methods :(

Missing:

special.py

  • RetryDDBChecksumError.is_retryable (needs RetryHandler.needs_retry down)

retryhandler.py

  • CRC32Checker._check_response

@thehesiod thehesiod requested a review from terricain May 5, 2022 04:48
@thehesiod thehesiod linked an issue May 5, 2022 that may be closed by this pull request
6 tasks
@thehesiod thehesiod mentioned this pull request May 5, 2022
@thehesiod
Copy link
Collaborator Author

@achimgaedke please let me know if this works for you

@thehesiod
Copy link
Collaborator Author

holy cow is this a pain

@achimgaedke
Copy link

@achimgaedke please let me know if this works for you

Hey! Thanks for the hard work. It sounds like Whak-A-Mole...

I have taken one version 2 hours ago and used it with DVC, that's the realistic use case:

I got many of those errors:

ERROR: failed to transfer 'md5: 37dee44a04f930b99d26ad16217dcfd1' - ClientResponseProxy.read() takes 1 positional argument but 2 were given
ERROR: failed to transfer 'md5: b52cca673fe356892b47ea513c893efd' - ClientResponseProxy.read() takes 1 positional argument but 2 were given

I'll rebuild the DS workbench again with the updated branch and see how it goes.

and many more...

@thehesiod
Copy link
Collaborator Author

I've fixed a bunch of bugs, have you tried the latest?

@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@thehesiod
Copy link
Collaborator Author

ok @achimgaedke I've fixed the last tests

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 4 alerts and fixes 4 when merging d131caa into 4694ba4 - view on LGTM.com

new alerts:

  • 2 for Missing call to `__init__` during object initialization
  • 1 for Unreachable 'except' block
  • 1 for Unused import

fixed alerts:

  • 2 for `__eq__` not overridden when adding attributes
  • 1 for Unreachable 'except' block
  • 1 for Unused import

@achimgaedke
Copy link

achimgaedke commented May 5, 2022

ok @achimgaedke I've fixed the last tests

Yep, it works perfectly for me now.
Thanks a lot for all your work.

I just got aware that the increment from 2.2 to 2.3 will prevent this fix from flowing on.

pip install git+https://github.com/aio-libs/aiobotocore.git@thehesiod/awsresponse

...

s3fs 2022.3.0 requires aiobotocore~=2.2.0, but you have aiobotocore 2.3.0 which is incompatible.
Successfully installed aiobotocore-2.3.0

"Doing some adverstising" for this PR.

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #934 (d50fe81) into master (4694ba4) will decrease coverage by 0.45%.
The diff coverage is 84.44%.

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
- Coverage   87.12%   86.66%   -0.46%     
==========================================
  Files          46       52       +6     
  Lines        4969     5265     +296     
==========================================
+ Hits         4329     4563     +234     
- Misses        640      702      +62     
Flag Coverage Δ
unittests 86.66% <84.44%> (-0.46%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiobotocore/_endpoint_helpers.py 100.00% <ø> (ø)
aiobotocore/httpchecksum.py 25.00% <25.00%> (ø)
aiobotocore/response.py 75.67% <33.33%> (ø)
aiobotocore/retries/special.py 40.00% <40.00%> (ø)
aiobotocore/handlers.py 63.63% <74.07%> (+16.57%) ⬆️
aiobotocore/retryhandler.py 82.24% <82.24%> (ø)
aiobotocore/httpsession.py 78.44% <88.88%> (+4.81%) ⬆️
aiobotocore/utils.py 76.31% <94.28%> (+5.22%) ⬆️
aiobotocore/awsrequest.py 94.44% <94.44%> (ø)
aiobotocore/client.py 86.18% <95.00%> (+1.09%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4694ba4...d50fe81. Read the comment docs.

@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 3 alerts and fixes 4 when merging d50fe81 into 4694ba4 - view on LGTM.com

new alerts:

  • 3 for Missing call to `__init__` during object initialization

fixed alerts:

  • 2 for `__eq__` not overridden when adding attributes
  • 1 for Unreachable 'except' block
  • 1 for Unused import

@aio-libs aio-libs deleted a comment from lgtm-com bot May 5, 2022
@thehesiod
Copy link
Collaborator Author

I did 2.2 -> 2.3 because I fixed some of the exception conversions due to except ordering. I'm going to merge and then if there are issues we can issue patch releases.

@thehesiod thehesiod merged commit 0c6c571 into master May 5, 2022
@thehesiod thehesiod deleted the thehesiod/awsresponse branch May 5, 2022 22:41
@terricain
Copy link
Collaborator

oof that did look painful, nice job though, looks great :)

@thehesiod
Copy link
Collaborator Author

@terrycain thanks! ugh, just realized today that 3.6 tests weren't actually running in 3.6 due to pipenv "upgrading" the python env :(. Another pain. Landing that shortly

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

Successfully merging this pull request may close these issues.

S3 error : Unable to locate credentials
3 participants