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

Parse the query parameter of url without using infer_storage_options #912

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

wang2bo2
Copy link
Contributor

@wang2bo2 wang2bo2 commented Nov 7, 2024

Parsing S3 access point URI, like s3://arn:aws:s3:us-west-2:1234:accesspoint/abc/123.jpg, causes problems in fsspec.utils.infer_storage_options because the :accesspoint was parsed as port number.

This patch removes the need to call fsspec.utils.infer_storage_options in s3fs, as discussed in fsspec/filesystem_spec#1743 (comment)

@martindurant
Copy link
Member

For an example URL like this, does the bucket end up being "arn:aws:s3:us-west-2:1234:accesspoint"?

We should have a test ensuring that this does the right thing.

@wang2bo2
Copy link
Contributor Author

wang2bo2 commented Nov 8, 2024

There’s already a test on checking the accesspoint bucket parsing

"arn:aws:s3:region:123456789012:accesspoint/my-access-point-name",

@martindurant
Copy link
Member

There’s already a test

but the codepath you fixed is for url_to_fs, not split_path, right? That test was already passing without this change.

@wang2bo2
Copy link
Contributor Author

wang2bo2 commented Nov 9, 2024

The newly added test would check S3FileSystem's compatibility with fsspec.url_to_fs and this test would fail without the fix in this PR.

@martindurant martindurant merged commit 78249af into fsspec:main Nov 11, 2024
25 checks passed
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.

2 participants