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

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

Merged
merged 20 commits into from
Mar 1, 2021

Conversation

mpenkov
Copy link
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
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

smart_open/s3.py Show resolved Hide resolved
@mpenkov
Copy link
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
Collaborator Author

mpenkov commented Feb 22, 2021

@piskvorky Ping

Copy link
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?

smart_open/s3.py Outdated Show resolved Hide resolved
Co-authored-by: Radim Řehůřek <radimrehurek@seznam.cz>
@mpenkov
Copy link
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
Owner

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

@mpenkov
Copy link
Collaborator Author

mpenkov commented Mar 1, 2021

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

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

mpenkov commented Mar 1, 2021

@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