-
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
add option to not relink #10389
add option to not relink #10389
Conversation
parser.add_argument( | ||
"--no-relink", | ||
dest="relink", | ||
action="store_false", | ||
help="Don't recreate links from cache to workspace.", | ||
) |
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.
I'd avoid introducing this asymmetry here.
(What you have done is useful if you have support for both negative and positive behavors. Eg: you'd see this in git
: git stash --include-untracked
and git stash --no-include-untracked
).
parser.add_argument( | |
"--no-relink", | |
dest="relink", | |
action="store_false", | |
help="Don't recreate links from cache to workspace.", | |
) | |
parser.add_argument( | |
"--no-relink", | |
default=False, | |
action="store_true", | |
help="Don't recreate links from cache to workspace.", | |
) |
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.
Same in commit
.
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.
@skshetry Should we add both? We already have dvc checkout --relink
. We could have --relink
and --no-relink
options for all of those commands even if it's redundant when it's the default.
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.
We haven't done anything like that before in dvc. So maybe better to not introduce them. But up to you.
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.
I'm seeing plenty of examples of either store_false
and store_true
in the codebase for similar situations, and I think it is just as awkward to use store_true
(I will have to do relink=not self.args.no_relink
), so I'm going to leave this as is.
@@ -51,6 +51,7 @@ def run(self): | |||
to_remote=self.args.to_remote, | |||
remote_jobs=self.args.remote_jobs, | |||
force=self.args.force, | |||
relink=self.args.relink, |
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.
Based on https://github.com/iterative/dvc/pull/10389/files#r1565145737.
relink=self.args.relink, | |
relink=not self.args.no_relink, |
Adds
dvc add --no-relink
anddvc commit --no-relink
as a way to speed up add, but at the potential cost of lots of copies and maybe having to relink later.Related to: