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

mkdir has a default of exist_ok=False, but spec expects True? #134

Closed
jorisvandenbossche opened this issue Nov 19, 2020 · 5 comments
Closed

Comments

@jorisvandenbossche
Copy link

The mkdir implementation here has a default of exist_ok=False:

https://github.com/dask/adlfs/blob/54d5826627a25dc8541e35754bf4e9512543a027/adlfs/spec.py#L896-L901

However, when looking at eg the local FileSystem implementation of the core fsspec library, there it clearly is OK with creating a directory that already exists:

https://github.com/intake/filesystem_spec/blob/e6596825b5df0d43a7590ff82f50f2b1fead3ab3/fsspec/implementations/local.py#L31-L36

This seems to be a difference between mkdir and mkdirs (where this last one indeed has a default of exist_ok=False). Should this default be changed for mkdir?

@hayesgb
Copy link
Collaborator

hayesgb commented Nov 19, 2020

Thanks @jorisvandenbossche. Question for @martindurant

Per fsspec, exist_ok=True should raise an exception if the target exists. This means that the LocalFileSystem makedirs, where create_parents=True would be expected to raise an exception. However, os.makedirs() uses the opposite convention for the boolean exist_ok. Should the docs be updated for fsspec?

Looks like s3fs uses the convention followed by os.

@martindurant
Copy link
Member

Yeah, it looks like the docstring has it the wrong way around

@hayesgb
Copy link
Collaborator

hayesgb commented Nov 21, 2020

@jorisvandenbossche and @martindurant.

Based on the cross-reference with this issue, what do we want to align on for the expected behavior of makedirs and mkdir? @jorisvandenbossche -- if we align to the Python convention for os, then I would reimplement mkdir as makedirs, with the behavior described there, and propose a change to the fsspec docstring.

From the Python docs, mkdir does not include the "create_parents" keywords, so I would propose dropping that from fsspec, and aligning to the Python os convention.

Thoughts?

@jorisvandenbossche
Copy link
Author

Apart from os.mkdir/makedirs, there is also patlib.Path.mkdir as a reference (https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir), which has a parents keyword.

mkdir does not include the "create_parents" keywords, so I would propose dropping that from fsspec, and aligning to the Python os convention.

Personally, I would not remove the keyword since it is quite useful (and not all implementations have a makedirs method), but another option could also be to change the default, so that the default behaviour will match os.mkdir / the default of Path.mkdir, but still allow to get the behaviour of makedirs if desired.

This was referenced Jan 14, 2021
@hayesgb
Copy link
Collaborator

hayesgb commented Jan 15, 2021

This is addressed in #165 . Note that create_parents will create a container if it is missing.

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

No branches or pull requests

3 participants