Skip to content

import: clarify -o behavior for subdirectories #1668

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 1 commit into from
Aug 6, 2020

Conversation

pmrowla
Copy link
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
Member

Thanks @pmrowla , really appreciate this!

Just curious - is it the same for get?

@shcheklein shcheklein merged commit 951b529 into iterative:master Aug 6, 2020
@pmrowla
Copy link
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
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
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
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
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
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
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 iterative/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
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