Avoid passing mpu_kwargs to create_multipart_upload #252
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #252 +/- ##
========================================
Coverage 94.21% 94.21%
========================================
Files 34 34
Lines 5875 5875
========================================
Hits 5535 5535
Misses 340 340 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SpacemanPaul
left a comment
SpacemanPaul
left a comment
There was a problem hiding this comment.
We need to get pylint and mypy passing.
|
|
||
| def initiate(self, **kw) -> str: | ||
| def initiate(self) -> str: | ||
| """Initiate the S3 multipart upload.""" |
There was a problem hiding this comment.
This breaks the type signature established by the base class. I'm not that familiar with this part of the code but could we leave either:
- leave
**kwin the signature, and just not pass it on tos3.create_multipart_upload()below, OR - revert the changes in
initiateall together and just not pass the kwargs to initiate below, in_ensure_init()?
There was a problem hiding this comment.
NO, we can't just disable configuration of s3.multi_part_upload (original commit using blame: ee72392 clearly shows it was done for a reason), we just need to fix changes linked in the initial description (fab6011) to make sure that only parameters meant for boto make it to boto and others are forwarded to the appropriate place.
There was a problem hiding this comment.
👍 sounds good. I found the existing code a little confusing because currently none of the **kw arguments are being used to configure s3.create_multipart_upload. I've reversed my original changes and I now filter kwargs to only pass in parameters if they are in the signature
There was a problem hiding this comment.
I feel like the error needs to be fixed higher in the call-stack. BTW @laurence-kobold how are you calling this, your original issue didn't include the call that errors out.
There was a problem hiding this comment.
I'm calling save_cog_with_dask(data, str(tiff_path), compression="deflate").compute()
Co-authored-by: Kirill Kouzoubov <[email protected]>
3 participants
Fixes #251
In fab6011, the S3 multipart-upload was changed. This meant that the writer was passed the result of
get_mpu_kwargs, rather than the remaining kwargs passed toupload. This, in turn, meant that the writer was passing the result ofget_mpu_kwargswhen callinginitiate, leading to them being passed to S3 API call.This PR prevents the result of
get_mpu_kwargsfrom being passed toinitiate, fixing the S3 API call