Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add: performance and reliability issues #6227

Closed
5 tasks done
skshetry opened this issue Jun 25, 2021 · 4 comments
Closed
5 tasks done

add: performance and reliability issues #6227

skshetry opened this issue Jun 25, 2021 · 4 comments
Labels
A: data-management Related to dvc add/checkout/commit/move/remove enhancement Enhances DVC performance improvement over resource / time consuming tasks ui user interface / interaction

Comments

@skshetry
Copy link
Member

skshetry commented Jun 25, 2021 •
edited
Loading

  • Repeated dvc add is not skipped.

    $ dvc add data
$ dvc add data

    In 1.X, it'd have been skipped. And, dvc still deletes the file and tries to restore it from the cache making it slower.

  • DVC uses move-then-checkout logic. It moves the file from the workspace to the cache and then checks it out again, rather than just using copy.

    This is slow and might result in data loss if it happens to fail in between the operations.

  • DVC deletes the stage file, before even adding those files. This means that if the dvc add operation fails, the existing pointer file is lost, which is the only way to get access to the data.

  • DVC resets the stages multiple times (only if multiple targets are provided) and forces the stage recollection which is slow.

  • To the same effect, it resets the internal state of the repo after creating each stage, which also happens to reset dulwich's ignore manager, making it horribly slow if using too many targets (or, -R).

dvc/dvc/repo/add.py

Line 266 in 4e792ae

repo._reset() # pylint: disable=protected-access

skshetry added a commit to skshetry/dvc that referenced this issue Jun 28, 2021
@skshetry
add: do not delete stage files before add
720d724 
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 iterative#2886 and iterative#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 iterative#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.
This was referenced Jun 28, 2021
Merged
Closed
efiop pushed a commit that referenced this issue Jul 3, 2021
@skshetry
add: do not delete stage files before add (#6239)
8d5f9ab 
* add: do not delete stage files before add

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 #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 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 #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.

* add tests

* make the test more specific
@pared pared added performance improvement over resource / time consuming tasks ui user interface / interaction enhancement Enhances DVC labels Sep 9, 2021
@pared
Copy link
Contributor

pared commented Sep 9, 2021

DVC uses move-then-checkout logic. It moves the file from the workspace to the cache and then checks it out again, rather than just using copy.

Wasn't this intended to enforce cache link type? I guess in case of copy it would make sense but what about others?

@skshetry
Copy link
Member Author

skshetry commented Sep 9, 2021

For other links, the one I suggested was to change copy behaviour to be move + link that works atomically.
@efiop also suggested using hardlinks instead.

@skshetry skshetry mentioned this issue Nov 15, 2021
Closed
@daavoo daavoo added the A: data-management Related to dvc add/checkout/commit/move/remove label Feb 22, 2022
@dberenbaum
Copy link
Collaborator

@skshetry Do you think we should include this as part of the data epic?

skshetry added a commit to skshetry/dvc that referenced this issue Apr 27, 2022
@skshetry
add: do not delete stage files before add
c3701ce 
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 iterative#2886 and iterative#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 iterative#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 skshetry mentioned this issue Jul 30, 2024
Merged
@skshetry
Copy link
Member Author

Closed by

and, released in https://github.com/iterative/dvc/releases/tag/3.54.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A: data-management Related to dvc add/checkout/commit/move/remove enhancement Enhances DVC performance improvement over resource / time consuming tasks ui user interface / interaction
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@dberenbaum @pared @daavoo @skshetry