Feat/add comfy env var support#478
Conversation
- COMFY_BACKEND=host:port (one variable, colon-separated) - COMFY_HOST and COMFY_PORT (separate variables) These provide fallback when --host/--port are not passed on the command line. Resolves the missing environment variable oversight for backend configuration.
…d COMFY_BACKEND env vars - Extend _get_backend_from_env to handle COMFY_HOST_PORT (and keep COMFY_BACKEND for combined form) - Update docstrings and usage in cmdline.py and jobs.py - CLI flags still take precedence as before - This matches the requested env var naming (tracking CLI params closely)
|
🎉 Thank you for your contribution, we really appreciate it! 🎉 Like many open source projects, we require contributors to sign our Contributor License Agreement (CLA). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:
CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility. ✍ To sign, please post a new comment on this PR with exactly the following text: ✍ I have read and agree to the Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
📝 WalkthroughWalkthroughThis PR adds environment-variable-driven backend host/port resolution. A new ChangesEnvironment-based Backend Resolution
No port left unturned, no host unheard — with envvars now, the config's less absurd! A helper here, a fallback there, this backend resolution's finally fair. Compact metadata
Related issues: None specified 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
dosubot
Bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
comfy_cli/command/jobs.py (1)
38-64: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSame helper, twice the maintenance burden.
Identical to
_get_backend_from_env()incomfy_cli/cmdline.py(docstring, IPv6 rsplit gap, silentValueErrorswallow, and the localimport osshadowing the module-level import at line 23 — flagged by Pylint W0621/W0404/C0415, all included). See the detailed fix suggestion on the cmdline.py copy; the punchline here is the same: extract once, import twice, and this bug-fixing duet becomes a solo.🤖 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 `@comfy_cli/command/jobs.py` around lines 38 - 64, The helper _get_backend_from_env in jobs.py is a duplicate of the same logic in cmdline.py and should not be maintained in two places. Move the backend parsing into one shared helper and have both callers import/use that single implementation, keeping the existing behavior for COMFY_BACKEND/COMFY_HOST_PORT and COMFY_HOST/COMFY_PORT while preserving the same return shape. Also remove the local import os inside _get_backend_from_env so it uses the module-level import, matching the shared cleanup.Source: Linters/SAST tools
🤖 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 `@comfy_cli/cmdline.py`:
- Around line 912-916: The validate command’s envvar handling is inconsistent
with run() because it only uses Typer envvar bindings for COMFY_HOST/COMFY_PORT
and ignores COMFY_BACKEND/COMFY_HOST_PORT. Update the validate path in
comfy_cli/cmdline.py to use the same backend resolution logic as
_get_backend_from_env(), or a shared helper, so both validate and run honor the
same environment variables. Keep the fix anchored around validate() and
_get_backend_from_env() to preserve parity for where-aware commands.
- Around line 47-73: The env-backend parsing helper is duplicated and should be
centralized so `cmdline.py` and `command/jobs.py` both use a shared function
instead of copy-pasting `_get_backend_from_env`. Move the logic into a common
module and import it at each call site, then remove the local `import os` from
`_get_backend_from_env` since `os` is already available at module scope. While
refactoring, make the parser handle IPv6/bracketed hosts correctly and avoid
silently swallowing invalid port values; keep the shared helper consistent with
the host/port resolution behavior used elsewhere such as `_resolve_host_port`.
---
Duplicate comments:
In `@comfy_cli/command/jobs.py`:
- Around line 38-64: The helper _get_backend_from_env in jobs.py is a duplicate
of the same logic in cmdline.py and should not be maintained in two places. Move
the backend parsing into one shared helper and have both callers import/use that
single implementation, keeping the existing behavior for
COMFY_BACKEND/COMFY_HOST_PORT and COMFY_HOST/COMFY_PORT while preserving the
same return shape. Also remove the local import os inside _get_backend_from_env
so it uses the module-level import, matching the shared cleanup.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7419be01-7f4d-4455-89cc-09fe789e7847
📒 Files selected for processing (2)
comfy_cli/cmdline.pycomfy_cli/command/jobs.py
|
|
||
| def _get_backend_from_env(): | ||
| """Resolve host/port from environment variables as requested. | ||
| - COMFY_BACKEND=host:port or COMFY_HOST_PORT=host:port (combined, colon-separated) | ||
| - COMFY_HOST and COMFY_PORT (separate) | ||
| CLI flags take precedence; this is a fallback. | ||
| """ | ||
| import os | ||
| be = os.environ.get("COMFY_BACKEND") or os.environ.get("COMFY_HOST_PORT") | ||
| if be: | ||
| b = be.replace("http://", "").replace("https://", "").strip("/") | ||
| if ":" in b: | ||
| h, p = b.rsplit(":", 1) | ||
| try: | ||
| return h, int(p) | ||
| except ValueError: | ||
| return h, None | ||
| return b, None | ||
| h = os.environ.get("COMFY_HOST") | ||
| p = os.environ.get("COMFY_PORT") | ||
| if h or p: | ||
| try: | ||
| return h, int(p) if p else None | ||
| except ValueError: | ||
| return h, None | ||
| return None, None | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Duplicate helper + local os reimport — consolidate to a shared module.
This exact function (docstring included) is copy-pasted into comfy_cli/command/jobs.py (lines 39-64). Two copies of the same env-parsing logic is a DRY violation waiting to drift out of sync — one env fix in jobs.py and the other silently rots. Extract to a shared module (e.g. comfy_cli/env_backend.py) and import from both call sites.
Also, os is already imported at module scope in this file — the local import os on line 54 is flagged by Pylint as a redefinition/reimport (W0621/W0404/C0415); no need to smuggle it back in through the function door.
Two more gremlins worth squashing:
b.rsplit(":", 1)mishandles bracketed/bare IPv6 hosts without an explicit port (e.g.COMFY_BACKEND=[::1]→ host becomes"[:", port becomes"1]").target.pyandjobs.py's_resolve_host_portcarefully bracket IPv6 elsewhere — this parser doesn't account for it at all.- A malformed
COMFY_PORT/embedded port silently swallows theValueErrorand returnsNone, so a typo'd env var quietly falls back to defaults with zero feedback — a maintenance riddle for future debuggers.
♻️ Sketch of a shared helper with IPv6-awareness
-def _get_backend_from_env():
- """Resolve host/port from environment variables as requested.
- - COMFY_BACKEND=host:port or COMFY_HOST_PORT=host:port (combined, colon-separated)
- - COMFY_HOST and COMFY_PORT (separate)
- CLI flags take precedence; this is a fallback.
- """
- import os
- be = os.environ.get("COMFY_BACKEND") or os.environ.get("COMFY_HOST_PORT")
- if be:
- b = be.replace("http://", "").replace("https://", "").strip("/")
- if ":" in b:
- h, p = b.rsplit(":", 1)
- try:
- return h, int(p)
- except ValueError:
- return h, None
- return b, None
- h = os.environ.get("COMFY_HOST")
- p = os.environ.get("COMFY_PORT")
- if h or p:
- try:
- return h, int(p) if p else None
- except ValueError:
- return h, None
- return None, None
+from comfy_cli.env_backend import get_backend_from_env # shared, IPv6-safe, logs on parse failureAs per Pylint (C0415/W0621/W0404) hints, move os usage to the top-level import.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 56-56: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🪛 Pylint (4.0.6)
[warning] 54-54: Redefining name 'os' from outer scope (line 2)
(W0621)
[warning] 54-54: Reimport 'os' (imported line 2)
(W0404)
[convention] 54-54: Import outside toplevel (os)
(C0415)
🤖 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 `@comfy_cli/cmdline.py` around lines 47 - 73, The env-backend parsing helper is
duplicated and should be centralized so `cmdline.py` and `command/jobs.py` both
use a shared function instead of copy-pasting `_get_backend_from_env`. Move the
logic into a common module and import it at each call site, then remove the
local `import os` from `_get_backend_from_env` since `os` is already available
at module scope. While refactoring, make the parser handle IPv6/bracketed hosts
correctly and avoid silently swallowing invalid port values; keep the shared
helper consistent with the host/port resolution behavior used elsewhere such as
`_resolve_host_port`.
Source: Linters/SAST tools
| typer.Option(show_default=False, help="ComfyUI host (default 127.0.0.1).", envvar="COMFY_HOST"), | ||
| ] = None, | ||
| port: Annotated[ | ||
| int | None, | ||
| typer.Option(show_default=False, help="ComfyUI port (default 8188)."), | ||
| typer.Option(show_default=False, help="ComfyUI port (default 8188).", envvar="COMFY_PORT"), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Feature-parity gap: validate's envvar wiring skips COMFY_BACKEND/COMFY_HOST_PORT.
run() resolves env fallback via _get_backend_from_env(), which honors COMFY_BACKEND/COMFY_HOST_PORT in addition to COMFY_HOST/COMFY_PORT. validate() instead relies on Typer's built-in envvar= binding, which only recognizes COMFY_HOST/COMFY_PORT — a combined COMFY_BACKEND=host:port won't help here, host/port hoping in vain. Two commands, two different resolution mechanisms for what's supposed to be the same feature is a confusing precedent for whoever adds the next where-aware command.
Consider routing validate() through the same _get_backend_from_env() fallback (or the shared helper suggested above) so both commands honor the identical set of env vars.
🤖 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 `@comfy_cli/cmdline.py` around lines 912 - 916, The validate command’s envvar
handling is inconsistent with run() because it only uses Typer envvar bindings
for COMFY_HOST/COMFY_PORT and ignores COMFY_BACKEND/COMFY_HOST_PORT. Update the
validate path in comfy_cli/cmdline.py to use the same backend resolution logic
as _get_backend_from_env(), or a shared helper, so both validate and run honor
the same environment variables. Keep the fix anchored around validate() and
_get_backend_from_env() to preserve parity for where-aware commands.
1 participant
Title: Feature support COMFY_HOST, COMFY_PORT, COMFY_HOST_PORT and COMFY_BACKEND environment variables
Body:
Plan
The goal was to add first-class support for environment variables so the comfy tool can be configured to talk to a non-local ComfyUI backend without always requiring
--host/--portflags or relying on hardcoded defaults. This was driven by the need for reliable, scriptable, non-interactive usage (e.g., in containers, CI, or multi-machine setups) while keeping CLI flags as the highest-precedence option.Spec
COMFY_prefix for clarity):COMFY_HOSTCOMFY_PORTCOMFY_HOST_PORT(combinedhost:portform)COMFY_BACKEND(combined form, kept for compatibility)--host/--port).COMFY_BACKENDorCOMFY_HOST_PORT, or separateCOMFY_HOST+COMFY_PORT)._get_backend_from_env) lives in bothcomfy_cli/cmdline.py(main entry) andcomfy_cli/command/jobs.py(job-related paths) to ensure consistent behavior.Tests performed (all on the modified source from the fork, using visible shell simulation via terminal, one or two vars at a time
pip install -e .).COMFY_HOST=172.29.112.112+COMFY_PORT=8188→ resolved to('172.29.112.112', 8188)COMFY_BACKEND=172.29.112.112:8188→ resolved correctlyCOMFY_HOST_PORT=172.29.112.112:8188→ resolved correctly (combined path)curl -I http://172.29.112.112:8188returned HTTP 200 OK.=== INPUT:):comfy --help(withCOMFY_HOST+COMFY_PORT)comfy(bare, with vars set) → showed full agent-aware banner + quick-start (no localhost fallback)comfy nodes ls(withCOMFY_HOST+COMFY_PORT) → returned liveobject_infofrom the remote server (1861 nodes, including packs like ImpactPack, KJNodes, InspirePack, partner nodes for Tripo/ByteDance/Vidu/Wan, etc.). This is the key proof: the data came from172.29.112.112:8188, not defaults.Scripts).The tests prove the env vars are not only read but are actually wired into the backend connection used by real commands.
Additional notes
Tested in the Windows git-bash environment matching the user's setup.
No localhost/127.0.0.1/0.0.0.0 references were introduced or left in the changed code.