Repository: wandb/wandb
Pull request: feat(sdk): speed up iterating over directory in the `filtered_dir` function by also filtering by directories 
Fixes #4322

Description
-----------
What does the PR do?
Checks also if a directory is to be excluded and if so we skip it

Testing
-------
Locally on my computer


Problem statement:
[Feature]: Speed up filtering files in a directory with excluded sub-directories
### Description

Currently, when calling `wandb.sdk.lib.filenames.filtered_dir`, the function iterates over every file and folder in the root directory.
The problem starts if there is a directory containing a lot of files that needs to be excluded - in such case, the function will iterate all the files in this directory for no reason.

### Suggested Solution

The function current implementation:
```
def filtered_dir(
    root: str, include_fn: Callable[[str], bool], exclude_fn: Callable[[str], bool]
) -> Generator[str, None, None]:
    """Simple generator to walk a directory"""
    for dirpath, _, files in os.walk(root):
        for fname in files:
            file_path = os.path.join(dirpath, fname)
            if include_fn(file_path) and not exclude_fn(file_path):
                yield file_path
```

Instead of ignoring the directories, one can iterate over the directories as well and exclude them if needed. The change is implemented in the following code snippet:

```
def filtered_dir(
    root: str, include_fn: Callable[[str], bool], exclude_fn: Callable[[str], bool]
) -> Generator[str, None, None]:
    """Simple generator to walk a directory"""
    for dirpath, dirs, files in os.walk(root):
        for dir in dirs:
            if exclude_fn(dir):
                dirs.remove(dir)
        for fname in files:
            file_path = os.path.join(dirpath, fname)
            if include_fn(file_path) and not exclude_fn(file_path):
                yield file_path
```

### Alternatives

_No response_

### Additional Context

_No response_

Hints from issue discussion:
Hi @amitabe!

Thank you for your suggestion, I can create an internal feature request for this, or if you would like - you can submit a PR to this repository as well.

Thanks,
Ramit

Commit message: Merge branch 'main' into master

Patch to review:
diff --git a/wandb/sdk/lib/filenames.py b/wandb/sdk/lib/filenames.py
index 8e914b05858..ac518bd45b3 100644
--- a/wandb/sdk/lib/filenames.py
+++ b/wandb/sdk/lib/filenames.py
@@ -32,7 +32,10 @@ def filtered_dir(
     root: str, include_fn: Callable[[str], bool], exclude_fn: Callable[[str], bool]
 ) -> Generator[str, None, None]:
     """Simple generator to walk a directory"""
-    for dirpath, _, files in os.walk(root):
+    for dirpath, dirs, files in os.walk(root):
+        for dir in dirs:
+            if exclude_fn(dir):
+                dirs.remove(dir)
         for fname in files:
             file_path = os.path.join(dirpath, fname)
             if include_fn(file_path) and not exclude_fn(file_path):


Code review comments:
diff_hunk: @@ -32,7 +32,10 @@ def filtered_dir(
     root: str, include_fn: Callable[[str], bool], exclude_fn: Callable[[str], bool]
 ) -> Generator[str, None, None]:
     """Simple generator to walk a directory"""
-    for dirpath, _, files in os.walk(root):
+    for dirpath, dirs, files in os.walk(root):
+        for dir in dirs:
+            if exclude_fn(dir):
path: wandb/sdk/lib/filenames.py
text: @user1:
your logic doesn't handle the case where `include_fn` and `exclude_fn` disagree. 
This should probably be resolved and communicated to the user if there are some contradictions. 

Also, would you mind adding tests for this change? (you probably want to add it  in `tests/unit_tests`)

@author:
But also when such an event happens on the file level (i.e. when a file is both included and occluded) there is no communication to the user and the file is being excluded.. I expect the same to happen on the folder level.
So what you are describing is another feature.. 
Regarding testing, is there an existing test for this function I can expand?

@user1:
Well, like you mentioned in the file case we check both `include_fn` and `exclude_fn` so for the extended logic to be complete you need to handle this case as well. Potentially, you could modify your condition to account for these cases. 

Regarding testing, there is no specific test for this function, but you could use one of the test that test the `log_code` function since it uses this function, but I think it would be better to test this function separate as a unit test.