gc: Pass list of paths to fs.remove.

[daavoo]

Use "bulk" remove in filesystems that support it.

Requires treeverse/dvc-objects#176 to don't break in localfs

Closes treeverse/dvc#5961

[daavoo]

Just doubt I have, why .dir files have to be removed first?

[skshetry]

We consider .dir files (and tracked objects inside) to be one single unit. We use this for optimization (assumptions such as if .dir file exists in the remote, all the directory contents also exist, etc).

https://github.com/iterative/dvc-data/blob/e0d19abd5d25525d8d4bc0068c9ac748f3c2aad6/src/dvc_data/hashfile/status.py#L63-L64

We also try to upload all directory contents, and then only if they succeed, we upload .dir files.

So ideally here, we should try to delete .dir file first, and then all of it's contents in bulk and so on for all objects. Then again do bulk for HashFile objects.

[daavoo]

So ideally here, we should try to delete .dir file first, and then all of it's contents in bulk and so on for all objects. Then again do bulk for HashFile objects.

Should we open a separate issue for that?

This P.R. just keeps the previous behavior but benefits from filesystems that implement bulk delete like gcfs and s3fs

[efiop]

@daavoo You probably don't want to remove that comment completely.

[daavoo]

Should it be better a # TODO: comment to implement what @skshetry described above?

[skshetry]

@daavoo, let's just leave it out. It's harder to associate later in practice (Eg: what if there are overlaps between different .dir files, etc.).

It is unsafe, but gc always is.

[pmrowla]

As long as we remove .dir files before all other files first then gc should be safe. The optimization isn't affected by overlaps between dirs. The only thing we assume when checking remote status is that if a .dir file exists on the remote, then all files inside that .dir exist. It's more unsafe for us to actually attempt to remove .dir+directory contents, then another .dir+directory contents, and so on, because then we do have to get into figuring out directory overlaps.

The existing behavior (and the current state of the PR) should be fine for now.

[codecov-commenter]

Codecov Report

Base: 56.87% // Head: 56.78% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (daf458f) compared to base (b615418).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
- Coverage   56.87%   56.78%   -0.10%     
==========================================
  Files          50       50              
  Lines        3082     3087       +5     
  Branches      536      538       +2     
==========================================
  Hits         1753     1753              
- Misses       1243     1248       +5     
  Partials       86       86              

Impacted Files	Coverage Δ
src/dvc_data/hashfile/gc.py	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.