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

Allow passing additional kwargs for Azure writes #702

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

ddelange
Copy link
Contributor

Title

Allow passing additional kwargs for Azure writes

Motivation

Tests

Tests that smart_open.azure.Writer accepts metadata, and retrieving it afterwards (with FakeBlobClient.get_blob_properties).

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.

@ddelange
Copy link
Contributor Author

ddelange commented Jul 4, 2022

@mpenkov is there something I can do to move this along?

@MacHu-GWU
Copy link

@ddelange I am thinking similar things. Instead of using a optional arguments called blog_kwargs, I think smart_open should use a more general param name like rw_kwargs, and it pass down to all kinds of low level Python API. no matter it is a local FS, Azure, AWS, FTP etc ...

@ddelange
Copy link
Contributor Author

ddelange commented Jul 13, 2022

Although I wouldnt mind a unified API, I think that would be a bigger refactor (and major breaking change) to achieve that. Especially for s3, the mechanism is currently substantially different. See e.g. passing ACL, StorageClass, Metadata here in 3x def upload for azure,gcs,s3: https://github.com/stevearc/pypicloud/pull/304/files

For now, this PR is minimally invasive. Maybe it makes sense to open a separate issue linked to #701?

@ddelange
Copy link
Contributor Author

@mpenkov any thoughts?

@ddelange
Copy link
Contributor Author

@mpenkov could you please have a look?

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@mpenkov mpenkov merged commit 5efe951 into piskvorky:develop Jul 29, 2022
@mpenkov
Copy link
Collaborator

mpenkov commented Jul 29, 2022

OK, I've merged the changes.

Sorry for the delay, I've been busy with other parts of my life.

@ddelange
Copy link
Contributor Author

thanks and no problem!

@ddelange
Copy link
Contributor Author

ddelange commented Aug 2, 2022

@mpenkov could you push a release?

@mpenkov
Copy link
Collaborator

mpenkov commented Aug 4, 2022

@ddelange Looks like there's some sort of build problem: I need to fix this before making a release. Not sure when I'll get around to it, will try next week.

https://github.com/RaRe-Technologies/smart_open/runs/7574213524?check_suite_focus=true

(not sure, it looks like it may not be related to this PR)

@piskvorky
Copy link
Owner

piskvorky commented Aug 4, 2022

Screen Shot 2022-08-04 at 09 59 00

I've seen that warning before. chardet reached a new major version (5.x) and it caused an incompatibility with requests. What helped in my case was upgrading requests to the latest version. Are we pinning chardet or requests to a specific version?

Probably unrelated to the S3 error though (just a warning).

@ddelange
Copy link
Contributor Author

ddelange commented Aug 4, 2022

interesting to see a warning about chardet, as requests>=2.26 doesnt depend on chardet anymore ref.

also dont see chardet / requests being installed here 🤔 https://github.com/RaRe-Technologies/smart_open/runs/7574213524?check_suite_focus=true#step:5:204

@ddelange
Copy link
Contributor Author

ddelange commented Aug 19, 2022

can it be that the contents of this public bucket changed, and those keys are simply not there anymore?

@mpenkov
Copy link
Collaborator

mpenkov commented Aug 21, 2022

Yes, it appears so.

@mpenkov
Copy link
Collaborator

mpenkov commented Aug 21, 2022

ddelange added a commit to ddelange/pypicloud that referenced this pull request Aug 21, 2022
@ddelange
Copy link
Contributor Author

@mpenkov thanks for the quick fix and release! 💥

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.

Allow specifying metadata for azure uploads
4 participants