feat: Add InsightFace/PuLID support on macOS Apple Silicon

[jamesbrink]

Summary

Closes #13

This PR enables InsightFace and PuLID custom node support on macOS Apple Silicon by removing the MXNet dependency and using ONNX Runtime instead.

• Override insightface to remove mxnet dependency (only used for legacy CLI command)
• Patch facexlib to respect FACEXLIB_MODELPATH env var for Nix store compatibility
• Enable PuLID on macOS with proper model cache directory redirection
• Add documentation and helper script for model downloads

Changes

File	Change
nix/python-overrides.nix	Remove mxnet from insightface deps; patch facexlib for env var
nix/packages.nix	Enable face packages on all platforms; add cache env vars
nix/custom-nodes.nix	Add facexlib to PuLID deps; update comments
docs/pulid-setup.md	New comprehensive setup guide
scripts/download-pulid-models.sh	New helper script for model downloads

Technical Approach

1. InsightFace without MXNet: The nixpkgs insightface package depends on mxnet, which is Linux-only. However, mxnet is only used for one legacy CLI command - face analysis uses ONNX Runtime. We override insightface to filter out the mxnet dependency.

2. FaceXLib Nix Store Fix: FaceXLib tries to download models to its package directory (read-only in Nix). We patch it via postInstall sed to check FACEXLIB_MODELPATH environment variable, which the launcher sets to $BASE_DIR/.cache/facexlib.

3. Cache Directory Redirection: The launcher now sets TORCH_HOME, HF_HOME, and FACEXLIB_MODELPATH to redirect all model downloads to the user's data directory.

Test plan

• Build succeeds on macOS (aarch64-darwin)
• All flake checks pass
• InsightFace imports successfully without mxnet
• FaceXLib models download to cache directory (not Nix store)
• PuLID workflow executes successfully with face detection

[claude]

Code Review: InsightFace/PuLID macOS Apple Silicon Support

Overall, this is a well-implemented PR that successfully enables cross-platform support for face analysis features. The approach is technically sound and follows Nix best practices. I have a few suggestions for improvement:

---

✅ Strengths

1. Excellent Technical Approach: Removing the mxnet dependency from insightface is the right solution. MXNet is indeed only used for a legacy CLI command, and ONNX Runtime handles the actual face analysis.

2. Proper Nix Patterns:

   • Using postInstall with sed to patch facexlib is appropriate for Nix
   • The FACEXLIB_MODELPATH environment variable pattern follows Nix conventions
   • Good use of lib.optionalAttrs for conditional package overrides

3. Comprehensive Documentation: The docs/pulid-setup.md is thorough and user-friendly with clear instructions for multiple platforms.

4. Helper Script Quality: The download script has good error handling, platform detection, and user feedback.

---

🔍 Issues & Suggestions

1. Security: Unverified Model Downloads (Medium Priority)

Location: scripts/download-pulid-models.sh:56-90

The script downloads models without checksum verification:

curl -L "$PULID_URL" -o "$PULID_MODEL" --progress-bar

Recommendation: Add SHA256 checksum verification after downloads to prevent supply chain attacks:

PULID_SHA256="expected_hash_here"
curl -L "$PULID_URL" -o "$PULID_MODEL" --progress-bar

# Verify checksum
if command -v sha256sum &> /dev/null; then
    echo "$PULID_SHA256  $PULID_MODEL" | sha256sum -c - || error "Checksum verification failed"
elif command -v shasum &> /dev/null; then
    echo "$PULID_SHA256  $PULID_MODEL" | shasum -a 256 -c - || error "Checksum verification failed"
fi

2. Potential Regex Issue in sed Pattern (Low Priority)

Location: nix/python-overrides.nix:198

The sed command uses a complex replacement without escaping:

sed -i 's|^ROOT_DIR = os.path.dirname.*|_DEFAULT_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))\nROOT_DIR = os.environ.get("FACEXLIB_MODELPATH", _DEFAULT_ROOT)|' "$miscPy"

Issues:

• The replacement contains parentheses that could conflict with sed's regex
• No validation that the patch succeeded

Recommendation: Either use sed -i.bak to create a backup, or add verification:

postInstall = ''
  miscPy="$out/${final.python.sitePackages}/facexlib/utils/misc.py"
  if [[ -f "$miscPy" ]]; then
    sed -i.bak 's|^ROOT_DIR = os.path.dirname.*|_DEFAULT_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))\nROOT_DIR = os.environ.get("FACEXLIB_MODELPATH", _DEFAULT_ROOT)|' "$miscPy"
    if ! grep -q "FACEXLIB_MODELPATH" "$miscPy"; then
      echo "ERROR: Failed to patch facexlib misc.py" >&2
      exit 1
    fi
    rm "$miscPy.bak"
  fi
