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

Fix missing kwargs injection on upload blob call #392

Merged

Conversation

nffdiogosilva
Copy link
Contributor

@nffdiogosilva nffdiogosilva commented Jan 26, 2023

Hello there.

Currently, when uploading a blob, there's no way of injecting other arguments besides the data, overwrite, and metadata.

An example would be when I want to compress data and set the content encoding of the data to br. For that I need to pass a ContentSettings object into the upload_blob method kwargs:

i.e:

from azure.storage.blob import ContentSettings
import brotli
import fsspec

CONN_STRING = "<CONN_STRING>"
CONTAINER = "kwargs-injection"

abfs = fsspec.filesystem(
    "abfs",
    connection_string=CONN_STRING,
)

if __name__ == "__main__":
    content_settings = ContentSettings(
        content_type="application/json",
        content_encoding="br",
    )
    abfs.mkdir(CONTAINER, exist_ok=True)
    abfs.write_bytes(
        path=f"{CONTAINER}/compressed.br",
        value=brotli.compress(b'{"a": "hello world"}', quality=1),
        overwrite=True,
        **{"content_settings": content_settings},
    )

What I'm expecting to see in the azure storage containers web interface, when consulting the blob properties:

Screenshot 2023-01-26 at 11 22 36

What I see instead (no content encoding defined):
Screenshot 2023-01-26 at 11 35 05

script requirements:
$ pip install fsspec adlfs brotli

@TomAugspurger
Copy link
Contributor

xref fsspec/filesystem_spec#916. But agreed that forwarding arguments makes sense, even if that were to be standardized.

We should document this somewhere though. Maybe a short note in index.md?

@nffdiogosilva
Copy link
Contributor Author

nffdiogosilva commented Jan 30, 2023

@TomAugspurger that makes sense to standardised but in the meantime can we merge this PR so it can unblock my current use case?

Regarding the note, not sure what you are suggesting. I've added this below the usage section in the index.md:

Note: When uploading a blob (with write_bytes or write_text) you can injects kwargs directly into upload_blob method:

>>> from azure.storage.blob import ContentSettings
>>> fs = adlfs.AzureBlobFileSystem(account_name="ai4edataeuwest")
>>> fs.write_bytes(path="path", value=data, overwrite=True, **{"content_settings": ContentSettings(content_type="application/json", content_encoding="br")})

Was it something like this that you wanted?

@nffdiogosilva nffdiogosilva force-pushed the hotfix/inject-kwargs-on-upload-blob branch from 0c7402a to 6135279 Compare January 30, 2023 11:17
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

that makes sense to standardised but in the meantime can we merge this PR so it can unblock my current use case?

Yep. I think we'll want this PR regardless of what happens upstream, to support other keyword arguments.

It seems that there's a linting check failing. I can't actually view the Azure Pipelines output (cc @hayesgb). Maybe you can try running the linters locally to see what's failing? This looks good otherwise.

@nffdiogosilva nffdiogosilva force-pushed the hotfix/inject-kwargs-on-upload-blob branch from c5e5e22 to b50be78 Compare January 31, 2023 10:24
@nffdiogosilva nffdiogosilva force-pushed the hotfix/inject-kwargs-on-upload-blob branch from b50be78 to e0c1c0e Compare February 9, 2023 11:19
@nffdiogosilva nffdiogosilva force-pushed the hotfix/inject-kwargs-on-upload-blob branch from e0c1c0e to aa7054e Compare February 9, 2023 11:26
@@ -1,6 +1,6 @@
repos:
- repo: https://github.com/pycqa/isort
rev: 5.10.1
rev: 5.12.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nffdiogosilva
Copy link
Contributor Author

nffdiogosilva commented Feb 9, 2023

Hello @TomAugspurger. It seems that the tests were failing due to the isort version that was being used in the pre commit hooks. (you can understand the error better here: https://levelup.gitconnected.com/fix-runtimeerror-poetry-isort-5db7c67b60ff). Fix it now, and the checks are all passed.

Can we merge it now, please?

Thank you.

@TomAugspurger TomAugspurger merged commit 2d56e41 into fsspec:main Feb 9, 2023
@TomAugspurger
Copy link
Contributor

Thanks!

@nffdiogosilva
Copy link
Contributor Author

nffdiogosilva commented Feb 13, 2023

@TomAugspurger, wasn't a new version built with this fix? If not, can you build one please? Anything I didn't do that was suppose to do? Let me know if I can help. Thank you.

@TomAugspurger
Copy link
Contributor

@hayesgb has handled the last few releases. @hayesgb are you able to make a new release sometime, or would you need me to?

@eirki
Copy link

eirki commented Apr 26, 2023

Just a friendly notification: I would be super grateful if someone could build a new version with this fix.

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.

3 participants