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.

Sign up for GitHub

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

Jump to bottom

Fix is_local for paths starting with az:// #849

Closed
wants to merge 2 commits into from
Closed

Fix is_local for paths starting with az:// #849

wants to merge 2 commits into from

Conversation

mpfirrmann
Copy link
Contributor

Signed-off-by: Marco Pfirrmann [email protected]

Fixes #840.
The module used to extract the protocol does not differentiate between abfs:// and az:// paths, always returning abfs. This fix adds az to the protocol list to be checked.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 21, 2022
ejguan
ejguan reviewed Oct 21, 2022
View reviewed changes
Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

Thank you. Changes look good. Have you verified it working properly with the script in the issue. Could you please add a test here?

@ejguan ejguan requested a review from NivekT October 21, 2022 15:55
NivekT
NivekT reviewed Oct 21, 2022
View reviewed changes
Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR. I agree testing this behavior will be helpful.

NivekT
NivekT approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@NivekT NivekT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ignore

NivekT
NivekT suggested changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@NivekT NivekT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The test LGTM but we need to either install adlfs or skip it when adlfs is not available. The former is probably best.

@mpfirrmann
Copy link
Contributor Author

The test LGTM but we need to either install adlfs or skip it when adlfs is not available. The former is probably best.

I added the adlfs package to test/requirements.txt and it should be installed by the testing workflows.

@facebook-github-bot
Copy link
Contributor

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

NivekT
NivekT approved these changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

LGTM!

@ejguan ejguan mentioned this pull request Nov 17, 2022
Closed
facebook-github-bot pushed a commit that referenced this pull request Nov 17, 2022
@ejguan @facebook-github-bot
Fix azure test (#902)
e79ff42 
Summary:
Fix bug introduced by #849

- Replace `skipIfNoFSSpecS3` by `skipIfNoFSSpecAZ` for tests to access Azure Blob
- Add `adlfs` to conda test requirements.

Pull Request resolved: #902

Reviewed By: NivekT

Differential Revision: D41378561

Pulled By: ejguan

fbshipit-source-id: be99d1371379e1c87c2f2ac062b009f88575aa0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejguan ejguan ejguan left review comments

@NivekT NivekT NivekT approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

File paths get duplicated by list_files_by_fsspec pipeline if folder path starts with az://
4 participants
@mpfirrmann @facebook-github-bot @NivekT @ejguan