'';

3. Missing Test for insightface Override (Medium Priority)

Location: nix/python-overrides.nix:211-234

The insightface override filters out mxnet but doesn't verify the filtering worked correctly.

Recommendation: Add a build-time check:

insightface = prev.insightface.overridePythonAttrs (old: {
  dependencies = builtins.filter (dep: dep.pname or "" != "mxnet") (old.dependencies or [ ]);
  
  disabledTests = (old.disabledTests or [ ]) ++ [
    "test_cli"
  ];
  
  # Verify mxnet was actually removed
  postInstall = (old.postInstall or "") + ''
    # Check that mxnet isn't in the package metadata
    if grep -q "mxnet" "$out/${final.python.sitePackages}/insightface-"*.dist-info/METADATA 2>/dev/null; then
      echo "ERROR: mxnet still present in insightface metadata" >&2
      exit 1
    fi
  '';
  
  pythonImportsCheck = [
    "insightface"
    "insightface.app"
    "insightface.model_zoo"
  ];
  
  meta = (old.meta or { }) // {
    platforms = lib.platforms.unix;
  };
});

4. Directory Creation Race Condition (Low Priority)

Location: nix/packages.nix:363

export FACEXLIB_MODELPATH="$BASE_DIR/.cache/facexlib"
mkdir -p "$FACEXLIB_MODELPATH/facexlib/weights"

Creating the nested directory structure with the specific subdirectory is good, but the path structure seems to duplicate "facexlib" (/facexlib/facexlib/weights).

Question: Is this intentional based on how facexlib expects the path? If not, consider:

export FACEXLIB_MODELPATH="$BASE_DIR/.cache/facexlib"
mkdir -p "$FACEXLIB_MODELPATH/weights"

5. Documentation: License Clarity (Low Priority)

Location: docs/pulid-setup.md:163

