[Partner Nodes] feat(FAL): add Patina model (textures)#14678
Closed
bigcat88 wants to merge 1 commit into
Closed
Conversation
bigcat88
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughA new file, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_api_nodes/nodes_patina.py`:
- Around line 68-70: The _download_rgb helper currently calls
download_url_as_bytesio without any timeout, so a stalled fetch can hang the
node indefinitely. Update _download_rgb in nodes_patina.py to pass an explicit
timeout when calling download_url_as_bytesio, using a bounded value appropriate
for media downloads, and keep the existing RGB tensor conversion behavior
intact. If needed, align the change with download_url_as_bytesio in
util/download_helpers.py so this helper always enforces a finite network wait.
- Around line 309-321: The PATINA price badge expression in the price badge
configuration currently omits the prompt expansion surcharge, so the displayed
cost is too low when that option is enabled. Update both PATINA badge
expressions in nodes_patina.py to account for the prompt_expansion toggle
alongside the existing image_size and upscale_factor logic, and add the
documented ~$0.0025 increment when prompt_expansion is on. Use the existing
PATINA badge setup and related symbols like IO.PriceBadgeDepends, PATINA_MAPS,
and the badge expr string so the change stays consistent across both badges.
- Around line 47-64: The follow-up status and result requests in _run_patina are
hardcoded to the Patina queue path instead of using the provided model_id.
Update the poll_op and final sync_op ApiEndpoint paths to build from model_id,
consistent with the initial submit call, so each PATINA node checks the correct
request queue and result resource for its own 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c75bb9a-9223-4f03-b1f1-b53e440a3de8
⛔ Files ignored due to path filters (1)
comfy_api_nodes/apis/patina.pyis excluded by!comfy_api_nodes/apis/**
📒 Files selected for processing (1)
comfy_api_nodes/nodes_patina.py
bigcat88
force-pushed
the
feat/api-nodes/fal-patina
branch
from
0055613 to
e8095cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
comfy_api_nodes/nodes_patina.py (1)
208-212: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOffload new synchronous image preprocessing from async execution.
The new async nodes call synchronous tensor resize/convert helpers directly on the event loop for large image inputs. Consider
asyncio.to_thread()or the project’s equivalent executor around these preprocessing steps. Based on learnings, new synchronous CPU/IO-heavy helpers insideasync def executeshould be offloaded.Also applies to: 370-373
🤖 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_api_nodes/nodes_patina.py` around lines 208 - 212, The async node execution paths are running CPU-heavy synchronous preprocessing on the event loop, so offload those helpers before calling upload_image_to_comfyapi. Update the execute methods that use downscale_image_tensor_by_max_side and validate_image_dimensions to run them via asyncio.to_thread() or the project’s executor helper, while keeping the async upload step unchanged. Apply the same fix to the other execute block referenced by the review so all new sync image preprocessing is moved off the event loop.Source: Learnings
🤖 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_api_nodes/nodes_patina.py`:
- Around line 303-309: The backend defaults for prompt_expansion are
inconsistent with the UI and can unintentionally enable external prompt
expansion when API callers omit the field. Update both material node execute()
methods in nodes_patina.py so prompt_expansion defaults to False, matching the
IO.Boolean.Input default and keeping expansion off unless explicitly requested.
Check the execute signatures for the affected node classes and make the default
values consistent across all referenced prompt_expansion call sites.
- Around line 130-138: The backend defaults for roughness, metalness, and height
are inconsistent with the UI helper defaults in _map_toggle_inputs, causing
omitted booleans to request extra maps. Update all affected execute() signatures
so roughness, metalness, and height default to False to match the widget
behavior and keep backend output/cost aligned with the displayed defaults.
---
Nitpick comments:
In `@comfy_api_nodes/nodes_patina.py`:
- Around line 208-212: The async node execution paths are running CPU-heavy
synchronous preprocessing on the event loop, so offload those helpers before
calling upload_image_to_comfyapi. Update the execute methods that use
downscale_image_tensor_by_max_side and validate_image_dimensions to run them via
asyncio.to_thread() or the project’s executor helper, while keeping the async
upload step unchanged. Apply the same fix to the other execute block referenced
by the review so all new sync image preprocessing is moved off the event loop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c599c4b4-77e7-481d-9f47-981fc1569bbe
⛔ Files ignored due to path filters (1)
comfy_api_nodes/apis/patina.pyis excluded by!comfy_api_nodes/apis/**
📒 Files selected for processing (1)
comfy_api_nodes/nodes_patina.py
Signed-off-by: bigcat88 <bigcat88@icloud.com>
bigcat88
force-pushed
the
feat/api-nodes/fal-patina
branch
from
e8095cd to
9e0cc8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_api_nodes/nodes_patina.py`:
- Around line 75-90: The fallback-to-black behavior in _map_outputs and
_base_texture hides missing PATINA artifacts that should be treated as failures.
Update the request/result flow so the selected maps list used by the node is
passed into _map_outputs(), then validate the PatinaResult against those
requested outputs and raise an error when an expected map or base texture is
missing instead of silently returning a 1x1 tensor. Keep the placeholder only
for intentionally disabled sockets, and apply the same fail-fast handling in the
node path that calls _download_rgb and _base_texture.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5984f188-c282-4bf7-9298-b5f17321d612
⛔ Files ignored due to path filters (1)
comfy_api_nodes/apis/patina.pyis excluded by!comfy_api_nodes/apis/**
📒 Files selected for processing (1)
comfy_api_nodes/nodes_patina.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
2 participants
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.
This PR adds three new nodes, one for each of FAL's three PATINA endpoints.
API Node PR Checklist
Scope
Pricing & Billing
If Need pricing update:
QA
Comms