Skip to content

fix(resource): apply path filters during local tree staging - #4604

Open
ktz03 wants to merge 5 commits into
volcengine:mainfrom
ktz03:fix/stage-source-apply-path-filters
Open

ktz03 wants to merge 5 commits into
volcengine:mainfrom
ktz03:fix/stage-source-apply-path-filters

Conversation

@ktz03

@ktz03 ktz03 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Pass ignore_dirs / include / exclude into stage_source()
  • Apply the same directory-scan / upload filter semantics in _copy_local_tree (including gitignore pruning) so watch refresh does not copy excluded trees into viking://temp
  • Add a unit test covering ignore_dirs + include + exclude

Why

Server-side watch refresh staged the full local tree before filters ran, which could copy tens of GB of excluded data into temp storage (#4570).

Test plan

  • pytest tests/resource/test_staged_source_filters.py
  • Manual: watch a tree with --ignore-dirs large-data --include '*.md' and confirm excluded files never appear under viking://temp/...

Fixes #4570

Honor ignore_dirs/include/exclude (and gitignore) in _copy_local_tree so watch refresh staging matches upload filtering and does not copy excluded trees into viking://temp.

Fixes volcengine#4570
@now-ing

now-ing commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Verified the wiring: the filter parameters land on the stage_source call inside _prepare_standard_source_plan, so both callers (the watch-refresh branch and the first-upload path at resource_service.py:1649) inherit them when their processor_kwargs carry the keys — good.

Reusing the directory_scan helpers (_should_skip_directory / _matches_include / _matches_exclude) plus GitignoreMatcher is the right call for keeping staging and scan semantics aligned. Two suggestions:

  1. Add one end-to-end assertion for the kwargs plumbing. The unit test exercises _copy_local_tree directly, but the bug class this fix guards against is "the keys don't reach staging" — e.g. a watch task whose processor_kwargs field names drift, making the filters silently no-op again (exactly the [Bug]: Watched local directories stage excluded files before applying filters #4570 symptom, just harder to see). A test that builds a watch-refresh-shaped kwargs dict and asserts the staged tree contains only the filtered files would pin the contract at the seam where it actually broke.

  2. Empty directories still get created. After filtering, a directory whose entire contents were excluded still gets mkdir-ed into staging (directories are registered during the walk before file-level filters run). Harmless functionally, but for the 57 GB scenario the staged tree will contain a skeleton of empty dirs from excluded subtrees — pruning directories that end up with no files (or checking _matches_* before adding) would make the staged tree match what users expect "excluded" to mean.

Nice, focused fix for a genuinely painful deployment pattern.

@ktz03

ktz03 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @now-ing — follow-up on this head:

  1. Staging no longer mkdirs every walked directory; only the staging root + parents of retained files are created, so ignored subtrees do not leave empty dir skeletons.
  2. Added test_copy_local_tree_does_not_mkdir_empty_excluded_dirs to pin that.

The unit test still covers _copy_local_tree filters directly; the empty-dir assertion is the seam that matches the 57GB excluded-subtree concern you flagged.

Add stage_source-level coverage for watch-shaped ignore_dirs/include/exclude kwargs (volcengine#4604 review nit).
@ktz03

ktz03 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @now-ing — follow-up on the remaining kwargs-plumbing nit:

Added est_stage_source_forwards_watch_shaped_filter_kwargs which builds watch-shaped processor_kwargs (ignore_dirs / include / �xclude) and calls stage_source(...) with those fields, asserting only the filtered files land in the staged tree.

Together with the earlier empty-excluded-dir coverage, that pins both review suggestions on this head.

@ktz03

ktz03 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @now-ing — remaining review nit on this head:

Added est_stage_source_forwards_watch_shaped_filter_kwargs, which builds watch/processor-shaped ignore_dirs / include / �xclude kwargs and asserts stage_source(...) only stages the filtered files. That pins the seam where #4570 broke (filters not reaching staging), on top of the earlier empty-excluded-dir coverage.

Happy to extend further if you want the assertion to go through _prepare_standard_source_plan with a fuller ResourceService mock.

@now-ing

now-ing commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Thanks — verified on 48a8fea5: test_stage_source_forwards_watch_shaped_filter_kwargs builds watch-shaped processor_kwargs (ignore_dirs/include/exclude as string fields) and drives the real stage_source(...) entry, asserting only the filtered files land in staging — that pins exactly the seam where the #4570 class of "keys silently don't reach staging" regressions would show up. The empty-dir-skeleton fix has its own regression test too, and all 3 tests in tests/resource/test_staged_source_filters.py pass locally on this head. That resolves my point. LGTM from my side.

@ktz03

ktz03 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Friendly ping for maintainer review when convenient.

Happy to address any further feedback.

@FOWEPJF255

Copy link
Copy Markdown

Friendly bump on path-filter application during local tree staging (ignore_dirs / include / exclude + gitignore pruning).

Still interested in landing this so watch refresh does not copy excluded paths 鈥?let me know if anything else is needed from review.

@ktz03

ktz03 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Synced with latest main; ready for review when convenient.

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

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

[Bug]: Watched local directories stage excluded files before applying filters

3 participants