Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 12, 2019

Fix #1852

@ghost ghost requested a review from efiop April 12, 2019 02:35
efiop
efiop previously requested changes Apr 12, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we forcing everything though?

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 not testing running dvc run twice, which is where persist should make second run ignore build cache and actually run the command.

@efiop
Copy link
Contributor

efiop commented Apr 15, 2019

For the record: Discussed this privately. Need to undo the repro part (notice discussion at #1884) and add a test for run part.

@ghost ghost requested a review from efiop April 24, 2019 23:06
@ghost ghost dismissed efiop’s stale review April 24, 2019 23:08

removed the code that was messing up with reproduce and left only run

Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be a warning during the first run.

Copy link
Author

Choose a reason for hiding this comment

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

@efiop , should we skip the overwrite stage file confirmation when outs-persist is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MrOutis why would we want that though? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

should we test that that warning is not printed during the first run now? 🙂

Copy link
Author

Choose a reason for hiding this comment

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

sure 👍

Copy link
Author

Choose a reason for hiding this comment

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

done, @efiop (sorry for such back-and-forth, I should've test it correctly beforehand 😅)

@ghost ghost requested a review from efiop April 24, 2019 23:46
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 44e20f1 into treeverse:master Apr 24, 2019
@ghost ghost deleted the fix-1852 branch April 24, 2019 23:59
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: make sure persist is ignoring build cache

1 participant