| AntelopeV2 | [MonsterMMORPG/tools](https://huggingface.co/MonsterMMORPG/tools) | Non-commercial |

The "Non-commercial" license should be verified and more specific. Some users may need to know the exact license terms.

Recommendation: Add a note about verifying license compatibility for commercial use.

---

🧪 Test Coverage

The PR description mentions:

• ✅ Build succeeds on macOS (aarch64-darwin)
• ✅ All flake checks pass
• ✅ InsightFace imports successfully without mxnet
• ✅ FaceXLib models download to cache directory
• ✅ PuLID workflow executes successfully

Suggestion: Consider adding these as automated checks if possible, especially for the import test.

---

📝 Code Style

The code follows the project's style guidelines well:

• ✅ Proper Nix formatting (suggest running nix fmt to confirm)
• ✅ Good comments explaining the technical decisions
• ✅ Shell script uses proper error handling with set -euo pipefail

---

🎯 Summary

Recommendation: Approve with minor suggestions

This PR successfully achieves its goal of enabling InsightFace/PuLID on macOS Apple Silicon. The core implementation is sound. The suggestions above are mostly defensive improvements around:

1. Security (checksum verification)
2. Robustness (patch verification)
3. Documentation clarity

None of these are blocking issues, but addressing them would make the implementation more production-ready.

Priority fixes (if any):

1. Add checksum verification to the download script
2. Verify the facexlib directory structure is correct

Great work on this cross-platform enablement! 🎉

[claude]

Code Review: InsightFace/PuLID macOS Apple Silicon Support

Summary

This PR successfully enables InsightFace and PuLID custom nodes on macOS Apple Silicon by removing the MXNet dependency and using ONNX Runtime. The implementation is well-designed and follows Nix best practices.

---

Strengths

1. Excellent Technical Approach ✅

• Smart dependency override: Removing MXNet from insightface while preserving ONNX Runtime functionality is exactly the right approach. MXNet was only used for one legacy CLI command (rec_add_mask_param.py) that isn't needed for face analysis.
• Platform-agnostic solution: Using ONNX Runtime makes this work across all platforms (macOS, Linux) without platform-specific hacks.
• Cache redirection: The FACEXLIB_MODELPATH environment variable approach elegantly solves the read-only Nix store problem.

2. Clean Code Quality ✅

• Nix overrides are minimal and well-documented with clear comments explaining the why
• The facexlib patch using sed in postInstall is straightforward and maintainable
• Environment variables (TORCH_HOME, HF_HOME, FACEXLIB_MODELPATH) are properly set in the launcher
• All cache directories are created before use

3. Excellent Documentation ✅

• docs/pulid-setup.md is comprehensive with clear examples for macOS, Linux, and custom directories
• The download script has proper help text with -h/--help flag
• Comments in code explain the technical rationale (e.g., why MXNet is removed)
• Proper model source attribution with licenses

4. User-Friendly Tooling ✅

• The download-pulid-models.sh script is well-designed with:
  • Color-coded output for better UX
  • Idempotent downloads (skips existing models)
  • Platform detection for default directories
  • Clear error messages and dependency checks

---

Issues & Recommendations

1. Security: facexlib Patch Robustness ⚠️ Medium Priority

Issue: The sed command in nix/python-overrides.nix:198 assumes a specific line format and could silently fail if the facexlib package changes its structure.

sed -i 's|^ROOT_DIR = os.path.dirname.*|_DEFAULT_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))\nROOT_DIR = os.environ.get("FACEXLIB_MODELPATH", _DEFAULT_ROOT)|' "$miscPy"

Recommendations:

1. Add validation after the sed to ensure it worked:

if \! grep -q 'FACEXLIB_MODELPATH' "$miscPy"; then
  echo "ERROR: Failed to patch facexlib - package structure may have changed"
  exit 1
fi

2. Consider adding a test in nix/checks.nix to verify the patch was applied:

facexlib-patch-check = pkgs.runCommand "check-facexlib-patch" {} ''
  if \! grep -q FACEXLIB_MODELPATH ${python3.pkgs.facexlib}/${python3.sitePackages}/facexlib/utils/misc.py; then
    echo "facexlib patch failed"
    exit 1
  fi
  touch $out
'';

2. Script Error Handling ⚠️ Low Priority

Issue: In scripts/download-pulid-models.sh, curl failures could leave partial downloads.

Current code (line 104):

curl -L "$PULID_URL" -o "$PULID_MODEL" --progress-bar

Recommendation: Add -f flag to fail on HTTP errors and use temp files:

TEMP_FILE="${PULID_MODEL}.tmp"
curl -fL "$PULID_URL" -o "$TEMP_FILE" --progress-bar
mv "$TEMP_FILE" "$PULID_MODEL"

This prevents corrupted partial downloads from being left behind.

3. Potential Platform Edge Case ℹ️ Low Priority

Issue: The insightface override (line 215) uses:

dependencies = builtins.filter (dep: dep.pname or "" \!= "mxnet") (old.dependencies or [ ]);

This works well, but if old.dependencies doesn't exist, it falls back to an empty list which could miss other required dependencies.

Recommendation: Consider adding a check or comment explaining this is safe because insightface's other dependencies are already declared in nixpkgs.

4. Missing Directory in Script ℹ️ Very Low Priority

Issue: The download script creates models/pulid and models/insightface/models/antelopev2, but the launcher also creates several model subdirectories (line 263 in packages.nix). Consider documenting this or having the script create the full structure.

Recommendation: Either:

1. Document in pulid-setup.md that other directories are auto-created by ComfyUI
2. Or have the script create the full structure to match the launcher

---

Best Practices Adherence

✅ Follows CLAUDE.md guidelines:

• Uses writeShellApplication best practices (the launcher already does this)
• Pins versions in nix/versions.nix (facexlib is vendored)
• Proper comments explaining "why" not just "what"
• Nix formatting compatible with nix fmt

✅ Security:

• No hardcoded credentials or secrets
• Downloads from reputable sources (HuggingFace)
• Proper license attribution in docs
• Read-only Nix store is properly maintained

✅ Code Style:

• Bash script uses proper set -euo pipefail
• Clear variable naming (PULID_MODEL, ANTELOPE_DIR)
• Proper indentation (4 spaces in comments match repo style)

---

Testing Recommendations

Since this enables a new platform (macOS), consider adding:

1. CI test for macOS builds (if not already in CI):

- name: Test macOS build
  if: matrix.platform == 'macos'
  run: |
    nix build
    nix run . -- --help

2. Integration test for insightface import:

nix develop -c python -c 'import insightface; from insightface.app import FaceAnalysis; print("OK")'

3. Verify cache redirection works as expected:

# Test that FACEXLIB_MODELPATH is respected
export FACEXLIB_MODELPATH=/tmp/test-facexlib
nix run . -- --help  # Should create /tmp/test-facexlib/facexlib/weights

---

Performance Considerations

✅ No concerns:

• The changes don't affect runtime performance
• Model downloads are one-time operations
• ONNX Runtime provides efficient inference on all platforms

---

Conclusion

This is a high-quality PR that successfully achieves its goal. The code is clean, well-documented, and follows Nix best practices. The only issues are minor robustness improvements that could prevent future breakage.

Verdict: APPROVE ✅

Recommended actions before merge:

1. Add validation to the facexlib sed patch (medium priority)
2. Improve curl error handling in download script (nice-to-have)
3. Consider adding a basic macOS CI test if not already present

Great work on enabling cross-platform support! 🎉