Skip to content

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

Merged
efiop merged 1 commit into
treeverse:masterfrom
pared:1214
Mar 25, 2019
Merged

run: add --outs-persist and --outs-persist-no-cache options#1759
efiop merged 1 commit into
treeverse:masterfrom
pared:1214

Conversation

@pared
Copy link
Copy Markdown
Contributor

@pared pared commented Mar 21, 2019 •
edited
Loading

Fixes #1214

@pared pared requested a review from efiop March 21, 2019 12:43
efiop
efiop reviewed Mar 21, 2019
View reviewed changes
Comment thread tests/test_data_cloud.py Outdated
Comment thread tests/test_run.py Outdated
@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
efiop
efiop reviewed Mar 21, 2019
View reviewed changes
Comment thread dvc/stage.py Outdated
Comment thread dvc/repo/__init__.py Outdated
@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
efiop
efiop suggested changes Mar 22, 2019
View reviewed changes
Comment thread dvc/stage.py Outdated
Copy link
Copy Markdown
Contributor

@efiop efiop Mar 22, 2019

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.

@pared pared requested a review from efiop March 22, 2019 15:09
efiop
efiop suggested changes Mar 22, 2019
View reviewed changes
Comment thread dvc/output/base.py Outdated
efiop
efiop suggested changes Mar 22, 2019
View reviewed changes
Comment thread dvc/remote/local.py Outdated
Copy link
Copy Markdown
Contributor

@efiop efiop Mar 22, 2019

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
Copy Markdown
Contributor Author

@pared pared Mar 22, 2019

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor

@efiop efiop Mar 22, 2019

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().

efiop
efiop suggested changes Mar 22, 2019
View reviewed changes
Comment thread dvc/remote/local.py Outdated
Copy link
Copy Markdown
Contributor

@efiop efiop Mar 22, 2019

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().

efiop
efiop suggested changes Mar 22, 2019
View reviewed changes
Comment thread dvc/output/base.py Outdated
@pared pared force-pushed the 1214 branch 2 times, most recently from b4f511d to 4a3d0e6 Compare March 22, 2019 20:15
@pared pared requested a review from efiop March 25, 2019 08:46
efiop
efiop suggested changes Mar 25, 2019
View reviewed changes
Comment thread dvc/stage.py

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

@efiop efiop Mar 25, 2019

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
Copy Markdown
Contributor Author

@pared pared Mar 25, 2019

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
Copy Markdown
Contributor

@efiop efiop Mar 25, 2019

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 🙂

efiop
efiop approved these changes Mar 25, 2019
View reviewed changes
Copy link
Copy Markdown
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 treeverse: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
This was referenced Mar 25, 2019
Closed
Closed
@pared pared deleted the 1214 branch April 3, 2019 11:02
@jorgeorpinel jorgeorpinel mentioned this pull request Dec 17, 2020
Closed
@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Dec 17, 2020

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

@shcheklein
Copy link
Copy Markdown
Contributor

shcheklein commented Dec 17, 2020

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

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Dec 17, 2020 •
edited
Loading

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

Reviewers

1 more reviewer

@efiop efiop efiop approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@pared @jorgeorpinel @shcheklein @efiop