Azure / Azure/azure-sdk-for-python

get_upload_files_from_folder is very slow even when folders with many files are ignored

Open
#39,235 4 comments 0 reactions 1 assignee Claimed by @achauhan-scc View on GitHub
Client customer-reported feature-request Machine Learning needs-team-attention Service Attention
Dominant language
Python
Stars
5.6k
Forks
3.4k
Avg merge
1d 21h
Merged PRs (30d)
193

Description

- **Package Name**: azure-ai-ml
- **Package Version**: 1.22.4
- **Operating System**: Windows
- **Python Version**: 3.10

**Describe the bug**

I have large folder in my repository that contains python environments. I added these folder to the `.amlignore` file so that they do not get uploaded to AML when launching a job. Despite this folders are ignored, the process of launching a job very slow to a point where is a barely usable. This is because the function `get_upload_files_from_folder`
in `azure\ai\ml\_utils\_asset_utils.py` has been implemented using ` os.walk` which iterates recursively over all the files in these folders despite they are ignore and tests each file in these folders against the aml ignore rule in `ignore_file`.

**To Reproduce**
Steps to reproduce the behavior:
1.
create a folder with many file in the root and add it to the `.amlignore` file
**Expected behavior**

I would expect the function `get_upload_files_from_folder` to run quickly by discarding all the file in the ignored folder without having to enumerate all the files within that folder.
This could partially be done by
1. adding the method `is_folder_excluded` to the `IgnoreFile` classthat adds "/" to norm_file compared to "is_file_excluded"
```
def is_folder_excluded(self, folder_path: Union[str, os.PathLike]) -> bool:
"""Checks if given file_path is excluded.

:param file_path: File path to be checked against ignore file specifications
:type file_path: Union[str, os.PathLike]
:return: Whether the file is excluded by ignore file
:rtype: bool
"""
# TODO: current design of ignore file can't distinguish between files and directories of the same name
if self._path_spec is None:
self._path_spec = self._create_pathspec()
if not self._path_spec:
return False
folder_path = self._get_rel_path(folder_path)
if folder_path is None:
return True

norm_file = normalize_file(folder_path)+"/"
matched = False
for pattern in self._path_spec:
if pattern.include is not None:
if pattern.match_file(norm_file) is not None:
matched = pattern.include

return matched
```
2. removing the ignored folders in the loop in `get_upload_files_from_folder` (seems like it is doing the right thing when we modify that list in place)

```
def get_upload_files_from_folder(
path: Union[str, os.PathLike], *, prefix: str = "", ignore_file: IgnoreFile = IgnoreFile()
) -> List[str]:
"""
Returns a list of files to upload from a folder, avoiding ignored directories and files.

Args:
path (Union[str, os.PathLike]): Root folder to traverse.
prefix (str): Prefix to add to file paths.
ignore_file (IgnoreFile): Object to handle ignored files and directories.

Returns:
List[str]: List of files to upload.
"""
path = Path(path)
upload_paths = []

for root, dirs, files in os.walk(path, followlinks=True):
# Prune ignored directories
ignored_dirs = [d for d in dirs if ignore_file.is_folder_excluded(Path(root) / d)]

for ignored_dir in ignored_dirs:
dirs.remove(ignored_dir) # Prevent traversal into ignored directories

upload_paths += list(
traverse_directory(
root,
files,
prefix=Path(prefix).joinpath(Path(root).relative_to(path)).as_posix(),
ignore_file=ignore_file,
)
)
return upload_paths
```
3. replacing `get_directory_size`'s implementation by
```
def get_directory_size(
root: Union[str, os.PathLike], ignore_file: IgnoreFile = IgnoreFile(None)
) -> Tuple[int, Dict[str, int]]:
"""Returns total size of a directory and a dictionary itemizing each sub- path and its size.

If an optional ignore_file argument is provided, then files specified in the ignore file are not included in the
directory size calculation.

:param root: The directory to calculate the size of
:type root: Union[str, os.PathLike]
:param ignore_file: An ignore file that specifies files to ignore when computing the size
:type ignore_file: IgnoreFile
:return: The computed size of the directory, and the sizes of the child paths
:rtype: Tuple[int, Dict[str, int]]
"""
total_size = 0
size_list = {}

list_files = get_upload_files_from_folder(root, ignore_file=ignore_file)
for full_path, _ in list_files:
if not os.path.islink(full_path):
path_size = os.path.getsize(full_path)
else:
# ensure we're counting the size of the linked file
path_size = os.path.getsize(os.path.join(root, os.readlink(convert_windows_path_to_unix(full_path))))
size_list[full_path] = path_size
total_size += path_size

return total_size, size_list
```
putting a breakpoint in `get_upload_files_from_folder` I noticed it is called 3 times when launching a job, which does not seem optimal. We should be able to call it once an reuse the list of files.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.