refactor(framework): Extract project file utility for flwr build #6822
refactor(framework): Extract project file utility for flwr build #6822chongshenng wants to merge 37 commits intomainfrom
flwr build #6822Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…nclude-based-fab-build
…ent-related files
(cherry picked from commit d029d6d)
…ridge-fab-build-core
…ridge-fab-build-core
…nclude-based-fab-build
chongshenng
changed the title
flwr build
github-actions
bot
added
the
Maintainer
chongshenng
requested review from
danieljanes,
jafermarq,
panh99 and
tanertopal
as code owners
There was a problem hiding this comment.
Pull request overview
This PR refactors Flower CLI file-collection logic by extracting reusable project file utilities from flwr app publish into flwr.cli.utils, with the intent to reuse the same logic in flwr build.
Changes:
- Added
depth_ofand a newcollect_project_filesutility toframework/py/flwr/cli/utils.py. - Updated
flwr app publishto usecollect_project_filesinstead of its local helpers. - Moved
depth_ofunit test coverage frompublish_test.pytoutils_test.py.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| framework/py/flwr/cli/utils.py | Introduces shared helpers depth_of and collect_project_files (include/exclude filtering, max depth enforcement, deterministic sorting). |
| framework/py/flwr/cli/utils_test.py | Adds depth_of test coverage in the CLI utils test module. |
| framework/py/flwr/cli/app_cmd/publish.py | Replaces local file-walking/depth logic with the new shared collect_project_files helper. |
| framework/py/flwr/cli/app_cmd/publish_test.py | Removes the old _depth_of tests now that the helper is centralized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Build include/exclude pathspecs | ||
| # Note: This should be a temporary solution until we have a complete mechanism | ||
| # for configurable inclusion and exclusion rules. | ||
| # Note: Unlike Git, we do not support nested .gitignore files in subdirectories. | ||
| gitignore_patterns = tuple(load_gitignore_patterns(root / ".gitignore")) | ||
| exclude_spec = build_pathspec(gitignore_patterns + exclude_patterns) | ||
| include_spec = build_pathspec(include_patterns) | ||
|
|
There was a problem hiding this comment.
collect_project_files always merges patterns from root/.gitignore into exclude_patterns. The PR description says this utility is extracted to reuse in flwr build, but (per linked #6805) flwr build is moving away from .gitignore-based filtering. Consider parameterizing gitignore handling (e.g., gitignore_patterns: Sequence[str] | None or use_gitignore: bool = True) so callers like build can explicitly disable it while publish keeps current behavior.
| for path in root.rglob("*"): | ||
| if not path.is_file(): | ||
| continue | ||
|
|
||
| # Skip excluded or not included files | ||
| # Note: pathspec requires POSIX style relative paths | ||
| relative_path = path.relative_to(root) | ||
| posix = relative_path.as_posix() |
There was a problem hiding this comment.
path.is_file() returns true for symlinks to files, so collect_project_files can include symlink targets outside root (potentially uploading/packaging unintended files). Consider skipping symlinks (e.g., if path.is_symlink(): continue) or enforcing that path.resolve() stays within root.resolve() before including/reading files.
| def collect_project_files( | ||
| root: Path, | ||
| include_patterns: tuple[str, ...], | ||
| exclude_patterns: tuple[str, ...], | ||
| max_depth: int | None = None, | ||
| on_skip: Callable[[Path], None] | None = None, | ||
| ) -> list[Path]: | ||
| """Walk a project directory and return filtered file paths. | ||
|
|
||
| Files are included or excluded based on gitignore-style patterns. | ||
| Patterns from the project's ``.gitignore`` are merged with the | ||
| caller-supplied ``exclude_patterns`` before matching. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| root : Path | ||
| Absolute path to the project root directory. | ||
| include_patterns : tuple[str, ...] | ||
| Gitignore-style patterns for files to include. A file must | ||
| match at least one include pattern to be accepted. | ||
| exclude_patterns : tuple[str, ...] |
There was a problem hiding this comment.
collect_project_files is newly introduced as a shared utility, but there are no direct unit tests covering its include/exclude behavior, deterministic sorting, or on_skip callback semantics (current coverage is only indirect via publish tests). Adding focused tests in cli/utils_test.py would help lock down behavior as it gets reused by multiple commands.
This PR extracts the common functions from
flwr app publishto reuse inflwr build.Merge after