Skip to content

Introduce dvc commit#1601

Merged
efiop merged 7 commits into
treeverse:masterfrom
efiop:919
Feb 17, 2019
Merged

Introduce dvc commit#1601
efiop merged 7 commits into
treeverse:masterfrom
efiop:919

Conversation

@efiop
Copy link
Copy Markdown
Contributor

@efiop efiop commented Feb 10, 2019

Fixes #919

@efiop efiop changed the title Introduce dvc commit [WIP] Introduce dvc commit Feb 10, 2019
@efiop efiop force-pushed the 919 branch 2 times, most recently from a8c7ad1 to 8f6d4ef Compare February 10, 2019 05:33
@efiop efiop changed the title [WIP] Introduce dvc commit Introduce dvc commit Feb 10, 2019
@efiop efiop requested review from a user, pared and shcheklein February 10, 2019 05:53
@efiop efiop force-pushed the 919 branch 2 times, most recently from 19231d6 to 70453de Compare February 10, 2019 06:14
efiop pushed a commit to efiop/dvc.org that referenced this pull request Feb 10, 2019
Related to treeverse/dvc#1601

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop efiop changed the title Introduce dvc commit [WIP] Introduce dvc commit Feb 10, 2019
Comment thread dvc/command/add.py Outdated
Comment thread dvc/command/repro.py Outdated
Comment thread dvc/output/base.py Outdated
Comment thread dvc/project.py Outdated
Comment thread dvc/project.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can dvc commit change stage files?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. It will re-calculate checksums if they are missing or they don't match(this one only if explicitly confirmed with a prompt or --force flag).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It makes sense. How about this case then. In the next version we are going o have no-commit enabled by default, right? So, let's imagine I run something in master and then checkout another branch. It means that checksums in stage files are wrong now. But is does not mean that I want force them when I run dvc commit, all I want to do is to save files to cache. It might be that we'll need to abort the checkout itself, similar to what git is doing, right? Any thoughts on this? Basically, I'm thinking is it always the case that we want to force the change in stage files or do we just want to push data (e.g. generated in a different branch, or previous git commit, or whatnot) to cache.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a great point! We need to have that check on dvc checkout. Looking into it...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shcheklein Just checked and we do check that on dvc checkout anyway. E.g. not committed stage.dvc means that there is no cache for it and so dvc checkout will prompt user asking file 'foo' is going to be removed. Are you sure you want to proceed? So we are covered there. Still, the message is not very informative, so I'll add a clarification for it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the record:created #1622 for it for now.

Copy link
Copy Markdown
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Good stuff. More questions than fixes or improvements.

Copy link
Copy Markdown
Contributor

@dmpetrov dmpetrov left a comment

Choose a reason for hiding this comment

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

Great change! dvc will have commit finaly 🎉

Just a couple questions inline...

Comment thread dvc/command/add.py Outdated
Comment thread dvc/command/commit.py Outdated
Comment thread dvc/command/add.py Outdated
Comment thread dvc/command/commit.py Outdated
Copy link
Copy Markdown
Contributor

@dmpetrov dmpetrov left a comment

Choose a reason for hiding this comment

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

Great change! 🥇

Please take a look at this comment: #1403 (comment)

Ruslan Kuprieiev added 4 commits February 16, 2019 21:18
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Ruslan Kuprieiev added 3 commits February 16, 2019 21:18
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Copy link
Copy Markdown
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Good stuff!

@efiop efiop changed the title [WIP] Introduce dvc commit Introduce dvc commit Feb 17, 2019
@efiop efiop merged commit 25ae052 into treeverse:master Feb 17, 2019
efiop pushed a commit to efiop/dvc.org that referenced this pull request Feb 17, 2019
Related to treeverse/dvc#1601

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@colllin
Copy link
Copy Markdown

colllin commented Feb 21, 2019

This is a great addition!! I found dvc commit in your docs just now, looking for a way to save the outputs of a failed dvc repro to the cache — it had produced a good model, but the script later stalled. Another use case would be if I killed a training early because I was satisfied with the results and/or in a hurry. Anyway, I think this totally solved it. dvc commit; dvc push 🎉

@efiop efiop deleted the 919 branch February 21, 2019 15:41
@efiop
Copy link
Copy Markdown
Contributor Author

efiop commented Feb 21, 2019

@colllin glad you found it useful! Thank you for the feedback! 🙂

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.

4 participants