Skip to content

import: clarify -o behavior for subdirectories#1668

Merged
shcheklein merged 1 commit into
treeverse:masterfrom
pmrowla:import-output-subdir
Aug 6, 2020
Merged

import: clarify -o behavior for subdirectories#1668
shcheklein merged 1 commit into
treeverse:masterfrom
pmrowla:import-output-subdir

Conversation

@pmrowla
Copy link
Copy Markdown
Contributor

@pmrowla pmrowla commented Aug 6, 2020

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@shcheklein
Copy link
Copy Markdown
Contributor

Thanks @pmrowla , really appreciate this!

Just curious - is it the same for get?

@shcheklein shcheklein merged commit 951b529 into treeverse:master Aug 6, 2020
@pmrowla
Copy link
Copy Markdown
Contributor Author

pmrowla commented Aug 6, 2020

@shcheklein For get we do a fetch + checkout and create the output subdirectory if it doesn't already exist, since that's the normal behavior for checkout.

@pmrowla pmrowla deleted the import-output-subdir branch August 6, 2020 16:21
Comment on lines +81 to +82
data will be placed inside. If `path` contains a parent directory which does
not exist, this command will fail.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for late review @pmrowla, I have a Q:

If path contains a parent directory

What does that mean? "Containing a parent" sounds contradictory. Parents contain you 😋

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Plus how can you contain something that does not exist? Then you don't contain it... Sorry, just confused by this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you run dvc import -o foo/bar and foo/ doesn't already exist, import will fail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, got it. I may reword a little. Thanks Peter!

@jorgeorpinel
Copy link
Copy Markdown
Contributor

For get we ... create the output subdirectory if it doesn't already exist

I'll add this note too.

BTW I get the technicality involving internal processes (checkout) but it's kind of weird that get creates them but import doesn't... And why doesn't import use checkout? Hmmmm 🤔

@pmrowla
Copy link
Copy Markdown
Contributor Author

pmrowla commented Aug 7, 2020

It's because in import we are defining an actual stage/dependency, but for get it's treated more like a standalone file

from treeverse/dvc#4340 (comment)

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.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Makes sense, that's relevant context for sure. My idea would be that for consistency maybe no DVC commands should makedirs... In fact probably only get does that. But anyway, I won't open a ticket about this, up to you to bring it up with the core team if you wish Peter 🙂

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