Update install_env.sh of Comfy #1170
Conversation
WalkthroughThe pull request modifies the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
onediff_comfy_nodes/benchmarks/scripts/install_env.sh(1 hunks)
🔇 Additional comments (1)
onediff_comfy_nodes/benchmarks/scripts/install_env.sh (1)
31-31: Pin nexfort package version for reproducibility.
The nexfort package installation lacks version pinning, which could lead to inconsistent environments across different installations.
-python3 -m pip install --user nexfort
+python3 -m pip install --user "nexfort==<specific-version>"Let's check the latest stable version:
| python3 -m pip install --user -r $CUSTOM_NODES/ComfyUI_InstantID/requirements.txt | ||
| python3 -m pip install --user -r $CUSTOM_NODES/PuLID_ComfyUI/requirements.txt |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for requirements installation.
The requirements installation commands should include error handling to ensure the script fails gracefully if the requirements files are missing or if installations fail.
Consider this improvement:
- python3 -m pip install --user -r $CUSTOM_NODES/ComfyUI_InstantID/requirements.txt
- python3 -m pip install --user -r $CUSTOM_NODES/PuLID_ComfyUI/requirements.txt
+ for node in "ComfyUI_InstantID" "PuLID_ComfyUI"; do
+ req_file="$CUSTOM_NODES/$node/requirements.txt"
+ if [ -f "$req_file" ]; then
+ echo "Installing requirements for $node..."
+ python3 -m pip install --user -r "$req_file" || {
+ echo "Failed to install requirements for $node"
+ exit 1
+ }
+ else
+ echo "Warning: Requirements file not found for $node"
+ fi
+ done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| python3 -m pip install --user -r $CUSTOM_NODES/ComfyUI_InstantID/requirements.txt | |
| python3 -m pip install --user -r $CUSTOM_NODES/PuLID_ComfyUI/requirements.txt | |
| for node in "ComfyUI_InstantID" "PuLID_ComfyUI"; do | |
| req_file="$CUSTOM_NODES/$node/requirements.txt" | |
| if [ -f "$req_file" ]; then | |
| echo "Installing requirements for $node..." | |
| python3 -m pip install --user -r "$req_file" || { | |
| echo "Failed to install requirements for $node" | |
| exit 1 | |
| } | |
| else | |
| echo "Warning: Requirements file not found for $node" | |
| fi | |
| done |
| fi | ||
|
|
||
| echo "Installing common dependencies..." | ||
| pip install websocket-client==1.8.0 numpy==1.26.4 scikit-image -i https://pypi.tuna.tsinghua.edu.cn/simple | ||
| pip install nexfort | ||
| python3 -m pip install --user websocket-client==1.8.0 numpy==1.26.4 scikit-image "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i https://pypi.tuna.tsinghua.edu.cn/simple |
There was a problem hiding this comment.
💡 Codebase verification
Make mirror URL configurable and add CUDA version check
Based on the repository documentation and code analysis:
- The project explicitly requires CUDA 11.8 (with 12.1 and 12.2 as alternatives)
- The codebase includes GPU-specific optimizations and CUDA dependencies
- flash-attn is used in CrossAttention modules with specific dimension requirements
Suggested changes:
+# Check CUDA version
+if ! command -v nvcc &> /dev/null; then
+ echo "Warning: NVCC not found. flash-attn requires CUDA (11.8, 12.1, or 12.2)"
+ exit 1
+fi
+
+# Allow mirror configuration
+PIP_MIRROR=${PIP_MIRROR:-"https://pypi.tuna.tsinghua.edu.cn/simple"}
+
-python3 -m pip install --user websocket-client==1.8.0 numpy==1.26.4 scikit-image "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i https://pypi.tuna.tsinghua.edu.cn/simple
+python3 -m pip install --user websocket-client==1.8.0 numpy==1.26.4 scikit-image "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i "$PIP_MIRROR"🔗 Analysis chain
Consider environment-specific mirror selection and verify CUDA compatibility.
The script has two potential improvements:
- The Tsinghua mirror is hardcoded, which might not be optimal for users outside China
- flash-attn 2.5.8 has specific CUDA requirements that should be verified before installation
Consider making the mirror URL configurable:
-python3 -m pip install --user websocket-client==1.8.0 numpy==1.26.4 scikit-image "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i https://pypi.tuna.tsinghua.edu.cn/simple
+PIP_MIRROR=${PIP_MIRROR:-"https://pypi.tuna.tsinghua.edu.cn/simple"}
+python3 -m pip install --user websocket-client==1.8.0 numpy==1.26.4 scikit-image "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i "$PIP_MIRROR"Let's verify CUDA compatibility:
Let me check if there are any CUDA requirements or compatibility notes in the repository documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check CUDA version and nvcc availability
if command -v nvcc &> /dev/null; then
echo "CUDA version:"
nvcc --version
else
echo "Warning: NVCC not found. flash-attn might not work without CUDA support"
fi
Length of output: 170
Script:
#!/bin/bash
# Check for CUDA requirements in documentation
rg -i "cuda|gpu" README.md || echo "No CUDA/GPU mentions in README"
# Check for any installation requirements or prerequisites
fd "requirements|install" -e txt -e md -X cat {} || echo "No requirement files found"
# Look for any flash-attn related configuration or setup
rg -i "flash.?attn" -A 3 -B 3
Length of output: 4167
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
onediff_comfy_nodes/benchmarks/scripts/install_env.sh (1)
30-30: Consider splitting package installation into logical groupsThe single pip install command mixes different types of dependencies (websocket, numpy, ML-specific packages). Consider grouping them logically for better maintainability.
-python3 -m pip install --user nexfort websocket-client==1.8.0 numpy==1.26.4 scikit-image "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i https://pypi.tuna.tsinghua.edu.cn/simple +# Base dependencies +python3 -m pip install --user websocket-client==1.8.0 numpy==1.26.4 scikit-image -i "$PIP_MIRROR" +# ML-specific dependencies +python3 -m pip install --user "huggingface_hub==0.25.0" "flash-attn==2.5.8" -i "$PIP_MIRROR" +# Additional tools +python3 -m pip install --user nexfort -i "$PIP_MIRROR"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/examples.yml(1 hunks)onediff_comfy_nodes/benchmarks/scripts/install_env.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/examples.yml
🔇 Additional comments (3)
onediff_comfy_nodes/benchmarks/scripts/install_env.sh (3)
25-26: Add error handling for requirements installation.
The requirements installation commands need proper error handling to ensure the script fails gracefully if the requirements files are missing or if installations fail.
30-30: Make mirror URL configurable and add CUDA version check
The mirror URL is hardcoded and flash-attn has specific CUDA requirements.
30-30: Verify the purpose of the nexfort package
The nexfort package is being installed but wasn't mentioned in the PR summary or description. Please clarify its purpose and if it's a new requirement.
✅ Verification successful
Nexfort is a valid requirement - it's a PyTorch model compilation backend
Based on the code analysis, nexfort is a legitimate requirement used as a backend for PyTorch model compilation in this project. Specifically:
- It's used in the
BasicNexFortBoosterExecutorclass to optimize model performance through compilation - The package provides performance optimizations for various model components (ModelPatcher, VAE, ControlNet, etc.)
- It's integrated with OneDiff's inference compiler system as shown in the backend implementation
- The package is used to apply memory format optimizations and model compilation with settings like "max-optimize:max-autotune:low-precision"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for nexfort usage in the codebase
echo "Searching for nexfort imports and usage..."
rg -l "import.*nexfort|from.*nexfort" || echo "No direct imports found"
echo "Searching for nexfort in requirements files..."
fd requirements.txt -x grep -l "nexfort" {} \;
Length of output: 1705
Script:
#!/bin/bash
# Let's check the content of some key files to understand nexfort's purpose
echo "=== Checking nexfort implementation ==="
cat src/onediff/infer_compiler/backends/nexfort/__init__.py
echo -e "\n=== Checking nexfort booster implementation ==="
cat onediff_comfy_nodes/modules/nexfort/booster_basic.py
echo -e "\n=== Checking requirements.txt that mentions nexfort ==="
cat onediff_comfy_nodes/docs/sd3/requirements.txt
Length of output: 4768
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/examples.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/examples.yml
298-298: shellcheck reported issue in this script: SC1009:info:1:1: The mentioned syntax error was in this simple command
(shellcheck)
298-298: shellcheck reported issue in this script: SC1073:error:1:96: Couldn't parse this double quoted string. Fix to allow more checks
(shellcheck)
298-298: shellcheck reported issue in this script: SC1072:error:2:1: Expected end of double quoted string. Fix any mentioned problems and try again
(shellcheck)
🔇 Additional comments (5)
.github/workflows/examples.yml (5)
176-177: LGTM: Docker image updates
The Docker image specifications have been updated to use newer versions with PyTorch 2.3 support.
Also applies to: 183-183
286-286: Good practice: Added pip cache inspection and version pinning
The addition of pip cache inspection helps with debugging, and pinning huggingface_hub to a specific version helps ensure reproducible builds.
Also applies to: 293-294
295-297: LGTM: Conditional requirements installation
Requirements installation is properly conditioned based on the test suite being run.
299-300: LGTM: Transformer engine removal
Removing transformer-engine for ComfyUI test suite is correctly conditioned.
363-363: Verify package compatibility
The installation includes multiple packages with specific version constraints. Let's verify their compatibility.
.github/workflows/examples.yml
Outdated
| if: matrix.test-suite == 'diffusers_examples' | ||
| - run: docker exec ${{ env.CONTAINER_NAME }} python3 -m pip install -r tests/comfyui/requirements.txt --user | ||
| if: matrix.test-suite == 'comfy' | ||
| - run: docker exec ${{ env.CONTAINER_NAME }} python3 -m pip install -r ComfyUI/requirements.txt --user" |
There was a problem hiding this comment.
Fix shell script syntax error
There's a syntax error in the pip install command - an extra quote at the end.
Apply this fix:
- - run: docker exec ${{ env.CONTAINER_NAME }} python3 -m pip install -r ComfyUI/requirements.txt --user"
+ - run: docker exec ${{ env.CONTAINER_NAME }} python3 -m pip install -r ComfyUI/requirements.txt --user📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - run: docker exec ${{ env.CONTAINER_NAME }} python3 -m pip install -r ComfyUI/requirements.txt --user" | |
| - run: docker exec ${{ env.CONTAINER_NAME }} python3 -m pip install -r ComfyUI/requirements.txt --user |
🧰 Tools
🪛 actionlint (1.7.4)
298-298: shellcheck reported issue in this script: SC1009:info:1:1: The mentioned syntax error was in this simple command
(shellcheck)
298-298: shellcheck reported issue in this script: SC1073:error:1:96: Couldn't parse this double quoted string. Fix to allow more checks
(shellcheck)
298-298: shellcheck reported issue in this script: SC1072:error:2:1: Expected end of double quoted string. Fix any mentioned problems and try again
(shellcheck)
| - run: docker exec -w /src/onediff/onediff_comfy_nodes/benchmarks ${{ env.CONTAINER_NAME }} bash scripts/install_env.sh /app/ComfyUI | ||
| if: matrix.test-suite == 'comfy' |
There was a problem hiding this comment.
💡 Codebase verification
Script permissions need to be updated for execution
The installation script (install_env.sh) exists but lacks execution permissions (current: -rw-r--r--). This needs to be addressed for the workflow to execute properly.
- File:
onediff_comfy_nodes/benchmarks/scripts/install_env.shneeds execution permissions (chmod +x) - The script's contents look correct and safe:
- It properly validates input parameters
- Creates symbolic links for custom nodes
- Handles CI vs local environment appropriately
- Installs required dependencies
To fix this, you should add a step to set execution permissions before running the script:
- run: chmod +x onediff_comfy_nodes/benchmarks/scripts/install_env.sh
- run: docker exec -w /src/onediff/onediff_comfy_nodes/benchmarks ${{ env.CONTAINER_NAME }} bash scripts/install_env.sh /app/ComfyUI🔗 Analysis chain
Verify installation script execution
The installation script is being executed in the ComfyUI environment. Let's verify its contents and execution permissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the installation script exists and has proper permissions
ls -l onediff_comfy_nodes/benchmarks/scripts/install_env.sh
# Check the script's contents for potential issues
cat onediff_comfy_nodes/benchmarks/scripts/install_env.sh
Length of output: 1229
Summary by CodeRabbit
New Features
opencv-python-headlessto the requirements.Bug Fixes