feat: add generic --feature-flag CLI arg and --list-feature-flags registry#13685
feat: add generic --feature-flag CLI arg and --list-feature-flags registry#13685Kosinkadink merged 8 commits intomasterfrom
Conversation
…istry Add --feature-flag KEY=VALUE CLI argument that allows setting arbitrary server feature flags at startup. Values are auto-converted to appropriate Python types (bool, int, float, string). CLI flags are merged into SERVER_FEATURE_FLAGS but cannot overwrite core flags. Add --list-feature-flags which prints the registry of known CLI-settable feature flags as JSON and exits, enabling launchers to discover valid flags for a specific ComfyUI version. Part of Comfy-Org/ComfyUI-Desktop-2.0-Beta#415 Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019d9386-54d3-74d9-a661-97e0a8d37b6b
… wrapper Address code review feedback: - _coerce_flag_value: wrap coercion in try/except (ValueError, TypeError) and log a warning instead of crashing startup on malformed values. - _parse_cli_feature_flags: bare --feature-flag KEY (no '=') now defaults to 'true' so registered bool flags work as toggles. - Remove the get_cli_feature_flag_registry() wrapper; export and use CLI_FEATURE_FLAG_REGISTRY directly in main.py and tests. Add tests for coercion-failure fallback and bare-flag default behavior. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019deba2-bfe2-7118-913c-562beee48972
📝 WalkthroughWalkthroughAdds two CLI options: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy/cli_args.py`:
- Line 241: Update the CLI argument declaration for --feature-flag to document
that a bare key (no "=VALUE") is accepted and treated as true: change the
metavar from "KEY=VALUE" to something that shows both forms (e.g. "KEY[=VALUE]"
or "KEY|KEY=VALUE") and revise the help string in the call to
parser.add_argument("--feature-flag", ...) to explicitly mention that supplying
just KEY will set the flag to true, in addition to the existing examples about
boolean and numeric auto-conversion; keep the action='append' and default
behavior unchanged so the parsing code handling bare keys (implicit true)
matches the new help.
🪄 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: Pro
Run ID: a7b7d7c0-18ac-4424-9161-216b14fa30ca
📒 Files selected for processing (1)
comfy/cli_args.py
Per CodeRabbit suggestion, the metavar 'KEY=VALUE' and help text omitted the supported '--feature-flag KEY' (no '=value', implicitly true) form. Update metavar to 'KEY[=VALUE]' and rewrite the help string to mention both forms with examples. Amp-Thread-ID: https://ampcode.com/threads/T-019df26e-96f4-7518-94da-0e4263680e3c Co-authored-by: Amp <amp@ampcode.com>
Per @rattus128 review on PR #13685: silently coercing typo'd bool values (e.g. `--feature-flag show_signin_button=ture`) to `False` was a confusing UX. Make bool coercion strict and drop unparseable flags entirely. - `_coerce_bool`: accept only `true`/`false` (case-insensitive); raise `ValueError` for anything else (`ture`, `yes`, `1`, ``). - `_coerce_flag_value`: no longer swallows exceptions; raises on bad coercion so the caller decides what to do. - `_parse_cli_feature_flags`: catches `ValueError`/`TypeError`, logs a warning ("dropping flag"), and omits the flag from the result. ComfyUI still starts; `SERVER_FEATURE_FLAGS` retains the registered default; other valid `--feature-flag` entries on the same command line are unaffected. Tests: - `test_bool_typo_raises`: `ture`/`yes`/`1`/`""` all raise ValueError. - `test_failed_int_coercion_raises`: replaces the old "falls back to string" test now that coercion failures propagate. - `test_invalid_bool_value_dropped`: parser drops the bad flag and logs a warning, while a valid sibling flag still parses. 19/19 unit tests pass. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019df5a8-36be-7107-a4af-c7e4f51687df
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests-unit/feature_flags_test.py (2)
132-141: ⚡ Quick winAdd coverage for registered-unknown-type fallback behavior.
_coerce_flag_value()intentionally returns raw string when a registered key has an unrecognizedtype. A small test here would prevent accidental behavior changes if registry types evolve.Proposed test addition
class TestCoerceFlagValue: @@ def test_failed_int_coercion_raises(self, monkeypatch): @@ with pytest.raises(ValueError): _coerce_flag_value("test_int_flag", "not_a_number") + + def test_registered_unknown_type_stays_string(self, monkeypatch): + monkeypatch.setitem( + CLI_FEATURE_FLAG_REGISTRY, + "test_unknown_type", + {"type": "custom", "default": "", "description": "test"}, + ) + assert _coerce_flag_value("test_unknown_type", "abc") == "abc"🤖 Prompt for 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. In `@tests-unit/feature_flags_test.py` around lines 132 - 141, Add a unit test to cover the registered-unknown-type fallback in _coerce_flag_value: use monkeypatch.setitem to register a flag key (e.g., "test_unknown_type") in CLI_FEATURE_FLAG_REGISTRY with a type value that _coerce_flag_value does not recognize (e.g., "weird"), then call _coerce_flag_value("test_unknown_type", "some_string") and assert it returns the raw string "some_string" unchanged; place this alongside the existing tests (same file) to prevent regressions when registry types change.
157-160: ⚡ Quick winAdd a whitespace-normalization parse test.
_parse_cli_feature_flags()strips both key and value, but current coverage only checks=valueempty-key shape. Please add a case like" show_signin_button = true "and" =value"to lock in the trimming behavior.Proposed test addition
class TestParseCliFeatureFlags: @@ def test_empty_key_skipped(self, monkeypatch): monkeypatch.setattr("comfy_api.feature_flags.args", type("Args", (), {"feature_flag": ["=value", "valid=1"]})()) result = _parse_cli_feature_flags() assert result == {"valid": "1"} + + def test_whitespace_trimmed_and_blank_key_skipped(self, monkeypatch): + monkeypatch.setattr( + "comfy_api.feature_flags.args", + type("Args", (), {"feature_flag": [" show_signin_button = true ", " =value"]})(), + ) + result = _parse_cli_feature_flags() + assert result == {"show_signin_button": True}🤖 Prompt for 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. In `@tests-unit/feature_flags_test.py` around lines 157 - 160, Update the unit test for CLI feature flag parsing by adding cases that exercise whitespace normalization: in tests-unit/feature_flags_test.py modify or add to test_empty_key_skipped so monkeypatch sets comfy_api.feature_flags.args.feature_flag to include strings like " show_signin_button = true " and " =value" (in addition to the existing "=value" and "valid=1"), then call _parse_cli_feature_flags() and assert the result contains {"show_signin_button":"true","valid":"1"} while empty-key entries are skipped; reference the _parse_cli_feature_flags function and the test_empty_key_skipped test to locate where to change the input and expected output.
🤖 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.
Nitpick comments:
In `@tests-unit/feature_flags_test.py`:
- Around line 132-141: Add a unit test to cover the registered-unknown-type
fallback in _coerce_flag_value: use monkeypatch.setitem to register a flag key
(e.g., "test_unknown_type") in CLI_FEATURE_FLAG_REGISTRY with a type value that
_coerce_flag_value does not recognize (e.g., "weird"), then call
_coerce_flag_value("test_unknown_type", "some_string") and assert it returns the
raw string "some_string" unchanged; place this alongside the existing tests
(same file) to prevent regressions when registry types change.
- Around line 157-160: Update the unit test for CLI feature flag parsing by
adding cases that exercise whitespace normalization: in
tests-unit/feature_flags_test.py modify or add to test_empty_key_skipped so
monkeypatch sets comfy_api.feature_flags.args.feature_flag to include strings
like " show_signin_button = true " and " =value" (in addition to the
existing "=value" and "valid=1"), then call _parse_cli_feature_flags() and
assert the result contains {"show_signin_button":"true","valid":"1"} while
empty-key entries are skipped; reference the _parse_cli_feature_flags function
and the test_empty_key_skipped test to locate where to change the input and
expected output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e94d9cb1-1b91-4f9c-b22b-46cc6a7edb56
📒 Files selected for processing (2)
comfy_api/feature_flags.pytests-unit/feature_flags_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- comfy_api/feature_flags.py
Summary
Adds two new CLI arguments for runtime feature-flag control:
--feature-flag KEY=VALUE(repeatable) — sets a server feature flag at startup. Values are auto-coerced to the registered type (bool/int/float); unregistered keys keep the raw string. Bare--feature-flag KEY(no=) defaults to the valuetrue, so registered bool flags work as toggles.--list-feature-flags— prints the JSON registry of known CLI-settable flags and exits, so launchers (e.g. Desktop 2.0) can discover the valid flag set for a given ComfyUI version.CLI-provided flags are merged into
SERVER_FEATURE_FLAGSbut cannot overwrite core flags (supports_preview_metadata,max_upload_size,extension,node_replacements,assets).The registry currently exposes:
show_signin_buttonfalseExample
python main.py --feature-flag show_signin_button=true python main.py --feature-flag show_signin_button # bare form, equivalent to =true python main.py --list-feature-flagsRobustness
--feature-flag some_int=not_a_number) log a warning and fall back to the raw string instead of crashing startup.Tests
tests-unit/feature_flags_test.pycovers:truetype,default,description)All 17 tests pass.
Part of Comfy-Org/ComfyUI-Desktop-2.0-Beta#415.