Add ignore keyword argument to copytree

[aaossa]

Following #145 , the ignore argument works similarly to shutil.copytree, and supports shutil.ignore_patterns to allow using a similar interface. Added two tests by creating additional files and ignoring them using shutil.ignore_patterns and a custom ignore function.

Closes #145

[pjbull]

Approved the CI tests. Note: the live tests will fail since this is a PR from a fork, but that is fine. We'll run those on a local branch once this PR is ready.

[aaossa]

Fixed the warnings from Black 👌 Wasn't sure if this action was used in the project. I think there's already an issue suggesting the inclusion of a CONTRIBUTING.md file, but it has my +1 to explicitly state that Black is being used and also explain how to execute the tests (make test)

[pjbull]

I think there's already an issue suggesting the inclusion of a CONTRIBUTING.md file, but it has my +1 to explicitly state that Black is being used and also explain how to execute the tests (make test)

Yep, thanks. Would you mind noting anything you hit in that issue so it can go in when we write it?

[aaossa]

Ok, seems like the latest issue was related to typing. I was using the wrong return type for the ignore keyword argument. Instead of collections.abc.Sequence the correct type is collections.abc.Iterable because a set is not a sequence.

---

Yep, thanks. Would you mind noting anything you hit in that issue so it can go in when we write it?

Sure, I'll leave a comment with some ideas

[pjbull]

Two thoughts here:

(1) I think that we may want self._no_prefix_no_drive instead of self.fspath, which removes the s3://bucket portion but keeps the rest. This way the first argument won't include the path to the potentially arbitrary local folder in the cache.

(2) I know it follows the CPython implementation, but it's not ideal to call iterdir twice since there's network call overhead on both of them. Would it be feasible to do something like the following within the main loop instead?

...

for subpath in self.iterdir():
    if not ignore(subpath.parent._no_prefix_no_drive, [subpath.name]):
        continue
    ...  

Maybe not ideal if calls to ignore are expensive, but there's a tradeoff with the network overhead of iterdir that may be worth it.

[pjbull]

Just looked a little closer at the CPython one—at least point 2 we could address like their implementation where we introduce a _copytree that gets passed entries so that we don't do the expensive operation twice (like they do to avoid calling scandir twice)

[aaossa]

Yeah, I get it. Is not a bad idea since it replaces a single call on every file for multiple calls on a single file each time, but keeping the interface untouched. Would it be a bad idea to cache or store the result of self.iterdir() and use it twice?

contents = list(self.iterdir())  # Using list because iterdir is a generator

if ignore is not None:
    ignored_names = ignore(subpath.parent._no_prefix_no_drive, [x.name for x in contents])
else:
    ignored_names = set()

# ...

for subpath in contents:
    # ...

EDIT: Just noticed your second comment, I'll give it a try later

[aaossa]

Seems like what I proposed is equivalent to what you described. They do the step of creating a list of entries here

[pjbull]

👍 Oh yeah, that looks simple and saves us some network overhead! Let's do it

[aaossa]

Ah, maybe we should still use their solution. copytree is a recursive function, so we still are making network calls (at least is half now). Seems like the only way to make a single network call is using their approach to determine the list of entries before actually calling copytree (which will be named _copytree then)

[aaossa]

Also, the recursive call (when subpath is a directory) also needs the ignore argument passed

[aaossa]

Ah, maybe we should still use their solution. copytree is a recursive function, so we still are making network calls (at least is half now). Seems like the only way to make a single network call is using their approach to determine the list of entries before actually calling copytree (which will be named _copytree then)

NVM, seems like this won't make a difference since iterdir is not recursive, so we actually need to call iterdir on each depth level of the recursive function

[aaossa]

Also, the recursive call (when subpath is a directory) also needs the ignore argument passed

Fixed in the latest push

[pjbull]

Thanks @aaossa! I've pulled your changes into a branch that will run tests against the live servers and opened a PR for that branch at #273. If those all pass, we'll merge it. Much appreciated.