v1.4.28: Face-detailer ultralytics fix and Nix flake packaging#400
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a Nix flake for Linux/NixOS support and automates the installation of the ultralytics Python dependency required by the face-detailer custom node. The feedback recommends pinning the ultralytics dependency version to adhere to supply-chain hardening policies, and switching to wrapGAppsHook3 in the Nix flake to match the GTK 3 environment. Additionally, the reviewer suggests simplifying ensure_mooshie_node_requirements to return () and removing the corresponding .map_err(...)? calls in process.rs so that dependency installation failures do not inadvertently block application startup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Python deps the mooshie-nodes package imports but ComfyUI does not ship. | ||
| // `ultralytics` (YOLOv8) backs the face-detailer node. torch/torchvision are | ||
| // already provided by ComfyUI's own requirements, so they are not repeated. | ||
| const MOOSHIE_NODES_REQUIREMENTS: &str = "ultralytics\n"; |
There was a problem hiding this comment.
To adhere to the project's supply-chain hardening policy (as established in v0.6.7, where unpinned pip installations are flagged as blocking errors to prevent malicious or breaking dependency updates), the ultralytics package requirement should be pinned to a specific version (e.g., 8.4.34, which is the version used elsewhere in the project).
| const MOOSHIE_NODES_REQUIREMENTS: &str = "ultralytics\n"; | |
| const MOOSHIE_NODES_REQUIREMENTS: &str = "ultralytics==8.4.34\\n"; |
| nodejs | ||
| npmHooks.npmConfigHook # populates node_modules from npmDeps | ||
| pkg-config | ||
| wrapGAppsHook4 |
There was a problem hiding this comment.
The application is built using GTK 3 and WebKitGTK 4.1 (which is GTK 3 based), as specified in buildInputs and the webkit2gtk Rust crate dependency. Therefore, wrapGAppsHook3 should be used instead of wrapGAppsHook4 to ensure the correct GTK 3 environment variables, schemas, and icon themes are wrapped and available at runtime.
wrapGAppsHook3
| pub async fn ensure_mooshie_node_requirements( | ||
| comfyui_path: &str, | ||
| venv_path: &str, | ||
| network_proxy: Option<&str>, | ||
| pip_index_url: Option<&str>, | ||
| ) -> Result<(), String> { |
There was a problem hiding this comment.
Since ensure_mooshie_node_requirements handles all internal errors (such as installation failures) by logging warnings and always returns Ok(()), returning a Result<(), String> is unnecessary. Changing the return type to () simplifies the function signature and removes redundant error-handling boilerplate at the call sites. Note that the Ok(()) at the end of this function should also be removed.
pub async fn ensure_mooshie_node_requirements(\n comfyui_path: &str,\n venv_path: &str,\n network_proxy: Option<&str>,\n pip_index_url: Option<&str>,\n) {| // Non-fatal: logs a warning on failure rather than blocking startup. | ||
| super::nodes::ensure_mooshie_node_requirements( | ||
| &config.comfyui_path, | ||
| &config.venv_path, | ||
| config.network_proxy.as_deref(), | ||
| config.pip_index_url.as_deref(), | ||
| ) | ||
| .await | ||
| .map_err(AppError::ProcessSpawnFailed)?; |
There was a problem hiding this comment.
The comment states that this operation is non-fatal, but using ? on the result of ensure_mooshie_node_requirements means any returned Err would block startup. If ensure_mooshie_node_requirements is refactored to return () (as it always succeeds internally by logging warnings on failure), we can remove the redundant .map_err(...)? boilerplate here.
| // Non-fatal: logs a warning on failure rather than blocking startup. | |
| super::nodes::ensure_mooshie_node_requirements( | |
| &config.comfyui_path, | |
| &config.venv_path, | |
| config.network_proxy.as_deref(), | |
| config.pip_index_url.as_deref(), | |
| ) | |
| .await | |
| .map_err(AppError::ProcessSpawnFailed)?; | |
| // Non-fatal: logs a warning on failure rather than blocking startup.\n super::nodes::ensure_mooshie_node_requirements(\n &config.comfyui_path,\n &config.venv_path,\n config.network_proxy.as_deref(),\n config.pip_index_url.as_deref(),\n )\n .await; |
| // Non-fatal: logs a warning on failure rather than blocking startup. | ||
| super::nodes::ensure_mooshie_node_requirements( | ||
| &config.comfyui_path, | ||
| &config.venv_path, | ||
| config.network_proxy.as_deref(), | ||
| config.pip_index_url.as_deref(), | ||
| ) | ||
| .await | ||
| .map_err(AppError::ProcessSpawnFailed)?; |
There was a problem hiding this comment.
Similarly to the primary startup path, we can remove the redundant .map_err(...)? boilerplate here to match the non-fatal nature of the operation.
| // Non-fatal: logs a warning on failure rather than blocking startup. | |
| super::nodes::ensure_mooshie_node_requirements( | |
| &config.comfyui_path, | |
| &config.venv_path, | |
| config.network_proxy.as_deref(), | |
| config.pip_index_url.as_deref(), | |
| ) | |
| .await | |
| .map_err(AppError::ProcessSpawnFailed)?; | |
| // Non-fatal: logs a warning on failure rather than blocking startup.\n super::nodes::ensure_mooshie_node_requirements(\n &config.comfyui_path,\n &config.venv_path,\n config.network_proxy.as_deref(),\n config.pip_index_url.as_deref(),\n )\n .await; |
|
Triage of the Gemini review (per docs/BOT_REVIEW_TRIAGE.md): Fixed (d12d75c)
Keeping intentionally (Skip)
Deferred (Low priority)
|
Summary
Two independent changes, both originally developed on NixOS against the v1.4.27 tree and re-validated on the synced v1.4.28 tree with a full
nix build .#default.Fix: face-detailer node installs ultralytics (all platforms)
The bundled mooshie-nodes custom node imports
from ultralytics import YOLO, but nothing ever pip-installedultralyticsinto the ComfyUI venv, so any face-detailer workflow failed withNo module named 'ultralytics'. This affected Windows, macOS, and Linux.mooshie-nodes now ships a
requirements.txt, and a newensure_mooshie_node_requirements()installs it into the venv on launch (hash-stamped so it runs once per change, non-fatal on failure), mirroring the existing ControlNet and style-transfer pattern. Wired into both ComfyUI startup paths (start_comfyui_processandstart_worker_process). Existing users with a stale venv get ultralytics installed automatically on next launch.Packaging: Nix flake for Linux/NixOS
New
flake.nixplusflake.lock. Builds the Tauri app viarustPlatform.buildRustPackageandcargo-tauri.hook, then wraps it inbuildFHSEnvso the setup wizard's downloaded uv and python-build-standalone binaries (and the pip wheels they install) get a standard glibc and loader on NixOS. Usable withnix build .#defaultornix profile add .#default. The npm deps hash is pinned against this tree'spackage-lock.json.Release
Version bumped to 1.4.28 across
package.json,src-tauri/Cargo.toml,src-tauri/Cargo.lock,src-tauri/tauri.conf.json, andflake.nix. Changelog and release notes updated.Bot triage
Open PRs are all dependabot dependency bumps (#386, #385, #328 through #321). None are release-blocking or conflicting with this change.
Validation
nix build .#defaultgreen on the 1.4.28 tree (Rust release build plus cargo test, npm build, and FHS wrap).src-tauri/targetand nopython3on PATH, which makes the hook's full-repo grep and python checks unusable. The CI GlassWorm Infection Audit runs the authoritative scan on a clean checkout.