feat(cli): add server workers option#1851
Conversation
Documentation preview |
Greptile SummaryThis PR adds a
|
| Filename | Overview |
|---|---|
| nemoguardrails/cli/init.py | Adds --workers option; propagates config via env vars before worker spawn; guards against incompatible --prefix/--auto-reload combinations, but those guards come after env/app state has already been mutated. |
| nemoguardrails/server/api.py | Adds module-level env-var reads for NEMO_GUARDRAILS_CONFIG_PATH, AUTO_RELOAD, DEFAULT_CONFIG_ID, and VERBOSE so worker processes pick up config at import time; adds create_app factory returning the module-level singleton. |
| tests/cli/test_cli_main.py | New tests for workers factory path, reject cases, and verbose env var — all correctly isolated with @patch.dict/getcwd/exists mocks; the two pre-existing tests (test_server_with_disable_chat_ui, test_server_with_auto_reload) remain unpatched and will fail when no config/ dir exists in cwd, as flagged in the previous review. |
| docs/reference/cli/index.md | Documents the new --workers option with default value; entry is accurate and placed in correct table position. |
Sequence Diagram
sequenceDiagram
participant CLI as nemoguardrails server CLI
participant Env as os.environ
participant Uvicorn as uvicorn.run
participant Worker as Worker Process
participant ApiPy as nemoguardrails.server.api
CLI->>Env: Set NEMO_GUARDRAILS_CONFIG_PATH
CLI->>Env: Set NEMO_GUARDRAILS_VERBOSE (if --verbose)
CLI->>Env: Set NEMO_GUARDRAILS_DISABLE_CHAT_UI (if --disable-chat-ui)
CLI->>Env: Set NEMO_GUARDRAILS_AUTO_RELOAD (if --auto-reload)
CLI->>Env: Set NEMO_GUARDRAILS_DEFAULT_CONFIG_ID (if --default-config-id)
alt "workers == 1"
CLI->>Uvicorn: run(server_app, port, ...)
Uvicorn->>ApiPy: already imported in parent process
else "workers > 1"
CLI->>CLI: Guard: reject --prefix or --auto-reload
CLI->>Uvicorn: "run(api:create_app, factory=True, workers=N)"
loop Each worker process
Uvicorn->>Worker: spawn/fork
Worker->>ApiPy: import reads env vars at module level
ApiPy-->>Worker: app with rails_config_path auto_reload etc
Worker->>ApiPy: create_app returns module-level app singleton
end
end
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
nemoguardrails/cli/__init__.py:213-235
**Incompatibility guards fire after state has already been mutated**
The checks for `--prefix` and `--auto-reload` against `workers > 1` are evaluated only after env vars (`NEMO_GUARDRAILS_CONFIG_PATH`, `NEMO_GUARDRAILS_DEFAULT_CONFIG_ID`, etc.) have been written, `FastAPI().mount()` has been called, and `set_default_config_id` has executed. When the command exits with code 1, this state is already in place. Moving the guards to just before the prefix-mount block would keep the failure path clean and prevent any unnecessary side effects from running before the error is reported.
Reviews (8): Last reviewed commit: "test(server): isolate verbose worker env..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| api.set_default_config_id(default_config_id) # Call function | ||
|
|
||
| uvicorn.run(server_app, port=port, log_level="info", host="0.0.0.0") | ||
| uvicorn.run(server_app, port=port, log_level="info", host="0.0.0.0", workers=workers) |
There was a problem hiding this comment.
workers > 1 silently exits when app is passed as an instance
Uvicorn requires an import string (e.g. "nemoguardrails.server.api:app") — not an application object — when workers is set to anything greater than 1. Passing the live server_app object with workers=2 causes uvicorn to print "You must pass the application as an import string to enable 'reload' or 'workers'" and then exit with SystemError 1. This means every invocation of --workers N where N > 1 is silently broken.
The fix requires either (a) passing the app as an import string when workers > 1 (which complicates the current per-process attribute-patching approach used for rails_config_path, disable_chat_ui, etc.), or (b) raising an explicit error when workers > 1 with a clear message about this constraint until a proper solution is implemented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/cli/__init__.py
Line: 204
Comment:
**`workers > 1` silently exits when app is passed as an instance**
Uvicorn requires an import string (e.g. `"nemoguardrails.server.api:app"`) — not an application object — when `workers` is set to anything greater than 1. Passing the live `server_app` object with `workers=2` causes uvicorn to print `"You must pass the application as an import string to enable 'reload' or 'workers'"` and then exit with `SystemError 1`. This means every invocation of `--workers N` where N > 1 is silently broken.
The fix requires either (a) passing the app as an import string when `workers > 1` (which complicates the current per-process attribute-patching approach used for `rails_config_path`, `disable_chat_ui`, etc.), or (b) raising an explicit error when `workers > 1` with a clear message about this constraint until a proper solution is implemented.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in eb00418. --workers > 1 no longer passes a live app instance to uvicorn.
The server now calls uvicorn.run("nemoguardrails.server.api:create_app", factory=True, workers=workers). create_app is a new factory in api.py that reads the same env vars the parent process sets (NEMO_GUARDRAILS_CONFIG_PATH, NEMO_GUARDRAILS_DISABLE_CHAT_UI, NEMO_GUARDRAILS_DEFAULT_CONFIG_ID, the auto-reload flag) and rebuilds the app from scratch in each worker, so the previously-relied-on parent-process attribute patches (api.app.rails_config_path = ... etc.) are no longer needed in worker mode.
The single-worker path is unchanged and still uses the live server_app so --prefix (which mounts the app at a sub-path) keeps working as before. workers > 1 rejects --prefix explicitly with a clear error (commit 1a85158).
📝 WalkthroughWalkthroughA ChangesMulti-Worker Server Support
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoguardrails/cli/__init__.py`:
- Line 204: The current call uvicorn.run(server_app, ...) passes a live app
instance so --workers>1 fails and the per-process mutations (rails_config_path,
disable_chat_ui, auto_reload, default_config_id set between lines 173-202) don’t
propagate to workers; change to run Uvicorn with an import string or factory so
workers spawn correctly and receive configuration: expose a factory like
create_app(...) or export the ASGI app as a module-level name (e.g.,
"nemoguardrails.cli.__init__:server_app" or better
"nemoguardrails.cli.__init__:create_app") and call uvicorn.run with that import
string when workers>1, and move the mutating logic (rails_config_path,
disable_chat_ui, auto_reload, default_config_id) into the app factory or into
startup hooks/environment variables so each worker initializes with the intended
config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f065d2bf-33da-4e7d-9f2d-b9bcf077d8cf
📒 Files selected for processing (2)
docs/reference/cli/index.mdnemoguardrails/cli/__init__.py
| if workers > 1: | ||
| if prefix: | ||
| typer.secho( | ||
| "The --prefix option is not supported with --workers > 1.", | ||
| fg=typer.colors.RED, | ||
| ) | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
--auto-reload and --workers > 1 are combined, each uvicorn worker process inherits NEMO_GUARDRAILS_AUTO_RELOAD=true and independently enters the lifespan's start_auto_reload_monitoring branch, spawning N concurrent file watchers on the same directory. This causes redundant reload signals and potential race conditions. The PR already guards --prefix against workers > 1; the same guard is needed for --auto-reload.
| if workers > 1: | |
| if prefix: | |
| typer.secho( | |
| "The --prefix option is not supported with --workers > 1.", | |
| fg=typer.colors.RED, | |
| ) | |
| raise typer.Exit(1) | |
| if workers > 1: | |
| if prefix: | |
| typer.secho( | |
| "The --prefix option is not supported with --workers > 1.", | |
| fg=typer.colors.RED, | |
| ) | |
| raise typer.Exit(1) | |
| if auto_reload: | |
| typer.secho( | |
| "The --auto-reload option is not supported with --workers > 1.", | |
| fg=typer.colors.RED, | |
| ) | |
| raise typer.Exit(1) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/cli/__init__.py
Line: 210-216
Comment:
When `--auto-reload` and `--workers > 1` are combined, each uvicorn worker process inherits `NEMO_GUARDRAILS_AUTO_RELOAD=true` and independently enters the lifespan's `start_auto_reload_monitoring` branch, spawning N concurrent file watchers on the same directory. This causes redundant reload signals and potential race conditions. The PR already guards `--prefix` against `workers > 1`; the same guard is needed for `--auto-reload`.
```suggestion
if workers > 1:
if prefix:
typer.secho(
"The --prefix option is not supported with --workers > 1.",
fg=typer.colors.RED,
)
raise typer.Exit(1)
if auto_reload:
typer.secho(
"The --auto-reload option is not supported with --workers > 1.",
fg=typer.colors.RED,
)
raise typer.Exit(1)
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 0932b17.
Added the same explicit guard pattern that already existed for --prefix. --auto-reload combined with --workers > 1 now exits 1 with "The --auto-reload option is not supported with --workers > 1." before uvicorn is invoked, so the N-concurrent-file-watchers race never happens. The guard is paired with a unit test in tests/cli/test_cli_main.py that asserts the reject path and confirms uvicorn.run is not called.
| @patch("uvicorn.run") | ||
| @patch("nemoguardrails.server.api.app") | ||
| def test_server_rejects_prefix_with_workers(self, mock_app, mock_uvicorn): | ||
| result = runner.invoke(app, ["server", "--workers=2", "--prefix=/api/v1"]) | ||
|
|
||
| assert result.exit_code == 1 | ||
| assert "The --prefix option is not supported with --workers > 1." in result.output | ||
| mock_uvicorn.assert_not_called() | ||
|
|
||
| @patch("uvicorn.run") | ||
| @patch("nemoguardrails.server.api.app") | ||
| def test_server_rejects_auto_reload_with_workers(self, mock_app, mock_uvicorn): | ||
| result = runner.invoke(app, ["server", "--workers=2", "--auto-reload"]) | ||
|
|
||
| assert result.exit_code == 1 | ||
| assert "The --auto-reload option is not supported with --workers > 1." in result.output | ||
| mock_uvicorn.assert_not_called() |
There was a problem hiding this comment.
os.getcwd() / os.path.exists() and don't guard the environment with @patch.dict. When there is no config/ directory in the working directory, rails_config_path falls back to api.app.rails_config_path (a MagicMock), which means os.fspath returns "" and NEMO_GUARDRAILS_CONFIG_PATH is set to an empty string in the real os.environ. That leaked value persists into subsequent tests in the same process and could silently affect any test that reads NEMO_GUARDRAILS_CONFIG_PATH without clearing it first. Adding the same isolation decoration used by the other workers test fixes both problems.
| @patch("uvicorn.run") | |
| @patch("nemoguardrails.server.api.app") | |
| def test_server_rejects_prefix_with_workers(self, mock_app, mock_uvicorn): | |
| result = runner.invoke(app, ["server", "--workers=2", "--prefix=/api/v1"]) | |
| assert result.exit_code == 1 | |
| assert "The --prefix option is not supported with --workers > 1." in result.output | |
| mock_uvicorn.assert_not_called() | |
| @patch("uvicorn.run") | |
| @patch("nemoguardrails.server.api.app") | |
| def test_server_rejects_auto_reload_with_workers(self, mock_app, mock_uvicorn): | |
| result = runner.invoke(app, ["server", "--workers=2", "--auto-reload"]) | |
| assert result.exit_code == 1 | |
| assert "The --auto-reload option is not supported with --workers > 1." in result.output | |
| mock_uvicorn.assert_not_called() | |
| @patch.dict(os.environ, {}, clear=True) | |
| @patch("uvicorn.run") | |
| @patch("nemoguardrails.server.api.app") | |
| @patch("os.path.exists") | |
| @patch("os.getcwd") | |
| def test_server_rejects_prefix_with_workers( | |
| self, mock_getcwd, mock_exists, mock_app, mock_uvicorn | |
| ): | |
| mock_getcwd.return_value = "/current/dir" | |
| mock_exists.return_value = True | |
| result = runner.invoke(app, ["server", "--workers=2", "--prefix=/api/v1"]) | |
| assert result.exit_code == 1 | |
| assert "The --prefix option is not supported with --workers > 1." in result.output | |
| mock_uvicorn.assert_not_called() | |
| @patch.dict(os.environ, {}, clear=True) | |
| @patch("uvicorn.run") | |
| @patch("nemoguardrails.server.api.app") | |
| @patch("os.path.exists") | |
| @patch("os.getcwd") | |
| def test_server_rejects_auto_reload_with_workers( | |
| self, mock_getcwd, mock_exists, mock_app, mock_uvicorn | |
| ): | |
| mock_getcwd.return_value = "/current/dir" | |
| mock_exists.return_value = True | |
| result = runner.invoke(app, ["server", "--workers=2", "--auto-reload"]) | |
| assert result.exit_code == 1 | |
| assert "The --auto-reload option is not supported with --workers > 1." in result.output | |
| mock_uvicorn.assert_not_called() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/cli/test_cli_main.py
Line: 257-273
Comment:
The two reject tests don't mock `os.getcwd()` / `os.path.exists()` and don't guard the environment with `@patch.dict`. When there is no `config/` directory in the working directory, `rails_config_path` falls back to `api.app.rails_config_path` (a `MagicMock`), which means `os.fspath` returns `""` and `NEMO_GUARDRAILS_CONFIG_PATH` is set to an empty string in the real `os.environ`. That leaked value persists into subsequent tests in the same process and could silently affect any test that reads `NEMO_GUARDRAILS_CONFIG_PATH` without clearing it first. Adding the same isolation decoration used by the other workers test fixes both problems.
```suggestion
@patch.dict(os.environ, {}, clear=True)
@patch("uvicorn.run")
@patch("nemoguardrails.server.api.app")
@patch("os.path.exists")
@patch("os.getcwd")
def test_server_rejects_prefix_with_workers(
self, mock_getcwd, mock_exists, mock_app, mock_uvicorn
):
mock_getcwd.return_value = "/current/dir"
mock_exists.return_value = True
result = runner.invoke(app, ["server", "--workers=2", "--prefix=/api/v1"])
assert result.exit_code == 1
assert "The --prefix option is not supported with --workers > 1." in result.output
mock_uvicorn.assert_not_called()
@patch.dict(os.environ, {}, clear=True)
@patch("uvicorn.run")
@patch("nemoguardrails.server.api.app")
@patch("os.path.exists")
@patch("os.getcwd")
def test_server_rejects_auto_reload_with_workers(
self, mock_getcwd, mock_exists, mock_app, mock_uvicorn
):
mock_getcwd.return_value = "/current/dir"
mock_exists.return_value = True
result = runner.invoke(app, ["server", "--workers=2", "--auto-reload"])
assert result.exit_code == 1
assert "The --auto-reload option is not supported with --workers > 1." in result.output
mock_uvicorn.assert_not_called()
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in b7520be.
Both test_server_rejects_prefix_with_workers and test_server_rejects_auto_reload_with_workers now use the same @patch.dict(os.environ, {}, clear=True) plus @patch("os.getcwd") / @patch("os.path.exists") decoration suggested above. mock_getcwd.return_value = "/current/dir" and mock_exists.return_value = True so the CLI takes the local_configs_path branch deterministically and never falls through to the MagicMock-leakage path. The env var leak into subsequent tests is gone.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| @patch.dict(os.environ, {}, clear=True) | ||
| @patch("uvicorn.run") | ||
| @patch("nemoguardrails.server.api.app") | ||
| def test_server_with_verbose_sets_worker_env(self, mock_app, mock_uvicorn): | ||
| result = runner.invoke(app, ["server", "--verbose"]) | ||
| assert result.exit_code == 0 | ||
| assert os.environ["NEMO_GUARDRAILS_VERBOSE"] == "true" |
There was a problem hiding this comment.
os.getcwd / os.path.exists mocks — test fails in CI
When neither --config is passed nor a config/ directory exists in the cwd, the CLI falls back to rails_config_path = api.app.rails_config_path (the patched MagicMock). The subsequent os.fspath(mock_app.rails_config_path) call requires __fspath__() to return a str or bytes; a MagicMock auto-attribute satisfies neither, so os.fspath raises TypeError. The repo root has no config/ directory, so os.path.exists returns False and the error path is always hit. result.exit_code will be 1 and the assert result.exit_code == 0 will fail. The three newly-added worker/reject tests correctly use @patch("os.getcwd") + @patch("os.path.exists") for exactly this reason — the same fix is needed here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/cli/test_cli_main.py
Line: 209-215
Comment:
**Missing `os.getcwd` / `os.path.exists` mocks — test fails in CI**
When neither `--config` is passed nor a `config/` directory exists in the cwd, the CLI falls back to `rails_config_path = api.app.rails_config_path` (the patched MagicMock). The subsequent `os.fspath(mock_app.rails_config_path)` call requires `__fspath__()` to return a `str` or `bytes`; a MagicMock auto-attribute satisfies neither, so `os.fspath` raises `TypeError`. The repo root has no `config/` directory, so `os.path.exists` returns `False` and the error path is always hit. `result.exit_code` will be `1` and the `assert result.exit_code == 0` will fail. The three newly-added worker/reject tests correctly use `@patch("os.getcwd")` + `@patch("os.path.exists")` for exactly this reason — the same fix is needed here.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 8f05fd2.
test_server_with_verbose_sets_env_var (and the related verbose tests) now mock os.getcwd and os.path.exists exactly like the new worker-guard tests do, plus @patch.dict(os.environ, {}, clear=True) to keep NEMO_GUARDRAILS_CONFIG_PATH from leaking into the next test in the process. The TypeError from os.fspath(MagicMock()) is gone because the CLI now takes the local_configs_path branch where mock_exists.return_value = True.
Pouyanpi
force-pushed
the
develop
branch
4 times, most recently
from
a6be550 to
c69efe5
Compare
|
Addressed greptile review comment on the missing |
Add os.getcwd / os.path.exists mocks to test_server_with_verbose_sets_worker_env so it does not rely on MagicMock's __fspath__ fallback or the absence of a config/ directory in the working tree, matching the isolation pattern used by the other worker-related tests. Signed-off-by: Aditya Singh <adisin650@gmail.com>
adityasingh2400
force-pushed
the
feat/server-workers-option
branch
from
2492720 to
8f05fd2
Compare
1 participant
Summary
--workersoption tonemoguardrails server.uvicorn.run.Fixes #1746
Verification
python3 -m compileall nemoguardrails/cli/__init__.pyPR overlap check
1746; no existing open PR covers this issue.Summary by CodeRabbit
--workersCLI option to the server command for specifying the number of Uvicorn worker processes to run (default: 1, minimum: 1), enabling multi-process serving configurations.