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

Fix UnseekableStreamError during upload #1853

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Conversation

sir-sigurd
Copy link
Member

@sir-sigurd sir-sigurd commented Oct 13, 2020

Description

When boto's put_object() retries it calls seek(0) of Body, which calls progress callback with a negative value. tqdm.update() supports negative values since 4.32.

TODO

Use this section for work-in-progress pull requests. If you're using this section,
please tag your PR "WIP".

@sir-sigurd sir-sigurd force-pushed the fix-negative-tqdm-update branch 3 times, most recently from 50af86c to fe348f0 Compare October 13, 2020 17:36
@sir-sigurd sir-sigurd changed the title [WIP] Fix UnseekableStreamError during upload Fix UnseekableStreamError during upload Oct 13, 2020
@sir-sigurd sir-sigurd marked this pull request as ready for review October 13, 2020 17:37
@sir-sigurd
Copy link
Member Author

Might make sense to move the tests to a separate PR, if they look messy.

lambda session:
Config(signature_version=UNSIGNED)
if session.get_credentials() is None
else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if session.client('s3', config=None) would work (it does) and I think it's worth linking the docs in a comment: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html#boto3.session.Session.client

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand, could you elaborate on that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The reader knows that config=None can happen but without docs can't know if that call is legal or will crash boto


class S3UploadProgressTest(unittest.TestCase):
def setUp(self):
self.s3_client = boto3.client('s3', config=botocore.client.Config(signature_version=botocore.UNSIGNED))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice-to-have (not required) config=None case

docs/CHANGELOG.md Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 13, 2020

Codecov Report

Merging #1853 into master will increase coverage by 0.20%.
The diff coverage is 92.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1853      +/-   ##
==========================================
+ Coverage   89.57%   89.77%   +0.20%     
==========================================
  Files          62       62              
  Lines        7335     7367      +32     
==========================================
+ Hits         6570     6614      +44     
+ Misses        765      753      -12     
Flag Coverage Δ
#api-python 88.36% <92.50%> (+0.33%) ⬆️
#lambda 92.30% <ø> (ø)

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

Impacted Files Coverage Δ
api/python/setup.py 0.00% <ø> (ø)
api/python/quilt3/data_transfer.py 81.04% <50.00%> (+1.72%) ⬆️
api/python/tests/test_data_transfer.py 100.00% <100.00%> (ø)

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 9ceb8e6...bd9d4f6. Read the comment docs.

When boto's put_object() retries it calls seek(0) of Body, which
calls progress callback with a negative value. tqdm.update() supports
negative values since 4.32.
@sir-sigurd sir-sigurd merged commit 218ec3c into master Oct 14, 2020
@sir-sigurd sir-sigurd deleted the fix-negative-tqdm-update branch October 14, 2020 08:12
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.

3 participants