Optimize ignore performance #4120

karajan1001 · 2020-06-26T10:20:45Z

Remove unnecessary file check during .dvcignore file finding
Merge ignore rules in different .dvcignore files
Use a trie tree to make .dvcignore files only influence it sub dirs.

❗ 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.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

This is the benchmark

The first is <1.0.1>, it is the baseline. Because this optimization involving merging of different .dvcignore files. I add another Benchmark testing dvc status with three files each having 10 rules.

Second is this PR. Now ignore file check takes little time both in 3 files or 30 files
@Suor you are right, we had rechecked files found via walk, run ignores twice. And it happened within the time when we are walking to find all .dvcignore files.

dvc/dvc/ignore.py

Line 126 in 83a2afc

dirs[:], files[:] = self(root, dirs, files)

This PR solve another issue in

dvc/dvc/ignore.py

Line 135 in 83a2afc

dirs, files = ignore(root, dirs, files)

If some files were ignored by some dvcignore files, then it could not be reincluded by other dvcignore files.

Git did not work like this.

And in gitignore

Patterns read from a .gitignore file in the same directory as the path, or in any parent directory, with patterns in the higher level> files (up to the toplevel of the work tree) being overridden by those in lower level files down to the directory containing the file. These patterns match relative to the location of the .gitignore file. A project normally includes such .gitignore files in its repository, containing patterns for files generated as part of the project build.

fix iterative#3869

karajan1001 · 2020-06-27T04:15:04Z

(0.00 durations hidden.  Use -vv to show these durations.)
=========================== short test summary info ===========================
FAILED tests/func/test_ignore.py::test_multi_ignore_file - AssertionError: as...
===== 1 failed, 1323 passed, 89 skipped, 8 warnings in 713.38s (0:11:53) ======
The command "bash ./scripts/ci/script.sh" exited with 1.
cache.2
store build cache

Might there be more details in the log from Travis CI ?

pared · 2020-06-27T06:45:28Z

@karajan1001
The log from Travis indicates that test_ignore_multi failed.

   def test_multi_ignore_file(tmp_dir, dvc, monkeypatch):
        tmp_dir.gen({"dir": {"subdir": {"should_ignore": "1", "not_ignore": "1"}}})
        tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "dir/subdir/*_ignore")
        tmp_dir.gen({"dir": {DvcIgnore.DVCIGNORE_FILE: "!subdir/not_ignore"}})
    
>       assert _files_set("dir", dvc.tree) == {
            "dir/subdir/not_ignore",
            "dir/{}".format(DvcIgnore.DVCIGNORE_FILE),
        }
E       AssertionError: assert {'dir/.dvcignore'} == {'dir/.dvcign...r/not_ignore'}
E         Extra items in the right set:
E         'dir/subdir/not_ignore'
E         Full diff:
E         - {'dir/.dvcignore', 'dir/subdir/not_ignore'}
E         + {'dir/.dvcignore'}

karajan1001 · 2020-06-27T07:24:55Z

@karajan1001
The log from Travis indicates that test_ignore_multi failed.

   def test_multi_ignore_file(tmp_dir, dvc, monkeypatch):
        tmp_dir.gen({"dir": {"subdir": {"should_ignore": "1", "not_ignore": "1"}}})
        tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "dir/subdir/*_ignore")
        tmp_dir.gen({"dir": {DvcIgnore.DVCIGNORE_FILE: "!subdir/not_ignore"}})
    
>       assert _files_set("dir", dvc.tree) == {
            "dir/subdir/not_ignore",
            "dir/{}".format(DvcIgnore.DVCIGNORE_FILE),
        }
E       AssertionError: assert {'dir/.dvcignore'} == {'dir/.dvcign...r/not_ignore'}
E         Extra items in the right set:
E         'dir/subdir/not_ignore'
E         Full diff:
E         - {'dir/.dvcignore', 'dir/subdir/not_ignore'}
E         + {'dir/.dvcignore'}

