-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: directory creation on pulling data #10580
Conversation
dvc/fs/dvc.py
Outdated
@@ -571,7 +571,7 @@ def _get( # noqa: C901, PLR0912 | |||
|
|||
os.makedirs(lpath, exist_ok=True) | |||
for d in _dirs: | |||
os.mkdir(d) | |||
os.mkdir(d, exist_ok=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no exist_ok=True
in os.mkdir
. That is in os.makedirs
.
This should either be:
os.mkdir(d, exist_ok=True) | |
try: | |
os.mkdir(d) | |
except OSError: | |
if not os.path.isdir(d): | |
raise |
or,
os.mkdir(d, exist_ok=True) | |
os.makedirs(d, exist_ok=True) |
I'd prefer the first one as makedirs
calls os.path.exists()
on all the parents of the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you did not go with the first option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less verbose, atomicity and clarity.
Hi, thank you for contributing. The tests and the linter is failing, please take a look at https://dvc.org/doc/contributing/core on how to run tests and fix linter issues. Please feel free to ask if you need any help. :) |
065f322
to
85a0f83
Compare
Thanks @skshetry for your feedback. Code's been updated. However somehow CI fails when running benchmars Proposed a fix in 1e268fa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM although I am a bit concerned with performance issues with makedirs
(stat
calls can be expensive). But let's go with this one for now, we'll revisit if I find it to be the culprit.
Also, thank you for fixing the benchmarks workflow.
Co-authored-by: Santiago Silva <[email protected]>
Closes #10579