Repository: skypilot-org/skypilot
Pull request #5092 (2025-04-02 04:20:42)

[API Server] Fix zip exclude files
<!-- Describe the changes in this PR -->

Previously, we did not exclude the files under an ignored folder in `.skyignore`.

To reproduce, see the newly added unit tests.

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [ ] Code formatting: install pre-commit (auto-check on commit) or `bash format.sh`
- [ ] Any manual or new tests for this PR (please specify below)
- [ ] All smoke tests: `/smoke-test` (CI) or `pytest tests/test_smoke.py` (local)
- [ ] Relevant individual tests: `/smoke-test -k test_name` (CI) or `pytest tests/test_smoke.py::test_name` (local)
- [ ] Backward compatibility: `/quicktest-core` (CI) or `pytest tests/smoke_tests/test_backward_compat.py` (local)

<!-- CI commands (/-prefixed) can only be triggered by repo members -->


Michaelvll: /smoke-test -k test_excluded_file_cloud_storage_upload_copy 

Patch:
diff --git a/sky/data/storage_utils.py b/sky/data/storage_utils.py
index afe5cc41f..b6ec4128e 100644
--- a/sky/data/storage_utils.py
+++ b/sky/data/storage_utils.py
@@ -223,7 +223,7 @@ def get_excluded_files_from_gitignore(src_dir_path: str) -> List[str]:
 def get_excluded_files(src_dir_path: str) -> List[str]:
     # TODO: this could return a huge list of files,
     # should think of ways to optimize.
-    """ List files and directories to be excluded."""
+    """List files and directories to be excluded."""
     expand_src_dir_path = os.path.expanduser(src_dir_path)
     skyignore_path = os.path.join(expand_src_dir_path,
                                   constants.SKY_IGNORE_FILE)
@@ -273,12 +273,22 @@ def zip_files_and_folders(items: List[str],
                     zipf.write(item)
                 elif os.path.isdir(item):
                     for root, dirs, files in os.walk(item, followlinks=False):
+                        # Modify dirs in-place to control os.walk()'s traversal
+                        # behavior. This filters out excluded directories BEFORE
+                        # os.walk() visits the files and sub-directories under
+                        # them, preventing traversal into any excluded directory
+                        # and its contents.
+                        # Note: dirs[:] = ... is required for in-place
+                        # modification.
+                        dirs[:] = [
+                            d for d in dirs
+                            if os.path.join(root, d) not in excluded_files
+                        ]
+
                         # Store directory entries (important for empty
                         # directories)
                         for dir_name in dirs:
                             dir_path = os.path.join(root, dir_name)
-                            if dir_path in excluded_files:
-                                continue
                             # If it's a symlink, store it as a symlink
                             if os.path.islink(dir_path):
                                 _store_symlink(zipf, dir_path, is_dir=True)


Test patch:
diff --git a/tests/unit_tests/test_storage_utils.py b/tests/unit_tests/test_storage_utils.py
index 38725f33d..421c91e07 100644
--- a/tests/unit_tests/test_storage_utils.py
+++ b/tests/unit_tests/test_storage_utils.py
@@ -134,6 +134,84 @@ def test_zip_files_and_folders(skyignore_dir):
         assert f'Zipped {skyignore_dir}' in log_file_content
 
 
+def test_zip_files_and_folders_excluded_directories():
+    """Test that files inside excluded directories are not included in zip file.
+    
+    File/directory structure:
+        temp_dir/ (temporary directory)
+        └── main_dir/
+            ├── main_file.txt         # contains "main file content"
+            ├── .skyignore            # contains "excluded_dir"
+            └── excluded_dir/         # this directory should be excluded
+                ├── excluded_file.txt # contains "excluded file content" 
+                └── nested_dir/
+                    └── nested_file.txt # contains "nested file content"
+
+    .skyignore content: 
+        excluded_dir
+    """
+    with tempfile.TemporaryDirectory() as temp_dir:
+        # Create a directory structure with nested files
+        main_dir = os.path.join(temp_dir, 'main_dir')
+        excluded_dir = os.path.join(main_dir, 'excluded_dir')
+        nested_dir = os.path.join(excluded_dir, 'nested_dir')
+
+        # Create directories
+        for dir_path in [main_dir, excluded_dir, nested_dir]:
+            os.makedirs(dir_path, exist_ok=True)
+
+        # Create files in each directory
+        with open(os.path.join(main_dir, 'main_file.txt'),
+                  'w',
+                  encoding='utf-8') as f:
+            f.write('main file content')
+
+        with open(os.path.join(excluded_dir, 'excluded_file.txt'),
+                  'w',
+                  encoding='utf-8') as f:
+            f.write('excluded file content')
+
+        with open(os.path.join(nested_dir, 'nested_file.txt'),
+                  'w',
+                  encoding='utf-8') as f:
+            f.write('nested file content')
+
+        # Create a skyignore file that excludes the directory
+        skyignore_content = "excluded_dir\n"
+        skyignore_path = os.path.join(main_dir, constants.SKY_IGNORE_FILE)
+        with open(skyignore_path, 'w', encoding='utf-8') as f:
+            f.write(skyignore_content)
+
+        # Create a temporary zip file
+        log_file = io.StringIO()
+        with tempfile.NamedTemporaryFile('wb+', suffix='.zip') as zip_file:
+            # Zip the main directory
+            storage_utils.zip_files_and_folders([main_dir], zip_file, log_file)
+
+            # Examine the zip content
+            zip_file.seek(0)
+            with zipfile.ZipFile(zip_file, 'r') as zipf:
+                zipped_files = zipf.namelist()
+
+            # Check for included files (using path suffixes)
+            assert any(
+                path.endswith('main_dir/main_file.txt')
+                for path in zipped_files)
+            assert any(
+                path.endswith(f'main_dir/{constants.SKY_IGNORE_FILE}')
+                for path in zipped_files)
+
+            # Check that excluded files are NOT included
+            assert not any(
+                path.endswith('excluded_file.txt') for path in zipped_files)
+            assert not any(
+                path.endswith('nested_file.txt') for path in zipped_files)
+
+            # Double-check by verifying the total number of files
+            # We should only have the main directory files
+            assert len(zipped_files) == 2  # main_file.txt and .skyignore
+
+
 def test_get_excluded_files_from_gitignore_with_submodules():
     """Test gitignore exclusion with submodules."""
     with tempfile.TemporaryDirectory() as temp_dir: