Skip to content

Refactor S3, replace high-level resource/session API with low-level client API#583

Merged
mpenkov merged 20 commits intodevelopfrom
client
Mar 1, 2021
Merged

Refactor S3, replace high-level resource/session API with low-level client API#583
mpenkov merged 20 commits intodevelopfrom
client

Conversation

@mpenkov
Copy link
Copy Markdown
Collaborator

@mpenkov mpenkov commented Feb 10, 2021

While functionally they are the same, the session/resource stuff is not safe to use across multiple threads and subprocesses, and the client stuff is. The former is a bit easier to use directly than the latter, but this does not impact the user, because smart_open is doing all the work.

I also refactored the way we pass keyword parameters to boto3. Previously, we had a separate parameter for each boto3 function. This was a pain, because this made parameter lists longer for each new function, e.g.

  • resource_kwargs
  • multipart_upload_kwargs
  • singlepart_upload_kwargs
  • object_kwargs

This PR moves all of the above parameters into a single nested dict and introduces a wrapper that transparently injects the parameters into the required function call.

This does break backwards compatibility, so will need a major version bump when releasing.

Copy link
Copy Markdown
Contributor

@menshikh-iv menshikh-iv left a comment

Choose a reason for hiding this comment

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

Awesome work @mpenkov 🔥

Just one note

Comment thread smart_open/s3.py
@mpenkov
Copy link
Copy Markdown
Collaborator Author

mpenkov commented Feb 17, 2021

@piskvorky Can you please have a look at this? I'd like to merge and release so I can use this in one of my projects.

@mpenkov
Copy link
Copy Markdown
Collaborator Author

mpenkov commented Feb 22, 2021

@piskvorky Ping

Copy link
Copy Markdown
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

I did a cursory review, the code seems good. But what about compatibility? What does this do to existing users?

Comment thread smart_open/s3.py Outdated
Co-authored-by: Radim Řehůřek <radimrehurek@seznam.cz>
@mpenkov
Copy link
Copy Markdown
Collaborator Author

mpenkov commented Feb 22, 2021

This is a breaking change. People doing stuff with kwargs will have to switch from using resource paramaters to using client ones. This will mean a major version bump.

@piskvorky
Copy link
Copy Markdown
Owner

OK. And is this documented somewhere? A migration guide?

@mpenkov
Copy link
Copy Markdown
Collaborator Author

mpenkov commented Mar 1, 2021

Good point. I added a section to the migration guide. Please have a look.

Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst Outdated
Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst
Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst Outdated
Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst Outdated
Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst Outdated
Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst
mpenkov and others added 2 commits March 1, 2021 19:30
Co-authored-by: Radim Řehůřek <radimrehurek@seznam.cz>
@mpenkov
Copy link
Copy Markdown
Collaborator Author

mpenkov commented Mar 1, 2021

Comment thread MIGRATING_FROM_OLDER_VERSIONS.rst
@mpenkov mpenkov merged commit d8f1602 into develop Mar 1, 2021
@mpenkov mpenkov deleted the client branch March 1, 2021 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants