add: support simple wildcards #4864

ju0gri · 2020-11-09T15:06:51Z

Related to #4816.

Signed-off-by: Ioana Grigoropol [email protected]

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

iterative/dvc.org#1928

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

dvc/repo/add.py

efiop · 2020-11-09T18:33:23Z

Fixes #4816.

It doesn't really fix it, just use Related please, so that github doesn't close it automatically 🙂

tests/func/test_add.py

efiop · 2020-11-09T18:37:11Z

tests/func/test_add.py

+            }
+        }
+    )
+    subdir_path = os.path.join("dir", "subdir", "subdata*")


Should we test more features or just single wildcards?

I'll add some more use cases for shell-style wildcards, and possible some negative ones for recursive matching which will not work as it is now, unless we enable the recursive flag. do we want that ?

@ju0gri Note that the more the tests - the more expensive it get's, as we run them in real filesystem. I suppose os.glob has some spec that it sticks to, so it probably doesn't make sense to test it all.

Recursive flag is for double wildcards only, right? Definitely seems useful. Any reason why it is disabled by default? Maybe some perf issues (except for the obvious misuse).

Yes, the recursive flag enables **. According to their docs:

Using the “**” pattern in large directory trees may consume an inordinate amount of time.

Would adding one test for each use case defined in fnmatch manual be an acceptable compromise ?

* | matches everything ? | matches any single character [seq] | matches any character in seq [!seq] | matches any character not in seq ** | searches recursively

Yes, the recursive flag enables **. According to their docs:

Sure, I am wondering whether it has some hidden cons. E.g. maybe it is considerably slower by itself when that pattern is supported. I guess need to give it a shot/look into internals for that.

Would adding one test for each use case defined in fnmatch manual be an acceptable compromise ?

Sounds good.

Btw, pathspec might not be desired in some cases. E.g. dvc add '?*[seq]**', where all the special stuff is already shieled by the single quotes. Current patch enabled globbing by default, but it looks like we should probably make it optional.

@efiop - Yes, makes sense. I am bad with naming, but added an argument called pathpattern for the add command to enable globing of the passed in target. Also, as I am not very familiar with the structure of the repo, are there other places where I need to add coverage for the new argument ?

efiop · 2020-11-10T18:46:10Z

dvc/command/add.py

@@ -57,6 +58,12 @@ def add_parser(subparsers, parent_parser):
        default=False,
        help="Allow targets that are outside of the DVC repository.",
    )
+    parser.add_argument(
+        "--pathpattern",


How about --glob?

dvc/repo/add.py

Related to iterative#4816. Signed-off-by: Ioana Grigoropol <[email protected]>

Adds a new argument for the add command `glob` that is disabled by default and when enabled it passes the input targets through glob filtering. Related: iterative#4816 Signed-off-by: Ioana Grigoropol <[email protected]>

efiop

Looks good! The only thing left now is to create a ticket for docs or create a doc PR, as noted in the PR checkbox.

efiop · 2020-11-11T18:38:05Z

Thank you!

jorgeorpinel · 2020-11-12T09:22:58Z

dvc/command/add.py

+    parser.add_argument(
+        "--glob",
+        action="store_true",
+        default=False,
+        help="Allows targets containing shell-style wildcards.",
+    )


Hi! A few questions (related to documenting this per iterative/dvc.org/issues/1928):

What does "glob" abbreviate or stand for?

Why is a flag needed to allow/enable wildcards?

Is there a short description of what "shell-style" means exactly? Or a link to some Linux doc, Python package, or something specifying what can be done in terms of wildcards?

I think it's more natural to use wildcards in the default mode without any flag, just like what we do in Unix. And it‘s better to extract the glob to some common function that could be used in all commands.

@karajan1001 I personally wasn't ready to enable this by default right away as I'm not sure it won't affect negatively some existing usecases 🙁 Once we're ready for it, it will be as simple as turning that flag on by default. For PS users --glob will be a temporary inconvenience, but at least the functionality is being slowly filled up.

@jorgeorpinel Maybe just forget about the doc ticket for it for now, we'll be adding more stuff for it in the future.

What does "glob" abbreviate or stand for?

https://docs.python.org/3/library/glob.html

Why is a flag needed to allow/enable wildcards?

Usually, your shell handles the expansion and it will still do that. Our --glob is useful for programatic use or in shells that don't support explansion natively (e.g. powershell). I'm personally not ready to enable it by default right now.

Is there a short description of what "shell-style" means exactly? Or a link to some Linux doc, Python package, or something specifying what can be done in terms of wildcards?

https://docs.python.org/3/library/glob.html

Cool, just wanted to understand this. The initial docs PR is ready for merge.

…limit * 'master' of github.com:iterative/dvc: dag: add --outs option (iterative#4739) Add test server and tests for webdav (iterative#4827) Simpler param updates with python-benedict (iterative#4780) checkpoints: set DVC_ROOT environment variable (iterative#4877) api: add support for simple wildcards (iterative#4864) tests: mark azure test as flaky (iterative#4881) setup.py: limit responses version for moto (iterative#4879) remote: avoid chunking on webdav. Fixes iterative#4796 (iterative#4828) checkpoints: `exp run` and `exp res[ume]` refactor (iterative#4855)

ju0gri force-pushed the ioana-fix-4816 branch from 43bce27 to e288095 Compare November 9, 2020 16:31

efiop reviewed Nov 9, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

ju0gri force-pushed the ioana-fix-4816 branch from e288095 to c12eedf Compare November 9, 2020 17:26

efiop reviewed Nov 9, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

efiop reviewed Nov 9, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

ju0gri force-pushed the ioana-fix-4816 branch 2 times, most recently from 81a82d3 to d0299e4 Compare November 9, 2020 18:28

efiop reviewed Nov 9, 2020

View reviewed changes

tests/func/test_add.py Show resolved Hide resolved

efiop reviewed Nov 9, 2020

View reviewed changes

ju0gri force-pushed the ioana-fix-4816 branch from d0299e4 to 4a7cf15 Compare November 9, 2020 20:27

efiop reviewed Nov 10, 2020

View reviewed changes

ju0gri force-pushed the ioana-fix-4816 branch from 2093e45 to 06ce046 Compare November 11, 2020 11:19

efiop reviewed Nov 11, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

efiop reviewed Nov 11, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

efiop reviewed Nov 11, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

efiop reviewed Nov 11, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

efiop reviewed Nov 11, 2020

View reviewed changes

dvc/repo/add.py Outdated Show resolved Hide resolved

ju0gri and others added 7 commits November 11, 2020 17:47

api: add support for simple wildcards

6fcd151

Related to iterative#4816. Signed-off-by: Ioana Grigoropol <[email protected]>

api: make wildcard interpretation optional

a1d0f87

Adds a new argument for the add command `glob` that is disabled by default and when enabled it passes the input targets through glob filtering. Related: iterative#4816 Signed-off-by: Ioana Grigoropol <[email protected]>

Update dvc/repo/add.py

2f89e9f

Update dvc/repo/add.py

aa9098f

Update dvc/repo/add.py

f782e8b

Update dvc/repo/add.py

8aba2b7

Update dvc/repo/add.py

0a49415

ju0gri force-pushed the ioana-fix-4816 branch from d501c2e to 0a49415 Compare November 11, 2020 17:47

efiop approved these changes Nov 11, 2020

View reviewed changes

efiop merged commit 556143f into iterative:master Nov 11, 2020

efiop mentioned this pull request Nov 11, 2020

docs: add dvc add --glob to command reference iterative/dvc.org#1928

Closed

skshetry added the enhancement Enhances DVC label Nov 12, 2020

skshetry changed the title ~~api: add support for simple wildcards~~ add: support simple wildcards Nov 12, 2020

jorgeorpinel mentioned this pull request Nov 12, 2020

add support for wildcard/patterns #4816

Open

5 tasks

jorgeorpinel reviewed Nov 12, 2020

View reviewed changes

jorgeorpinel mentioned this pull request Nov 12, 2020

docs: document new argument glob for add command iterative/dvc.org#1929

Closed

efiop mentioned this pull request Nov 19, 2020

repro: glob-style selection of stages #4912

Closed

jorgeorpinel mentioned this pull request Dec 1, 2020

repro: support glob/foreach-group to run at once through CLI #4976

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: support simple wildcards #4864

add: support simple wildcards #4864

ju0gri commented Nov 9, 2020 •

edited by efiop

Loading

efiop commented Nov 9, 2020

efiop Nov 9, 2020

ju0gri Nov 9, 2020

efiop Nov 9, 2020

ju0gri Nov 9, 2020

ju0gri Nov 9, 2020 •

edited

Loading

efiop Nov 9, 2020

efiop Nov 9, 2020 •

edited

Loading

ju0gri Nov 10, 2020 •

edited

Loading

efiop Nov 10, 2020

efiop left a comment

efiop commented Nov 11, 2020

jorgeorpinel Nov 12, 2020

karajan1001 Nov 12, 2020

efiop Nov 12, 2020 •

edited

Loading

jorgeorpinel Nov 12, 2020

add: support simple wildcards #4864

add: support simple wildcards #4864

Conversation

ju0gri commented Nov 9, 2020 • edited by efiop Loading

efiop commented Nov 9, 2020

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

ju0gri Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

ju0gri Nov 10, 2020 • edited Loading

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

efiop commented Nov 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju0gri commented Nov 9, 2020 •

edited by efiop

Loading

ju0gri Nov 9, 2020 •

edited

Loading

efiop Nov 9, 2020 •

edited

Loading

ju0gri Nov 10, 2020 •

edited

Loading

efiop Nov 12, 2020 •

edited

Loading