add: do not delete stage files before add #6239
Conversation
Because of the way we collect stages and cache them, we were not able to collect them for the `add` without removing them from the workspace. As doing so, we'd have two same/similar stages - one collected from the workspace and the other just created from the `dvc add` in-memory. This would raise errors during graph checks, so we started to delete them and reset them (which is very recently, see treeverse#2886 and treeverse#3349). By deleting the file before we even do any checks, we are making DVC fragile, and results in data loss for the users with even simple mistakes. This should make it more reliable and robust. And, recently, we have started to keep state of a lot of things, that by resetting them on each stage, we waste a lot of performance, especially on gitignores. We cache the dulwich's IgnoreManager, which when resetted too many times, will waste a lot of our time just collecting them again next time (see treeverse#6227). It's hard to say how much this improves, as this very much depends on no. of gitignores in the repo (which can be assumed to be quite in number for a dvc repo) and the amount of files that we are adding (eg: `-R` adding a large directory). On a directory with 10,000 files (in a datadet-registry repo), creating stages on `dvc add -R` went from 64 files/sec to 1.1k files/sec.
skshetry
added
enhancement
|
Can we add some tests for this (failed op doesn't remove file for example?). Or prevent regression from happening in some other way in the future. |
| # remove existing stages that are to-be replaced with these | ||
| # new stages for the graph checks. | ||
| old_stages = set(repo.stages) - set(stages) | ||
| try: | ||
| repo.check_modified_graph(stages) | ||
| repo.check_modified_graph(stages, list(old_stages)) |
There was a problem hiding this comment.
This would be better reflected by Index here, as we are creating a new index. Index is a collection of stages, so if some stages underneath are changed, is it the same index? So, immutability is at the heart of the index and would have been better reflected here.
new_index = repo.index.update(stages)
try:
new_index.check_graph()
Thanks, @shcheklein , I have added some tests. 🙂 |
efiop
left a comment
efiop
left a comment
There was a problem hiding this comment.
We are doing stuff like this because we wanted to not leave a dvc-file in case of failure to not lead the users to believe that the command was actually successful, but that was a wrong approach. What we should've really done is worked on better indication of success/failure and avoid losing the information.
3 participants
Because of the way we collect stages and cache them, we were not able to
collect them for the
addwithout removing them from the workspace.As doing so, we'd have two same/similar stages - one collected from the
workspace and the other just created from the
dvc addin-memory.This would raise errors during graph checks, so we started to delete them
and reset them (which is very recently, see #2886 and #3349).
By deleting the file before we even do any checks, we are making DVC
fragile, and results in data loss for the users with even simple mistakes.
This should make it more reliable and robust.
And, recently, we have started to keep states of a lot of things, that by
resetting them on each stage, we waste a lot of performance, especially
on gitignores. We cache the Dulwich's IgnoreManager, which when reset
too many times will waste a lot of our time just collecting them again
next time (see #6227).
It's hard to say how much this improves, as this very much depends on
no. of gitignores in the repo (which can be assumed to be quite a number
for a DVC repo) and the number of files that we are adding
(eg:
-Radding a large directory).On a directory with 10,000 files (in a dataset-registry repo),
creating stages on
dvc add -Rwent from 64 files/sec to 1.1k files/sec.Also relevant discussion: #3362
❗ 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. 🙏