Skip to content

import: fix importing into subdirectory bug #4340

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

Merged
merged 5 commits into from
Aug 6, 2020

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Aug 5, 2020

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Fixes #4169.

Makes dvc import ... -o subdir/file behave the same whether or not subdir already exists (new output will be subdir/file.dvc)

docs: iterative/dvc.org#1668

@pmrowla pmrowla self-assigned this Aug 5, 2020
@pmrowla pmrowla requested review from efiop, pared and skshetry August 5, 2020 07:16
Comment on lines +375 to +381
scheme = urlparse(out).scheme
if os.name == "nt" and scheme == abspath.drive[0].lower():
# urlparse interprets windows drive letters as URL scheme
scheme = ""
if (
os.path.exists(dirname) # out might not exist yet, so
and PathInfo(abspath).isin_or_eq(repo.root_dir)
not scheme
and abspath.isin_or_eq(repo.root_dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

out may be a local path or a remote://... url, but os.path.exists isn't a good enough check for whether or not out is a valid local path.

In the case where we are importing with -o subdir/file, if subdir doesn't already exist it is still a valid local output path (and we need to makedirs(subdir) later)

Comment on lines 30 to 32
if not os.path.exists(wdir):
makedirs(wdir)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to error-out if it doesn't exist? Kinda like cp and other tools do.

Copy link
Contributor Author

@pmrowla pmrowla Aug 5, 2020

Choose a reason for hiding this comment

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

Creating the directory if it doesn't exist feels like the more natural import -o behavior for me, but given that the current docs for -o say:

If an existing directory is specified, the target data will be placed inside.

it would also be okay for us to error out here.

Copy link
Member

Choose a reason for hiding this comment

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

please, don't forget to update the docs if we change this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@pmrowla We also don't makedirs in dvc run, but rather raise an error that wdir doesn't exist, so it seems it would be more consistent if we raise and error here too. Same with dvc add that shares the same logic with dvc run. I wonder why that

wdir_or_path="stage working dir" if is_wdir else "file path",
check wasn't triggered here as well πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue before was that we set wdir to os.cwd() if the output subdirectory didn't already exist, because we assumed that subdir not existing actually meant it was a remote URL and not a local path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Behavior has been updated here so that we use the correct subdir as wdir, and then error out in utils.check_stage_path if it doesn't already exist

Copy link
Contributor

@efiop efiop 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! Could double check the docs as well, please? πŸ™

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #4340 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4340   +/-   ##
=======================================
  Coverage   91.19%   91.19%           
=======================================
  Files         177      177           
  Lines       12209    12209           
=======================================
  Hits        11134    11134           
  Misses       1075     1075           

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 35dd1fd...e432438. Read the comment docs.

@pmrowla pmrowla merged commit d128d51 into iterative:master Aug 6, 2020
@pmrowla pmrowla deleted the 4169-import-from-dir branch August 6, 2020 14:26
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.

import: same command creates .dvc file in different locations
4 participants