-
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
exp apply
: enable forcing (unsaved files in the workspace)
#9992
Conversation
@@ -33,10 +25,11 @@ def add_parser(experiments_subparsers, parent_parser): | |||
formatter_class=argparse.RawDescriptionHelpFormatter, | |||
) | |||
experiments_apply_parser.add_argument( |
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.
[Q] Is it ok to deprecate this now? It has not been working for a while.
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 thought we were forcing by default? cc @pmrowla
Codecov ReportAll modified lines are covered by tests β
π’ Thoughts on this report? Let us know!. |
assert cli_args.func == CmdExperimentsApply | ||
|
||
cmd = cli_args.func(cli_args) | ||
m = mocker.patch("dvc.repo.experiments.apply.apply", return_value={}) |
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.
[C] Really I want to call dvc.repo.checkout.checkout
here but the test dies with an error. Any pointers?
exp apply
: Enable forcing exp apply (unsaved files in the workspace)exp apply
: Enable forcing (unsaved files in the workspace)
exp apply
: Enable forcing (unsaved files in the workspace)exp apply
: enable forcing (unsaved files in the workspace)
Sorry, I didn't mean to enable auto-merge here before having @pmrowla review. |
@pmrowla Please do take a look at this. I can't recall if this is expected behavior, an oversight in the CLI, or if there is a bug. |
"\tgit stash apply refs/exps/apply/stash\n" | ||
) | ||
self.repo.experiments.apply(self.args.experiment) | ||
self.repo.experiments.apply(self.args.experiment, force=self.args.force) |
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.
Setting force
here does not do anything, the underlying experiments.apply
behavior was changed to always do the force apply. Since the DVC checkout
now needs force to allow removing untracked files, that flag should just always be set internally by experiments.apply
.
Basically, the intent is to not have a CLI level option for exp apply --force/--no-force
at all
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. π
Closes #9991
Docs change iterative/dvc.org#4895
Comments inline.