Special dir warning #3436 #4229

aerubanov · 2020-07-17T13:07:56Z

Added a warning message when traversing special directories and a test for that. Also removed unused arguments in a couple of tests in tests/func/test_ignore.py

efiop · 2020-07-18T05:02:51Z

dvc/ignore.py

+    special_dirs = {
+        ".petest_cache",
+        ".mypy_cache",
+        "node_modules",
+        ".venv",
+        "venv",
+        "env",
+        "__pycache__",
+        "dist",
+        "eggs",
+        "wheels",
+        "htmlcov",
+        "docs",
+        ".ipynb_checkpoints",
+        ".vscode",
+        ".github",
+    }


Hm, makes me wonder if we should really just ignore them by default (like we do with .git and .hg), and if some user really needs these, he could !something in dvcignore to include it.

Or, we might consider generating a default .dvcignore (kinda like github generates .gitignore when you create a project) to make it extra explicit. Though this is simple on init, but managing this stuff in existing projects is not too simple 🙁

I'm just worried about these warnings getting too annoying. Plus, unless I'm missing something, ignore execution order is not deterministic (that's actually is a bigger problem for us in general), so you never know if this filter will be executed before or after .dvcignores in which user has ignored a special dir explicitly. 🙁

One more thing that this makes me think about is having some kind of safe guard, that would, for example, record the amount of time it took to walk through the directory and warn if it didn't find anything but it took, say, more than 5 sec.

This issue was opened after this conversation #3257. It was decided that it is better to warn the user if a special directory is found than to ignore it by default. But annoyance from warnings can be a problem. Maybe we should make it possible to disable this warning? From the other hand, the default .dvcignore also good idea because the ignore will be explicite to the user.

What makes idea of default .dvcignore compelling, is the fact that is not only explicit, but also editable by user.

If we decide to go with warnings, they definitely need turn off mechanism, just like we did with slow_link_guard.

Now I soppouse that the default .dvcignore, is better option. It`s more explicite and easy to configure. Also the list of special dirs always will be incomplite, we can not add all posiible variants here. And default .dvcignore provide additional example for users how to deal with such cases.

@efiop, @pared, if you are agree with me, then I will change the code. Should I close this PR and open a new one later, or commit right here?

👍 I love the idea of an explicit .dvcignore

I wonder whether it should be default. Maybe we should add some option to init, or dedicate special command for that? It seems to me that .dvcignore was supposed to let user decide what is part of his project. If we always create default .dvcignore on init, we make it our choice. @efiop @aerubanov, what do you think?

I think that default .dvcignore should consist comment with link to the dvcignore documentation. And we should add option for init to filling .dvcignore with some content. Not all list of special folder, rather based on type of project specified by user. Something like github gitignore template https://github.com/github/gitignore, but with fewer options ofcorse.

@aerubanov I think you are right on this one.

efiop

Amazing stuff @aerubanov ! Thank you! 🙏 Please see a comment above.

pared

I am not sure whether logic of finding potentially problematic dirs should be part of dvcignore.

Introducing this change in this form will cause us to check all directories that we visit during traversing.
That might have considerable impact for all users, while it will be helpful for a subset of them.

Maybe we should be doing this check only in a directory that also contains .dvc? It's not the perfect solution, but it would not impact each walk operation.

pared · 2020-07-22T10:57:11Z

dvc/ignore.py

+
+    def __call__(self, root, dirs, files):
+        for d in dirs:
+            if d in self.special_dirs:


I think we could do something like set(dirs).intersection(self.special_dirs) and print single warning for all problematic dirs.

aerubanov · 2020-07-26T15:04:41Z

I add default .dvcignore with link on docs and option for init to filling .dvcignore with some content based on project type specified by user. If there is no need for further changes to the functionality, I will add tests and changes to the documentation.

pared

LGTM @aerubanov
@shcheklein? @efiop?

efiop · 2020-07-28T22:01:31Z

dvc/ignore_templates/__init__.py

@@ -0,0 +1,2 @@
+# templates for .dvcignore files


This doesn't really have to be a module, as there is no actual code. You can either keep it as datafiles or maybe actually make these into the python shape by declaring these as strings. The latter way is easier to manage, as things like setup.py and pyinstaller will have a much easier time collecting these automatically.

efiop · 2020-07-28T22:04:16Z

dvc/ignore_templates/java

@@ -0,0 +1,23 @@
+# Compiled class file


Hm, I also wonder if these are really worth stealing directly from https://github.com/github/gitignore . They would be hard to maintain, really. But I'm not able to see a better way right now...

efiop · 2020-07-28T22:08:41Z

dvc/repo/__init__.py

+            no_scm=no_scm,
+            force=force,
+            subdir=subdir,
+            ignore_template_list=ignore_template_list,


People will probably want these even in existing projects, and they will probably forget to do this during the init phase. So we could add another command to install these, but maybe it is actually better to just document that people might want to get .dvcignore from https://github.com/github/gitignore and use it.

This also makes me wonder if we should should really somehow reuse existing .gitignores (e.g. to not search for dvc-files and .dvcignores in .gitignore'd directories). But I suppose many people also want to exclude these dirs from dirs they are dvc adding (or tracking), which doesn't play well, because we gitignore the data we are adding. 🤔

@efiop, Ok. May be we should add command for downloading template content from https://github.com/github/gitignore and pasting it in .dvcignore? Then, we need not to store it in dvc code base and maintain, and user need not to do it by hand.

And also we can add second option for this command to read existing ,gitignore, exlude all records that was added by dvc add and paste it in .dvcignore. Is it possible to easily get a list of all tracked files? (I think it should be, I just don't know where to look).

What do you think of these two possibilities?

@efiop, Ok. May be we should add command for downloading template content from https://github.com/github/gitignore and pasting it in .dvcignore? Then, we need not to store it in dvc code base and maintain, and user need not to do it by hand.

@aerubanov I don't think it is worth creating a special command. It is not hard to just go to a link in the docs and copypaste contents to .dvcignore (or use wget/curl/etc).

And also we can add second option for this command to read existing ,gitignore, exlude all records that was added by dvc add and paste it in .dvcignore. Is it possible to easily get a list of all tracked files? (I think it should be, I just don't know where to look).

That sounds a bit complex :( We had a discussion with @pmrowla about it, and maybe we could just use .gitignore directly, the only thing that is problematic is not dvcignoring dvc-tracked files, but that should be solvable by telling dvcignore about them when we collect the dag. We will soon have a pre-requisite PR that notifices dvcignore about those outputs for performace reasons and we might get back to this idea right after it.

Ok, i will make the necessary changes.

skshetry · 2020-08-05T11:20:00Z

dvc/repo/init.py

+        f.write(
+            "# This is .dvcignore file."
+            " See more https://dvc.org/doc/user-guide/dvcignore\n"
+        )


Good idea to create a default dvcignore. It'll make people aware of it. 👍 to that.

Regarding the comment, better to explain what it does. This is .dvcignore file. is clear from the filename.

Something like following, perhaps?

Suggested change

f.write(

"# This is .dvcignore file."

" See more https://dvc.org/doc/user-guide/dvcignore\n"

)

f.write(

"# add patterns of files dvc should ignore, can improve performance\n"

"# Learn more at https://dvc.org/doc/user-guide/dvcignore\n"

)

@skshetry Please see my comment above. Maybe we should just use .gitignore directly. Would love to know what you think about that.

Related with iterative/dvc#4229

* Add link on templates for .dvcignore. Related with iterative/dvc#4229 * Restyled by prettier Co-authored-by: Anatoly <44327258+aerubanov@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>

aerubanov · 2020-08-11T07:45:09Z

@efiop, @skshetry . I made the changes that we discussed above. Is there more to be done?

efiop · 2020-08-12T22:03:41Z

Thank you @aerubanov ! 🙏

Since iterative#4229, we have started creating .dvcignore file after `dvc init` This commit only adds that in assertions

Since #4229, we have started creating .dvcignore file after `dvc init` This commit only adds that in assertions

jorgeorpinel · 2020-08-26T20:54:48Z

dvc/ignore.py

+
+    with open(dvcignore, "w") as fobj:
+        fobj.write(
+            "# Add patterns of files dvc should ignore, which could improve\n"


Suggested change

"# Add patterns of files dvc should ignore, which could improve\n"

"# Add patterns of files DVC should ignore, which could improve\n"

aerubanov added 3 commits July 17, 2020 15:04

add special folder warning

94668a8

add special folder warning

8686f0d

merge

e0b26fc

skshetry linked an issue Jul 17, 2020 that may be closed by this pull request

Warn users when traversing special directories, recommend to ignore #3436

Closed

efiop reviewed Jul 18, 2020

View reviewed changes

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

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

Closed

pared self-requested a review July 22, 2020 10:32

pared reviewed Jul 22, 2020

View reviewed changes

aerubanov added 2 commits July 25, 2020 18:36

remove special folder warning, start working on default dvcignore

7dde434

Merge branch 'master' into special_dir_warning

0fa1a20

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

Weekly Digest (19 July, 2020 - 26 July, 2020) #4285

Closed

add default .dvcignote and templates

a3ee328

aerubanov requested review from pared, efiop and shcheklein July 26, 2020 15:04

pared reviewed Jul 27, 2020

View reviewed changes

efiop reviewed Jul 28, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Aug 2, 2020

Weekly Digest (26 July, 2020 - 2 August, 2020) #4315

Closed

skshetry reviewed Aug 5, 2020

View reviewed changes

aerubanov added 3 commits August 5, 2020 17:43

remove dvcignore templates

f98d742

change text and add tests

e8994dd

Merge branch 'master' into special_dir_warning

8da4d50

aerubanov added a commit to aerubanov/dvc.org that referenced this pull request Aug 6, 2020

Add link on templates for .dvcignore.

baa8563

Related with iterative/dvc#4229

aerubanov mentioned this pull request Aug 6, 2020

Add link on templates for .dvcignore. iterative/dvc.org#1666

Closed

weekly-digest bot mentioned this pull request Aug 9, 2020

Weekly Digest (2 August, 2020 - 9 August, 2020) #4365

Closed

pared requested review from efiop and skshetry August 11, 2020 09:53

efiop added 3 commits August 12, 2020 23:26

refactor

abfa2dc

dvcignore: ignore comments

50c0c84

adjust tests

824fbeb

efiop merged commit 5cb36fc into iterative:master Aug 12, 2020

aerubanov deleted the special_dir_warning branch August 13, 2020 14:06

skshetry mentioned this pull request Aug 15, 2020

RepoTree: add support for subrepo traversal #4381

Merged

2 tasks

skshetry added a commit to skshetry/dvc that referenced this pull request Aug 15, 2020

tests: add .dvcignore on the tests expectations

0758184

Since iterative#4229, we have started creating .dvcignore file after `dvc init` This commit only adds that in assertions

skshetry mentioned this pull request Aug 15, 2020

tests: add .dvcignore on the tests expectations #4396

Merged

2 tasks

skshetry added a commit that referenced this pull request Aug 15, 2020

tests: add .dvcignore on the tests expectations (#4396)

b773ba7

Since #4229, we have started creating .dvcignore file after `dvc init` This commit only adds that in assertions

jorgeorpinel reviewed Aug 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Special dir warning #3436 #4229

Special dir warning #3436 #4229

aerubanov commented Jul 17, 2020 •

edited

Loading

efiop Jul 18, 2020 •

edited

Loading

efiop Jul 18, 2020

aerubanov Jul 18, 2020 •

edited

Loading

pared Jul 22, 2020

aerubanov Jul 22, 2020

shcheklein Jul 22, 2020

pared Jul 23, 2020

aerubanov Jul 23, 2020

pared Jul 24, 2020

efiop left a comment

pared left a comment

pared Jul 22, 2020

aerubanov commented Jul 26, 2020

pared left a comment

efiop Jul 28, 2020

efiop Jul 28, 2020

efiop Jul 28, 2020

aerubanov Jul 31, 2020 •

edited

Loading

efiop Jul 31, 2020

aerubanov Aug 1, 2020

skshetry Aug 5, 2020

efiop Aug 6, 2020

aerubanov commented Aug 11, 2020

efiop commented Aug 12, 2020

jorgeorpinel Aug 26, 2020

	"# Add patterns of files dvc should ignore, which could improve\n"
	"# Add patterns of files DVC should ignore, which could improve\n"

Special dir warning #3436 #4229

Special dir warning #3436 #4229

Conversation

aerubanov commented Jul 17, 2020 • edited Loading

efiop Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerubanov Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop left a comment

Choose a reason for hiding this comment

pared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerubanov commented Jul 26, 2020

pared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerubanov Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerubanov commented Aug 11, 2020

efiop commented Aug 12, 2020

Choose a reason for hiding this comment

aerubanov commented Jul 17, 2020 •

edited

Loading

efiop Jul 18, 2020 •

edited

Loading

aerubanov Jul 18, 2020 •

edited

Loading

aerubanov Jul 31, 2020 •

edited

Loading