Thank you, but the question is where to get this.

pared · 2020-06-27T08:36:23Z

@karajan1001, you don't see the 'Details' link near check?

If you follow it, there should be build summary, and you should see Jobs and Stages section, where you can see particular builds.

codecov · 2020-06-27T20:55:10Z

Codecov Report

Merging #4120 into master will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4120      +/-   ##
==========================================
+ Coverage   92.60%   92.76%   +0.15%     
==========================================
  Files         162      162              
  Lines       11335    11278      -57     
==========================================
- Hits        10497    10462      -35     
+ Misses        838      816      -22

Impacted Files	Coverage Δ
dvc/repo/update.py	`88.23% <0.00%> (-5.89%)`	⬇️
dvc/dependency/repo.py	`96.87% <0.00%> (-1.57%)`	⬇️
dvc/remote/azure.py	`93.58% <0.00%> (-0.67%)`	⬇️
dvc/cli.py	`94.11% <0.00%> (-0.43%)`	⬇️
dvc/repo/tree.py	`92.55% <0.00%> (-0.41%)`	⬇️
dvc/scm/tree.py	`95.74% <0.00%> (-0.26%)`	⬇️
dvc/external_repo.py	`86.59% <0.00%> (-0.14%)`	⬇️
dvc/config.py	`97.71% <0.00%> (-0.14%)`	⬇️
dvc/repo/plots/template.py	`96.42% <0.00%> (-0.10%)`	⬇️
dvc/command/git_hook.py	`95.74% <0.00%> (-0.09%)`	⬇️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66def03...3791998. Read the comment docs.

karajan1001 · 2020-06-27T22:18:29Z

Needs more tests. And Some Improvements

karajan1001 · 2020-06-28T05:26:01Z

If you follow it, there should be build summary, and you should see Jobs and Stages section, where you can see particular builds.
@pared

For example in
https://travis-ci.com/github/iterative/dvc/jobs/354665083

I can only get this.

No more detail.

efiop · 2020-06-29T01:37:32Z

Hi @karajan1001 ! Full logs are starting here https://travis-ci.com/github/iterative/dvc/jobs/354665083#L3357

karajan1001 · 2020-06-29T03:06:21Z

Hi @karajan1001 ! Full logs are starting here https://travis-ci.com/github/iterative/dvc/jobs/354665083#L3357

@efiop Thank you, it seems that only members of the DVC have the permission.

karajan1001 · 2020-06-29T03:17:00Z

More tests are needed, especially the merging of two ignore files and the influence of the changing of the dirname .

skshetry · 2020-06-29T06:47:24Z

@karajan1001, from your #4120 (comment), it looks like you are able to access it. You have to search manually in that huge log for specific test failure. Or, download the raw log and grep locally.

If you are not able to, you can download the log of a specific job with following url:

https://api.travis-ci.com/v3/job/{job-id}/log.txt (subsitute job id)

Eg: https://api.travis-ci.com/v3/job/354665083/log.txt

karajan1001 · 2020-06-29T09:00:01Z

@karajan1001, from your #4120 (comment), it looks like you are able to access it. You have to search manually in that huge log for specific test failure. Or, download the raw log and grep locally.

If you are not able to, you can download the log of a specific job with following url:

https://api.travis-ci.com/v3/job/{job-id}/log.txt (subsitute job id)

Eg: https://api.travis-ci.com/v3/job/354665083/log.txt

@skshetry Thank you, but, I had tried this on this PR 2 days ago. The result is the same as what is in the https://api.travis-ci.com/v3/job/354665083/log.txt

pared

1 thing that I found.

More tests are needed, especially the merging of two ignore files and the influence of the changing of the dirname .

Besides that LGTM. @karajan1001 Do you intend to introduce those tests in this PR?

dvc/ignore.py

karajan1001 · 2020-06-29T13:58:23Z

1 thing that I found.

