Skip to content

refactor(processor): replace filepath.Glob with doublestar.Glob for enhanced pattern matching - #99

Open
BraveY wants to merge 2 commits into
modelpack:mainfrom
BraveY:feat-global-pattern
Open

BraveY wants to merge 2 commits into
modelpack:mainfrom
BraveY:feat-global-pattern

Conversation

@BraveY

@BraveY BraveY commented Mar 3, 2025

Copy link
Copy Markdown
Contributor
  • Replaced filepath.Glob with doublestar.Glob to support advanced glob patterns (e.g., **/*.py for recursive matching).

@BraveY
BraveY force-pushed the feat-global-pattern branch from 56e79e1 to 927a10c Compare March 3, 2025 12:36
gaius-qi
gaius-qi previously approved these changes Mar 3, 2025

@gaius-qi gaius-qi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

BraveY added 2 commits March 4, 2025 10:08
…nhanced pattern matching

- Replaced `filepath.Glob` with `doublestar.Glob` to support advanced glob patterns (e.g., `**/*.py` for recursive matching).

Signed-off-by: Yang Kaiyong <yangkaiyong.yky@antgroup.com>
…delfile

Added a detailed explanation of the unix-like glob path pattern.

Signed-off-by: Yang Kaiyong <yangkaiyong.yky@antgroup.com>
@BraveY
BraveY force-pushed the feat-global-pattern branch from decb6f8 to ea5b98e Compare March 4, 2025 02:09
@chlins

chlins commented Mar 4, 2025

Copy link
Copy Markdown
Member

Currently, the glob path in the standard library is based on the standard implementation, and most language implementations are relatively uniform. While doublestar introduces convenience, it also introduces additional interpretation costs for users. So let's hold off for now and merge when there are similar requirements.

@chlins chlins added the hold label Jun 25, 2025

@aftersnow aftersnow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status check on the current head (ea5b98e) against today's main. Two facts have changed since the "hold off" decision in March 2025:

1. doublestar is already a dependency of main. go.mod has github.com/bmatcuk/doublestar/v4 v4.10.0, pulled in by #473, and modelfile generate --include/--exclude match with doublestar semantics. The "extra interpretation cost for users" argument now cuts the other way: the two commands disagree on what a pattern means.

2. build silently drops ** patterns today. I built this Modelfile on main (f241cec):

NAME ws99
MODEL w.safetensors
CODE src/**/*.py

with src/y.py and src/a/b/x.py present. The build succeeded and only the MODEL layer was produced; filepath.Glob treats ** as *, matched nothing, and processor/base.go does not warn when a wildcard pattern matches zero files. A user who writes ** because generate --include accepts it loses files without any error.

So I think this change is worth reviving, with three adjustments:

  • Rebase onto main. The module path is now github.com/modelpack/modctl, go.mod/go.sum and pkg/backend/processor/base.go all conflict, and base.go now has a literal-path branch (absolute paths, "file specified in Modelfile does not exist") that must stay as is; only the wildcard branch should switch to doublestar.Glob(os.DirFS(absWorkDir), pattern).
  • Fail or warn when a wildcard pattern matches nothing. That is the actual footgun above, independent of which glob library is used.
  • Add a test with a ** pattern in pkg/backend/processor.

cc @chlins since the earlier decision was yours.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants