Skip to content

Avoid passing mpu_kwargs to create_multipart_upload #252

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.

Sign up for GitHub

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

Merged
SpacemanPaul merged 3 commits into from
Dec 9, 2025
+7 −1
Merged

Avoid passing mpu_kwargs to create_multipart_upload #252

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e61d6fe
Avoid passing mpu_kwargs to create_multipart_upload
laurence-kobold Oct 22, 2025
29058a1
Only pass arguments to create_multipart_upload if accepted by type si…
laurence-kobold Oct 24, 2025
b960b0d
Clarify s3 parameter variable names
laurence-kobold Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion odc/geo/cog/_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from __future__ import annotations

import inspect
from threading import Lock
from typing import TYPE_CHECKING, Any, Optional

Expand Down Expand Up @@ -114,7 +115,12 @@ def initiate(self, **kw) -> str:
"""Initiate the S3 multipart upload."""
Copy link
Contributor

@SpacemanPaul SpacemanPaul Oct 24, 2025

Choose a reason for hiding this comment

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

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 **kw in the signature, and just not pass it on to s3.create_multipart_upload() below, OR
  • revert the changes in initiate all together and just not pass the kwargs to initiate below, in _ensure_init()?

Copy link
Member

@Kirill888 Kirill888 Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@laurence-kobold laurence-kobold Oct 24, 2025

Choose a reason for hiding this comment

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

👍 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

Copy link
Member

@Kirill888 Kirill888 Oct 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@laurence-kobold laurence-kobold Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm calling save_cog_with_dask(data, str(tiff_path), compression="deflate").compute()

assert self.uploadId == ""
s3 = self.s3_client()
rr = s3.create_multipart_upload(Bucket=self.bucket, Key=self.key, **kw)

# Filter kwargs to only include valid parameters for create_multipart_upload
s3_params = set(inspect.signature(s3.create_multipart_upload).parameters.keys())
s3_kw = {k: v for k, v in kw.items() if k in s3_params}

rr = s3.create_multipart_upload(Bucket=self.bucket, Key=self.key, **s3_kw)
self.uploadId = rr["UploadId"]
return self.uploadId

Expand Down