More tests are needed, especially the merging of two ignore files and the influence of the changing of the dirname .

Besides that LGTM. @karajan1001 Do you intend to introduce those tests in this PR?

Of course. I would like to add every test I could think of according to the gitignore document. It would be a systematic test.

Because it may get DVC users in trouble if this algorithm changed frequently. Like what PathSpec's author said in cpburnz/python-path-specification#19

I suspect the discrepancy is that a .gitignore does additional filtering beyond that of Git's wildmatch algorithm. If I fixed this in my implementation of wildmatch, it could unexpectedly change the results of anyone already using this for a purpose other than emulating .gitingore.

karajan1001 · 2020-07-05T08:06:47Z

Current benchmark result

karajan1001 · 2020-07-05T08:09:23Z

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received
The build has been terminated

@pared Excuse me, What is this error? Never saw this before, seems not caused by my PR?

pared · 2020-07-06T09:13:02Z

@karajan1001 That does not seem like like an issue with your PR, I got the same error in other repository handled by Travis. Maybe it's their issue. For now, I am restarting the build, lets see how does it go this time.

karajan1001 · 2020-07-06T09:15:41Z

@karajan1001 That does not seem like like an issue with your PR, I got the same error in other repository handled by Travis. Maybe it's their issue. For now, I am restarting the build, lets see how does it go this time.

@pared Thank you very much.

Suor

It looks like you are reimplementing or completing implementing GitWildMatchPattern:

Is this unavodable?
If it is then can this be made in GitWildMatchPattern?
If it can't then can it be separated from dvc/ignore, i.e. any logic not specific to dvc moved separately?

dvc/ignore.py

karajan1001 · 2020-07-06T09:50:13Z

It looks like you are reimplementing or completing implementing GitWildMatchPattern:

Is this unavodable?

If it is then can this be made in GitWildMatchPattern?

If it can't then can it be separated from dvc/ignore, i.e. any logic not specific to dvc moved separately?

@Suor Thank you.
I learned a lot from your following advices.
But, I didn't reimplement GitWildMatchPattern. What I'm doing is only changing the relative path of the .dvcignore file. And this is required if we merged two .dvcignore rules in different paths.
For example, if we had

a
b

in /home/.dvcignore
and

!aa
!bb

in /home/dir/.dvcignore

And if we want to replace these two files in one. We must translate one of them to another's directory.

We can choose:

change /home/.dvcignore into /home/dir but it is harder to implement

**/dir/a

had to be overwritten into

**/dir/a # match /home/dir/dir/a
/a # match /home/a

change /home/dir/.dvcignore into /home/ and then it would be

!/dir/**/aa
!/dir/**/bb

And this is what I had done.

Suor · 2020-07-06T10:49:07Z

@karajan1001 Yes, this what I meant. You are implementing some math on top of GitWildMatchPattern. It looks like pathspec is a proper place to place this math or at least make it separate from dvc/ignore, i.e. make some pathspec_math.py with all those ops, which might be used from dvc/ignore:

pattern = join_patterns(
    (prefix1, pattern1),
    (prefix2, pattern2),
    ...
)

Then this pathspec_math might be separated or even contributed back to pathspec.

karajan1001 · 2020-07-06T11:11:14Z

@karajan1001 Yes, this what I meant. You are implementing some math on top of GitWildMatchPattern. It looks like pathspec is a proper place to place this math or at least make it separate from dvc/ignore, i.e. make some pathspec_math.py with all those ops, which might be used from dvc/ignore:
pattern = join_patterns(
    (prefix1, pattern1),
    (prefix2, pattern2),
    ...
)
Then this pathspec_math might be separated or even contributed back to pathspec.

@Suor . Thank you.
Sorry that I overlooked point 3.

I'm busy at work and can't pay much attention here.

Suor · 2020-07-06T15:07:10Z

@karajan1001 NP. This is really nice performance tuning work.

dvc/pathspec_math.py

dvc/ignore.py

