Skip to content

run: add --outs-persist and --outs-persist-no-cache options #1759

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
Mar 25, 2019

Conversation

pared
Copy link
Contributor

@pared pared commented Mar 21, 2019

Fixes #1214

@pared pared requested a review from efiop March 21, 2019 12:43
@pared pared changed the title run: add --outs-no-remove option output: handle "remove" flag Mar 21, 2019
@pared pared changed the title output: handle "remove" flag [WIP] output: handle "remove" flag Mar 21, 2019
@pared pared force-pushed the 1214 branch 3 times, most recently from e6c32eb to 321c661 Compare March 22, 2019 10:08
@pared pared changed the title [WIP] output: handle "remove" flag run: add --outs-persist and --outs-persist-no-cache options Mar 22, 2019
@pared pared requested a review from efiop March 22, 2019 10:10
@pared pared force-pushed the 1214 branch 2 times, most recently from 6b705dd to 5f9c12a Compare March 22, 2019 10:26
@pared pared changed the title run: add --outs-persist and --outs-persist-no-cache options [WIP] run: add --outs-persist and --outs-persist-no-cache options Mar 22, 2019
dvc/stage.py Outdated
@@ -260,7 +260,10 @@ def remove_outs(self, ignore_remove=False):
Used mainly for `dvc remove --outs`
"""
for out in self.outs:
out.remove(ignore_remove=ignore_remove)
if out.persist:
self.repo.unprotect(out.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lead to a bug in non-local out case. What we need to do is implement remote.unprotect and out.unprotect(that would call remote.unprotect). And also use remote.unprotect in repo.unprotect.

@@ -743,3 +744,6 @@ def _log_missing_caches(self, checksum_info_dict):
"nor on remote. Missing cache files: {}".format(missing_desc)
)
logger.warning(msg)

def unportect(self, target):
unprotect(target)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is backwards. Move repo.unprotect logic to RemoteLOCAL.unprotect and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move logic from repo.unprotect() to RemoteLOCAL.unprotect() and use RemoteLOCAL.unprotect in repo.unprotect().

@@ -743,3 +744,6 @@ def _log_missing_caches(self, checksum_info_dict):
"nor on remote. Missing cache files: {}".format(missing_desc)
)
logger.warning(msg)

def unportect(self, target):
unprotect(target)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move logic from repo.unprotect() to RemoteLOCAL.unprotect() and use RemoteLOCAL.unprotect in repo.unprotect().

@pared pared force-pushed the 1214 branch 2 times, most recently from b4f511d to 4a3d0e6 Compare March 22, 2019 20:15

def unprotect_outs(self):
for out in self.outs:
if out.scheme != "local" or not out.exists:
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we were simply ignoring an output if it didn't exist, but now unprotect() will raise an exception. This will break dvc repro when output doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality still exists, I moved it to output base class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, indeed! Sorry, didn't catch that 🙂

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.

👍

@efiop efiop merged commit e94bc88 into iterative:master Mar 25, 2019
@efiop efiop changed the title [WIP] run: add --outs-persist and --outs-persist-no-cache options run: add --outs-persist and --outs-persist-no-cache options Mar 25, 2019
@pared pared deleted the 1214 branch April 3, 2019 11:02
@jorgeorpinel
Copy link
Contributor

I just realize this was never documented 😋 iterative/dvc.org/issues/2027

@shcheklein
Copy link
Member

There was a PR, we didn't merge it to keep this option hidden for now.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 17, 2020

Wait I was wrong. This is documented in https://dvc.org/doc/command-reference/run#options, it's just missing from the dvc.yaml doc (explain well what "persist" means).

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.

run/repro: add option to not remove outputs before reproduction
4 participants