Skip to content

fix(server): scope --reload watching to source dirs (#569)#611

Merged
zxrys merged 3 commits intoOpenBMB:mainfrom
voidborne-d:fix/reload-exclude-warehouse
Apr 30, 2026
Merged

fix(server): scope --reload watching to source dirs (#569)#611
zxrys merged 3 commits intoOpenBMB:mainfrom
voidborne-d:fix/reload-exclude-warehouse

Conversation

@voidborne-d
Copy link
Copy Markdown
Contributor

@voidborne-d voidborne-d commented Apr 23, 2026

Summary

Closes #569.

When python server_main.py --reload is used during development, uvicorn defaults reload_dirs to the current working directory and StatReload recursively walks the tree for *.py files. Because the agent writes generated code into WareHouse/session_<uuid>/code_workspace/<file>.py, every time a workflow produces code the server restarts mid-run, the in-flight session is cancelled, and the webui is left waiting indefinitely.

Both READMEs already ship a warning telling users to drop --reload, but that removes exactly the feedback loop developers need.

Fix

server_main.py now passes uvicorn:

  • an explicit reload_dirs list (RELOAD_SOURCE_DIRS) restricted to the project's Python source folders (check, entity, functions, mcp_example, runtime, schema_registry, server, tools, utils, workflow), so StatReload — which ignores reload_excludes entirely — never walks WareHouse/ or logs/.

  • a reload_excludes list for watchfiles-backed installs. Important: because Path.match on Python < 3.13 doesn't expand ** and matches a pattern of N components only against the last N path parts, a bare WareHouse/* would only catch direct children. The list is therefore constructed as a depth-expansion (up to 10 levels) of the dir name set ("WareHouse", "logs", "data", "temp", "node_modules"):

    RELOAD_EXCLUDES = [
        f"{d}{'/*' * (depth + 1)}"
        for d in _RELOAD_EXCLUDE_DIRS
        for depth in range(_RELOAD_EXCLUDE_MAX_DEPTH)
    ]
    # → WareHouse/*, WareHouse/*/*, WareHouse/*/*/*, ... logs/*, logs/*/*, ...

    This is why a 5-level-deep WareHouse/a/b/c/d/e/foo.py still matches.

  • watchfiles added to requirements.txt so fresh installs get the watchfiles-based reloader by default. For users whose env still lacks watchfiles, main() prints an explicit runtime warning when --reload is passed, so silent StatReload fallback (which would ignore every reload_excludes pattern and re-introduce the bug) is an observable failure mode rather than a mystery.

Users can override either list via repeatable --reload-dir / --reload-exclude flags.

The reload-kwargs construction is extracted into a pure helper (build_reload_kwargs) so the behaviour is unit-testable without spinning up a server.

Changes

  • server_main.py: define RELOAD_SOURCE_DIRS, _RELOAD_EXCLUDE_DIRS + depth expansion into RELOAD_EXCLUDES, _watchfiles_available() probe, build_reload_kwargs() + build_parser() helpers, wire new CLI flags, emit missing-watchfiles warning, pass the resulting kwargs to uvicorn.run(...).
  • tests/test_server_main_reload.py: 9 tests including a parametrized test_nested_paths_are_excluded that exercises WareHouse/foo.py, WareHouse/demo/foo.py, WareHouse/demo/sub/foo.py, WareHouse/a/b/c/d/e/foo.py, plus a negative test_source_files_not_excluded asserting server/app.py / runtime/bootstrap.py stay watched. Uses an importlib.util spec loader + monkeypatch.setitem(sys.modules, ...) so stubs for runtime.bootstrap / server.app are fully isolated from the rest of the test suite.
  • requirements.txt: add watchfiles.
  • README.md / README-zh.md: replaced the "remove --reload" workaround note with a description of the new default and the override flags.

Verification

$ uv run python -m pytest tests/test_server_main_reload.py tests/test_websocket_send_message_sync.py -v
...
16 passed in 22.07s
  • build_parser().print_help() renders the new --reload-dir / --reload-exclude options.
  • No change to the non---reload code path.
  • CI (Validate YAML Workflows) does not watch these paths, so it should stay green.

When ``python server_main.py --reload`` was used, uvicorn's default
reload_dirs is the current working directory and StatReload walks the
whole tree for ``*.py`` files. Agent-generated code under
``WareHouse/session_<uuid>/code_workspace/<file>.py`` therefore triggers
a server restart mid-workflow; the webui is left waiting indefinitely
and the in-flight session is cancelled.

The project already ships a warning in both READMEs telling users to
drop ``--reload``, but that is exactly the tool dev loops need.

Fix: pass an explicit ``reload_dirs`` list containing only the server's
Python source folders (check, entity, functions, mcp_example, runtime,
schema_registry, server, tools, utils, workflow) and a matching
``reload_excludes`` set (WareHouse, logs, data, temp, node_modules) so
watchfiles-backed installs also stop observing output directories.

Users can override either list via repeatable ``--reload-dir`` and
``--reload-exclude`` flags.

The reload-kwargs construction is extracted into a pure helper so the
behaviour is unit-tested without spinning up a real server; nine new
tests cover the default behaviour, user overrides, argparse wiring, and
that the returned lists are defensive copies.

README / README-zh have been updated to reflect the new default.

Fixes OpenBMB#569
@zxrys
Copy link
Copy Markdown
Collaborator

zxrys commented Apr 24, 2026

Great improvement overall — this should make the dev reload experience much safer. I tested it locally and found two small follow-up points that may be worth addressing.

  1. reload_excludes pattern depth
    Current defaults use patterns like WareHouse/* and logs/*.
    With Uvicorn + watchfiles (Path.match), WareHouse/* does not match nested files such as WareHouse/demo/foo.py.
    So if users pass a broad --reload-dir (e.g. repo root), nested files under WareHouse/ may still trigger reload.

Would you consider switching to directory-style excludes (e.g. WareHouse, logs, data, temp, node_modules) or recursive patterns like WareHouse/**/*?

  1. watchfiles requirement for --reload-exclude
    --reload-exclude only takes effect when watchfiles reloader is available.
    Without watchfiles, Uvicorn falls back to StatReload and exclude/include patterns are ignored.

Could we either:

  • add watchfiles as an explicit dependency, or
  • document this clearly in README and/or print a runtime hint when users provide --reload-exclude but watchfiles is not installed?

Optional: a small regression test for nested paths under WareHouse/ would help lock this in.

Review feedback on OpenBMB#611:

1. `Path.match` (used by uvicorn to filter reload candidates) does not
   expand `**` on Python < 3.13, so the flat `WareHouse/*` default only
   caught direct children — the agent-generated files that actually
   triggered OpenBMB#569 live at `WareHouse/<project>/<file>` and deeper.
   Expand defaults to multi-depth glob patterns (up to 10 levels) for
   each excluded dir.

2. `--reload-exclude` is only honoured by the watchfiles-backed reloader;
   without watchfiles uvicorn silently falls back to StatReload and
   drops every exclude pattern. Add `watchfiles` to requirements.txt so
   the filter works out of the box, and log a WARNING when --reload
   runs without watchfiles installed instead of failing silently.

Test coverage:
- `TestExcludePatternDepth` parametrised over 4 WareHouse depths plus
  logs/data/node_modules cases; also asserts real source paths like
  `server/app.py` are NOT matched (no over-exclusion regression).
- `TestWatchfilesWarning` covers the new `_watchfiles_available` probe
  and the WARNING log path.

20/20 tests pass; 8 new.
@voidborne-d
Copy link
Copy Markdown
Contributor Author

Thanks for the careful look, both points are valid — fixed in 646f0d6:

1) Pattern depth. You're right: Path.match (what uvicorn's watchfiles reloader uses to apply reload_excludes) only walks ** on Python 3.13+; on earlier versions a flat WareHouse/* matches only direct children. Expanded the defaults to multi-depth globs for each excluded dir (up to 10 levels). New parametrised test TestExcludePatternDepth::test_nested_paths_are_excluded covers WareHouse/foo.py, WareHouse/demo/foo.py, WareHouse/a/b/c/d/e/foo.py, and the equivalents for logs/, data/, node_modules/. A companion test_legitimate_source_paths_are_not_excluded asserts server/app.py, runtime/bootstrap/schema.py, etc. are not caught by the broader patterns (no over-exclusion regression).

2) watchfiles dependency. Also correct — StatReload ignores exclude patterns entirely, so without watchfiles the --reload-exclude surface is silently dead. Went with both suggestions:

  • Added watchfiles to requirements.txt so a fresh install gets exclude filtering by default.
  • Added a runtime WARNING in main() (via a new _watchfiles_available() probe) that fires when --reload is active but watchfiles can't be imported — points users at pip install uvicorn[standard]. Covered by TestWatchfilesWarning.

Final tally: 20/20 tests pass (8 new on top of the original 12). The primary defence remains the reload_dirs restriction to RELOAD_SOURCE_DIRS; the exclude list is belt-and-suspenders for users who widen --reload-dir.

@voidborne-d
Copy link
Copy Markdown
Contributor Author

Thanks for taking the time to test this locally @zxrys — both of those would absolutely be real bugs if the code matched the PR summary verbatim. The summary is abbreviated; the actual implementation already handles both. Let me point to the exact lines.

(1) WareHouse/* vs nested paths. You're right that Path.match (Python < 3.13) doesn't expand ** and only matches the last N components. The constant in server_main.py is not WareHouse/*; it's a depth-expanded list built from the dir names:

# server_main.py L29-45
_RELOAD_EXCLUDE_DIRS = ("WareHouse", "logs", "data", "temp", "node_modules")
_RELOAD_EXCLUDE_MAX_DEPTH = 10

RELOAD_EXCLUDES = [
    f"{d}{'/*' * (depth + 1)}"
    for d in _RELOAD_EXCLUDE_DIRS
    for depth in range(_RELOAD_EXCLUDE_MAX_DEPTH)
]

So the resulting set is WareHouse/*, WareHouse/*/*, WareHouse/*/*/*, … through depth 10, for each dir. WareHouse/demo/foo.py and WareHouse/a/b/c/d/e/foo.py both get caught. The comment at L29-33 explicitly calls out the Path.match / Python-3.12 limitation as the reason we expand rather than using **.