Suor · 2020-07-13T09:35:13Z

dvc/ignore.py

+        common_dirname, merged_pattern = merge_patterns(
+            base_pattern.dirname,
+            base_pattern.pattern_list,
+            ignore_pattern.dirname,
+            ignore_pattern.pattern_list,
+        )


So we collect everything as we go in. We still leave partial results along the way. This means the overall structure will take quadratic memory depending on tree depth. Is this ok?

@Suor
Yes, the inherited pattern would be duplicated several times if we got several .dvcignore files. If not so, we would merge all of them at the file's checking. Space or speed? It depends on which is more important, I think.

Suor · 2020-07-13T09:39:30Z

tests/func/test_ignore.py

+    }
+
+
+def test_dvcignore_pattern_merge(tmp_dir, dvc):


I think this might be easier done with a unit test calling merge_patterns directly.

These tests mainly test the pattern trie tree not only merge patterns. I had renamed it.

list comprehension Co-authored-by: Alexander Schepanovski <[email protected]>

dvc/ignore.py

efiop · 2020-07-14T23:28:14Z

Thank you @karajan1001 !

karajan1001 added 3 commits June 23, 2020 22:28

Remove Duplicate ignore Match

858b57b

Continue Optimize Dvcignore

9ed4b7d

fix iterative#3869

Add a new test with multi ignore files

516f59f

efiop requested a review from pared June 26, 2020 14:21

karajan1001 added 2 commits June 27, 2020 10:54

Solve merging two dvc files

ae0dfef

Solve Code Climate

93d278a

karajan1001 changed the title ~~Fix ignore performance~~ Optimize ignore performance Jun 27, 2020

For Windows

25bfcd3

Complete addition of patterns. Add one test

8cf7b09

karajan1001 mentioned this pull request Jun 28, 2020

.dvcignore is broken on negation when blacklisting all #4103

Closed

pared suggested changes Jun 29, 2020

View reviewed changes

dvc/ignore.py Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Jul 5, 2020

Weekly Digest (28 June, 2020 - 5 July, 2020) #4168

Closed

karajan1001 added 3 commits July 5, 2020 15:22

Systematic test

03579de

Change request

640bf6f

Merge remote-tracking branch 'origin/master' into fix_ignore_performance

11019eb

Suor suggested changes Jul 6, 2020

View reviewed changes

dvc/ignore.py Show resolved Hide resolved

dvc/ignore.py Outdated Show resolved Hide resolved

dvc/ignore.py Outdated Show resolved Hide resolved

dvc/ignore.py Outdated Show resolved Hide resolved

dvc/ignore.py Outdated Show resolved Hide resolved

Change request

bb8a246

karajan1001 added 2 commits July 8, 2020 18:52

Merge remote-tracking branch 'origin/master' into fix_ignore_performance

6cd0c64

Seperate path sepcification math

cf3d235

pared reviewed Jul 10, 2020

View reviewed changes

dvc/pathspec_math.py Outdated Show resolved Hide resolved

pared approved these changes Jul 10, 2020

View reviewed changes

pared requested a review from Suor July 10, 2020 12:59

karajan1001 added 2 commits July 11, 2020 11:46

Rename and add comment

ec723bf

rename change_dirname to private

c629d30

Suor suggested changes Jul 13, 2020

View reviewed changes

karajan1001 and others added 2 commits July 13, 2020 17:54

Update dvc/pathspec_math.py

de22728

list comprehension Co-authored-by: Alexander Schepanovski <[email protected]>

Change request

012bb4a

efiop reviewed Jul 14, 2020

View reviewed changes

dvc/ignore.py Outdated Show resolved Hide resolved

Merge branch 'master' into fix_ignore_performance

fec7cd3

restyled-io bot mentioned this pull request Jul 14, 2020

Restyle Optimize ignore performance #4207

Closed

efiop reviewed Jul 14, 2020

View reviewed changes

dvc/ignore.py Outdated Show resolved Hide resolved

Update dvc/ignore.py

3791998

efiop merged commit 58f6534 into iterative:master Jul 14, 2020

weekly-digest bot mentioned this pull request Jul 19, 2020

Weekly Digest (12 July, 2020 - 19 July, 2020) #4238

Closed

karajan1001 deleted the fix_ignore_performance branch April 7, 2021 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize ignore performance #4120

Optimize ignore performance #4120

karajan1001 commented Jun 26, 2020 •

edited

Loading

karajan1001 commented Jun 27, 2020 •

edited

Loading

pared commented Jun 27, 2020

karajan1001 commented Jun 27, 2020

pared commented Jun 27, 2020

codecov bot commented Jun 27, 2020 •

edited

Loading

karajan1001 commented Jun 27, 2020

karajan1001 commented Jun 28, 2020

efiop commented Jun 29, 2020

karajan1001 commented Jun 29, 2020

karajan1001 commented Jun 29, 2020 •

edited

Loading

skshetry commented Jun 29, 2020

karajan1001 commented Jun 29, 2020

pared left a comment

karajan1001 commented Jun 29, 2020 •

edited

Loading

karajan1001 commented Jul 5, 2020

karajan1001 commented Jul 5, 2020 •

edited

Loading

pared commented Jul 6, 2020

karajan1001 commented Jul 6, 2020

Suor left a comment

karajan1001 commented Jul 6, 2020 •

edited

Loading

Suor commented Jul 6, 2020 •

edited

Loading

karajan1001 commented Jul 6, 2020

Suor commented Jul 6, 2020

Suor Jul 13, 2020

karajan1001 Jul 13, 2020 •

edited

Loading

Suor Jul 13, 2020

karajan1001 Jul 13, 2020

efiop commented Jul 14, 2020

Optimize ignore performance #4120

Optimize ignore performance #4120

Conversation

karajan1001 commented Jun 26, 2020 • edited Loading

karajan1001 commented Jun 27, 2020 • edited Loading

pared commented Jun 27, 2020

karajan1001 commented Jun 27, 2020

pared commented Jun 27, 2020

codecov bot commented Jun 27, 2020 • edited Loading

Codecov Report

karajan1001 commented Jun 27, 2020

karajan1001 commented Jun 28, 2020

efiop commented Jun 29, 2020

karajan1001 commented Jun 29, 2020

karajan1001 commented Jun 29, 2020 • edited Loading

skshetry commented Jun 29, 2020

karajan1001 commented Jun 29, 2020

pared left a comment

Choose a reason for hiding this comment

karajan1001 commented Jun 29, 2020 • edited Loading

karajan1001 commented Jul 5, 2020

karajan1001 commented Jul 5, 2020 • edited Loading

pared commented Jul 6, 2020

karajan1001 commented Jul 6, 2020

Suor left a comment

Choose a reason for hiding this comment

karajan1001 commented Jul 6, 2020 • edited Loading

Suor commented Jul 6, 2020 • edited Loading

karajan1001 commented Jul 6, 2020

Suor commented Jul 6, 2020

Suor Jul 13, 2020

Choose a reason for hiding this comment

karajan1001 Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Suor Jul 13, 2020

Choose a reason for hiding this comment

karajan1001 Jul 13, 2020

Choose a reason for hiding this comment

efiop commented Jul 14, 2020

karajan1001 commented Jun 26, 2020 •

edited

Loading

karajan1001 commented Jun 27, 2020 •

edited

Loading

codecov bot commented Jun 27, 2020 •

edited

Loading

karajan1001 commented Jun 29, 2020 •

edited

Loading

karajan1001 commented Jun 29, 2020 •

edited

Loading

karajan1001 commented Jul 5, 2020 •

edited

Loading

karajan1001 commented Jul 6, 2020 •

edited

Loading

Suor commented Jul 6, 2020 •

edited

Loading

karajan1001 Jul 13, 2020 •

edited

Loading