fix: skip workspace venv creation for global Python installs#394
Merged
fix: skip workspace venv creation for global Python installs#394
Conversation
📝 WalkthroughWalkthroughensure_workspace_python() gains PEP 668 detection via a new _is_externally_managed() helper and now returns the system Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ComfyInstall as comfy install
participant EnvResolver as ensure_workspace_python()
participant PythonEnv as Python Interpreter / sysconfig
participant DepCompiler as DependencyCompiler
User->>ComfyInstall: Run install (optionally --fast-deps)
ComfyInstall->>EnvResolver: Resolve workspace python for workspace
EnvResolver->>PythonEnv: Inspect sys.prefix/sys.base_prefix and sysconfig (PEP 668)
PythonEnv-->>EnvResolver: Report global vs isolated and externally-managed flag
alt Global & externally-managed
EnvResolver->>EnvResolver: Create workspace/.venv
EnvResolver-->>ComfyInstall: Return workspace venv python
else Global & not externally-managed
EnvResolver-->>ComfyInstall: Return system sys.executable
else Isolated environment
EnvResolver->>EnvResolver: Create workspace/.venv
EnvResolver-->>ComfyInstall: Return workspace venv python
end
ComfyInstall->>DepCompiler: If fast-deps and python != sys.executable then Install_Build_Deps(executable=python)
ComfyInstall->>DepCompiler: Compile/install deps (using returned python)
DepCompiler-->>ComfyInstall: Installation complete
ComfyInstall-->>User: DONE
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 74.66% 74.71% +0.05%
==========================================
Files 33 33
Lines 3915 3924 +9
==========================================
+ Hits 2923 2932 +9
Misses 992 992
🚀 New features to boost your workflow:
|
dosubot
bot
added
size:L
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 73-80: Update the README paragraph to explicitly describe the
precedence that comfy_cli.resolve_python.ensure_workspace_python() uses: first
use an active VIRTUAL_ENV or CONDA_PREFIX if present, next prefer an existing
workspace virtualenv at workspace/.venv or workspace/venv, and only then fall
back to the global-vs-isolated install behavior (pip vs pipx/uv tool) described;
mention that this overrides install-mode-only wording and that comfy install
will respect an existing workspace venv before choosing the install-mode target.
In `@tests/comfy_cli/test_global_python_install.py`:
- Around line 133-192: These tests call DependencyCompiler.compile_deps() which
shells out (DependencyCompiler.Compile -> _check_call -> subprocess.check_call)
so patch comfy_cli.uv._check_call in test_compile_produces_complete_output and
test_compile_nvidia_resolves_torch to avoid real network calls: in each test use
patch("comfy_cli.uv._check_call") to return a success code and write a
deterministic fake compiled requirements file to Path(dep.out) (including
entries like "pyyaml==.." "requests==.." and "--index-url" for the first test,
and "torch==.." "pyyaml==.." plus "https://pypi.org/simple" for the second), or
alternatively mark the two tests as integration tests and ensure the test
environment scrubs UV_INDEX_URL/PIP_INDEX_URL before calling dep.compile_deps();
keep test_install_targets_correct_python's existing _check_call mock pattern as
the model.
🪄 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
Run ID: e3eba3a2-9af7-488a-830a-9a81f3d354a9
📒 Files selected for processing (4)
README.mdcomfy_cli/resolve_python.pytests/comfy_cli/test_global_python_install.pytests/comfy_cli/test_resolve_python.py
…ADME precedence docs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_global_python_install.py`:
- Line 150: The test currently reads the file with Path(dep.out).read_text()
which relies on the system default encoding; change it to explicitly specify
UTF-8 by calling Path(dep.out).read_text(encoding="utf-8") so the variable
compiled is decoded consistently across platforms and avoids UnicodeDecodeError.
- Line 176: The test reads a file with Path(dep.out).read_text() without an
explicit encoding which can be platform-dependent; update the call used to
produce the compiled variable (the Path.read_text invocation where compiled =
Path(dep.out).read_text().lower()) to pass encoding="utf-8" so the file is read
consistently across platforms and then call .lower() as before.
- Around line 23-37: Replace the manual class-based context manager _clean_env
with a contextlib.contextmanager-based generator: keep the keys =
("VIRTUAL_ENV", "CONDA_PREFIX") and saved = {k: os.environ.pop(k, None) for k in
keys}, yield control once, and after the yield restore saved values back into
os.environ only when the saved value is not None; use the
`@contextlib.contextmanager` decorator on the _clean_env function to produce the
same behavior with cleaner syntax.
🪄 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
Run ID: 7cefc000-001b-4f00-8a58-f8a0ffed75fb
📒 Files selected for processing (2)
README.mdtests/comfy_cli/test_global_python_install.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_global_python_install.py`:
- Around line 23-37: The _clean_env() context manager is duplicated from
tests/comfy_cli/test_resolve_python.py; extract it to a shared test utility
(e.g., tests/comfy_cli/conftest.py or tests/comfy_cli/helpers.py) as a reusable
context manager (rename to something like clean_env or keep _clean_env)
implemented with contextlib.contextmanager, then update both
test_global_python_install.py and test_resolve_python.py to import and use the
shared helper instead of their local copies; ensure the saved keys tuple
("VIRTUAL_ENV","CONDA_PREFIX") and restore logic remain identical so behavior is
unchanged.
- Around line 40-74: The two tests test_global_python_skips_venv_creation and
test_isolated_env_creates_venv in tests/comfy_cli/test_global_python_install.py
duplicate coverage already provided by test_global_python_returns_sys_executable
and test_creates_venv_when_isolated_env in
tests/comfy_cli/test_resolve_python.py; update by consolidating or
differentiating: either remove the duplicated tests here and rely on the
existing tests in test_resolve_python.py, or refactor to call/parametrize the
shared behavior around ensure_workspace_python so a single test covers both
mocked global vs isolated cases, or modify these tests to assert only the unique
behavior (e.g., existence of .venv vs returned path) that is not already
asserted in test_resolve_python.py; reference ensure_workspace_python and the
four test function names when making the change.
🪄 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
Run ID: 02ff52b9-5ab0-4bd9-aef6-030239d050cb
📒 Files selected for processing (3)
comfy_cli/resolve_python.pytests/comfy_cli/test_global_python_install.pytests/comfy_cli/test_resolve_python.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_global_python_install.py`:
- Around line 83-92: Patch ConfigManager in the same with(...) block so the test
doesn't use the real singleton that writes to disk; specifically add a patch for
ConfigManager (the class used by install.execute()) and ensure the returned mock
instance stubs out set(...) and write_config(...) so calls like
ConfigManager().set(..., "disable") cannot persist to the real config; include
this new patch alongside the existing patches (e.g., with
patch("comfy_cli.command.install.ConfigManager") as MockConfig) to keep test
isolation.
- Around line 80-105: The test helper _run_execute leaves the process cwd
changed because install.execute() chdirs into repo_dir and never restores it;
add an import for os if missing, capture the current working directory at the
start of _run_execute (e.g., orig_cwd = os.getcwd()), call install.execute(...)
inside a try block, and in a finally block call os.chdir(orig_cwd) to ensure the
cwd is restored after the call; reference _run_execute and install.execute when
making this change.
🪄 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
Run ID: 5b26af30-fac0-49f1-b7c4-6fd256727bdf
📒 Files selected for processing (2)
comfy_cli/command/install.pytests/comfy_cli/test_global_python_install.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #375 introduced automatic workspace
.venvcreation so thatpipx/uv toolusers don't pollute their isolated tool environment with ComfyUI dependencies.However, it unconditionally creates a
.venveven when comfy-cli is installed via plainpip installinto the system Python (the typical Docker setup).In that case dependencies end up in the
.venvwhile ComfyUI is launched with the system Python, causingModuleNotFoundErrorforyaml,sqlalchemy, and other packages.The fix checks
sys.prefix == sys.base_prefixbefore deciding whether to create a workspace venv.When they match we are running from the global/system Python and can install deps there directly, matching the pre-#375 behaviour.
When they differ (pipx,
uv tool, etc.) the workspace.venvis still created as before.The README now documents the three environment modes (global pip, isolated tool, active venv/conda).
Fixes #393.