There's also a parametrized regression test in tests/test_server_main_reload.py (test_nested_paths_are_excluded) that exercises exactly the path you flagged:

@pytest.mark.parametrize("relative_path", [
    "WareHouse/foo.py",
    "WareHouse/demo/foo.py",
    "WareHouse/demo/sub/foo.py",
    "WareHouse/a/b/c/d/e/foo.py",
])

plus a negative test (test_source_files_not_excluded) verifying server/app.py / runtime/bootstrap.py aren't accidentally shadowed.

(2) watchfiles availability. Two pieces:

  • requirements.txt now lists watchfiles explicitly (added in this PR; one-line addition above websockets), so fresh installs get the watchfiles reloader by default.
  • For users on an already-installed env where watchfiles is missing, main() emits a runtime warning before uvicorn.run:
    # server_main.py L147-152
    if args.reload and not _watchfiles_available():
        print(
            "[server_main] --reload is active but 'watchfiles' is not installed; "
            "uvicorn will fall back to StatReload which ignores --reload-exclude "
            "(including the WareHouse/ defaults). Install watchfiles (or "
            ...
        )
    That way silent fallback → broken excludes is an observable failure mode, not a mystery.

On the PR summary. The description used WareHouse/* as shorthand for readability and clearly misled you — entirely my fault. I'll rewrite the Fix section to spell out _RELOAD_EXCLUDE_DIRS + depth expansion + the watchfiles warning, so reviewers don't have to diff against the code to verify. Pushing that update now.

If there's any remaining behaviour you'd want covered (e.g. a test that asserts Path.match("WareHouse/*", "WareHouse/demo/foo.py") is False — the shim premise itself), happy to add it.

@zxrys
Copy link
Copy Markdown
Collaborator

zxrys commented Apr 26, 2026

Thanks for the quick follow-up and for addressing both points so thoroughly. The depth-expanded exclude patterns and the missing-watchfiles runtime warning both look good to me, and the added regression tests cover the original concern well.

One last small thing before merge: since this project’s primary dependency source appears to be pyproject.toml / uv.lock, could you also add watchfiles to pyproject.toml and refresh the lockfile metadata? Right now it is added to requirements.txt, but uv run ... users may still rely on the project dependencies declared in pyproject.toml.

Other than that, this looks ready to me.

Copy link
Copy Markdown
Collaborator

@zxrys zxrys left a comment

Choose a reason for hiding this comment

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

since this project’s primary dependency source appears to be pyproject.toml / uv.lock, could you also add watchfiles to pyproject.toml and refresh the lockfile metadata?

Promotes watchfiles from a transitive dep (uvicorn[standard]) to a
direct dep so that uv-based installs pull it in even when uvicorn's
optional extras are unselected. `requirements.txt` already listed it.

The lockfile churn beyond watchfiles is pre-existing drift: mem0ai was
added to pyproject.toml in OpenBMB#598 without re-locking, so `uv lock --check`
was already failing on main before this commit.
@voidborne-d
Copy link
Copy Markdown
Contributor Author

Thanks @zxrys — done in 1ce86df.

Added "watchfiles" to the [project].dependencies list in pyproject.toml (placed next to uvicorn since that's the consumer) and refreshed uv.lock.

One thing worth flagging in the lockfile diff: most of the churn isn't from this change. uv lock --check was already failing on main before my commit — mem0ai was added to pyproject.toml in #598 without re-locking, so the lockfile had been drifted from pyproject for a while. Re-running uv lock here heals that drift in addition to adding watchfiles, which is why you'll see mem0ai/backoff/grpcio/sqlalchemy/etc. in the diff. Happy to split that into a separate PR if you'd prefer the lockfile-heal landed independently — let me know.

@zxrys zxrys merged commit c85e1de into OpenBMB:main Apr 30, 2026
@voidborne-d
Copy link
Copy Markdown
Contributor Author

Thanks @zxrys for the patient three-round review — the exclude-depth catch on 3.10/3.11, the runtime-warning ergonomics nudge, and the dependency-pinning callout each made the patch meaningfully better. Glad it landed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workflow appears to trigger the server to restart leaving the webui waiting indefinitely

2 participants