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

Catch PermissionError in fsspec.core when attempting to auto_mkdir #1406

Merged
merged 5 commits into from
Nov 6, 2023
Merged

Catch PermissionError in fsspec.core when attempting to auto_mkdir #1406

merged 5 commits into from
Nov 6, 2023

Conversation

DanielTsiang
Copy link
Contributor

Catch PermissionError in fsspec.core when attempting to auto_mkdir for parent dir, as the user may not have permission to create the parent dir.

This should hopefully allow the user to carry on without error if the parent dir already exists. If not, a later exception should be thrown when trying to write to a file when the parent dir doesn't exist.

This attempts to address the following issue:

Comments on how to improve this PR are welcome, as this is my first time contributing to the repo.

Catch `PermissionError` in fsspec.core when attempting to auto_mkdir for parent dir, as the user may not have permission to create the parent dir. 

This should hopefully allow the user to carry on without error if the parent dir already exists. If not, a later exception should be thrown when trying to write to a file when the parent dir doesn't exist.

This attempts to address the following issue:
* #1404

Comments on how to improve this PR are welcome, as this is my first time contributing to the repo.
Copy link
Member

@martindurant martindurant left a comment

Choose a reason for hiding this comment

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

Does PermissionError actually catch the S3 case?

We could probably test this with a mock on makedirs for some filesystem like memoryFS.

I feel like it would be nice if we kept the exception around in the OpenFile, so that if writing fails later, we can refer to it - else when the directory doens't exist and the user couldn't create it, they can know why their write fails.

fsspec/core.py Outdated Show resolved Hide resolved
@DanielTsiang
Copy link
Contributor Author

@martindurant I'm not sure why the tests have started failing because of pytest import error:

 Skipped: could not import 'moto': No module named 'moto'

Do you have any ideas?

@martindurant
Copy link
Member

This is an upstream issue in pytest-asyncio. Waiting on pytest-dev/pytest-asyncio#656

@DanielTsiang
Copy link
Contributor Author

DanielTsiang commented Nov 2, 2023

This is an upstream issue in pytest-asyncio. Waiting on pytest-dev/pytest-asyncio#656

I see, thanks. Since it's broken in 0.22.0, worth pinning to 0.21.1 in the meantime?

@martindurant
Copy link
Member

I'll give them a couple of days, see if it gets resolved.

@DanielTsiang
Copy link
Contributor Author

I'll give them a couple of days, see if it gets resolved.

Sounds reasonable.

In terms of this PR, once the pytest-asyncio issue is resolved, would you be comfortable merging this PR in?

@martindurant
Copy link
Member

Yes, this PR is good to go.

@seifertm
Copy link

seifertm commented Nov 3, 2023

I saw this PR linked in the pytest-asyncio repository.

pytest-asyncio v0.22 was so bad that it was yanked from PyPI. There are currently no plans to release a bugfix version. It would be best if you pinned the version pytest-asyncio!=0.22.0 and went ahead with this PR.

@martindurant
Copy link
Member

Thanks @seifertm . Will it also be removed from conda-forge?

@martindurant
Copy link
Member

(please merge from master - I don't have permissions on your repo to do so)

Merge master branch of filesystem_spec into PR branch

---------

Co-authored-by: Martin Durant <[email protected]>
Co-authored-by: Guido Diepen <[email protected]>
Co-authored-by: Martin Durant <[email protected]>
@DanielTsiang
Copy link
Contributor Author

(please merge from master - I don't have permissions on your repo to do so)

I've now merged latest from master branch into this PR branch.

@seifertm
Copy link

seifertm commented Nov 6, 2023

Thanks @seifertm . Will it also be removed from conda-forge?

@martindurant Unlikely. I couldn't find any information on how to do that. I'd appreciate if you had any pointers on this.

@martindurant
Copy link
Member

@seifertm , the README at https://github.com/conda-forge/admin-requests gives instructions. We are excluding the build from our test environment for now.

@martindurant
Copy link
Member

@DanielTsiang , thanks for cleaning up the history - not sure what happened there!

@martindurant martindurant merged commit 811d5d8 into fsspec:master Nov 6, 2023
10 checks passed
@seifertm
Copy link

seifertm commented Nov 8, 2023

@martindurant Thanks for the pointer. I filed a PR to mark pytest-asyncio-0.22.0 broken in conda-forge:
conda-forge/admin-requests#857

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