[v3] Perf(cnr): Optimize ComfyRegistry startup loading using incremental caching#3031
[v3] Perf(cnr): Optimize ComfyRegistry startup loading using incremental caching#3031craftingmod wants to merge 13 commits into
Conversation
📝 WalkthroughWalkthroughRefactors CNR fetching to use ChangesCNR Fetch Refactor and Cache Generalization
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@glob/cnr_utils.py`:
- Around line 140-142: The query string in the nodes request is built via raw
interpolation, so dynamic values like comfyui_ver and timestamp_filter can be
sent incorrectly when they contain reserved characters. Update the sub_uri
construction in the related fetch logic to build the query via urlencode (or
equivalent parameter encoding) for page, limit, comfyui_version, form_factor,
and timestamp, keeping the existing request path and ensuring the encoded values
come from the same variables used in the current block.
- Line 157: The fetch_all coroutine is using a blocking sleep inside async code,
which pauses the event loop between pages. Update the sleep call in fetch_all to
use asyncio’s non-blocking sleep instead of time.sleep, and make sure the
function already imports and uses asyncio appropriately so other startup/reload
tasks can continue running.
🪄 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 Plus
Run ID: e8894998-b981-4fbb-b21c-eedc0b4e3c7d
📒 Files selected for processing (3)
glob/cnr_utils.pyglob/manager_core.pyglob/manager_util.py
…rameter encoding in fetch_all
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
glob/cnr_utils.py (1)
124-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
[]here, not{}.manager_core.reloaditerates this as a list of node dicts, so an empty dict turns into string keys and can break reload on a cache miss.🤖 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 `@glob/cnr_utils.py` around lines 124 - 133, The cache-miss branch in normalized_mode == 'cache' currently returns an empty dict instead of the list shape expected by manager_core.reload. Update the logic in the cache handling path near get_cache_state so that the dont_wait/no cached_data case returns an empty list, matching the nodes list returned by cached_data.get('nodes', []). Keep the behavior consistent across this branch and the surrounding cache loading flow in cnr_utils.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 124-133: The cache-miss branch in normalized_mode == 'cache'
currently returns an empty dict instead of the list shape expected by
manager_core.reload. Update the logic in the cache handling path near
get_cache_state so that the dont_wait/no cached_data case returns an empty list,
matching the nodes list returned by cached_data.get('nodes', []). Keep the
behavior consistent across this branch and the surrounding cache loading flow in
cnr_utils.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 182793d0-2f69-4e42-9184-da6b5bb7646d
📒 Files selected for processing (1)
glob/cnr_utils.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
glob/cnr_utils.py (2)
124-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear
is_cache_loadingbefore every cache-mode return.Line 125 sets the global flag before the
dont_waitand fresh-cache fast paths, but those returns happen before thefinallyat Line 206. After any fast return, the manager can look permanently “loading” — a sticky little imp.Proposed fix
if normalized_mode == 'cache': - is_cache_loading = True - if dont_wait: if cached_data is not None: return cached_data.get('nodes', []) return [] if cached_data is not None and manager_util.get_cache_state(uri, expired_days=1) == 'cached': return cached_data.get('nodes', []) + is_cache_loading = True + async def fetch_all(timestamp_filter, existing_nodes):Also applies to: 206-208
🤖 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 `@glob/cnr_utils.py` around lines 124 - 133, The cache-mode fast paths in cnr_utils should clear the global is_cache_loading flag before returning, since the early returns in the normalized_mode == 'cache' branch bypass the later finally cleanup. Update the cache handling around the dont_wait and fresh cached_data checks so the flag is reset before each return, using the existing is_cache_loading and manager_util.get_cache_state flow to locate the fix.
175-191: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not advance incremental cache timestamps with the local clock.
When no node timestamp is available, Line 191 persists
datetime.now(...)aslast_updated. If the client clock is ahead of the registry server, the nexttimestampquery can skip updates created before that local time. Keep the previous timestamp, or leave it unset, until a node/server timestamp is observed.Proposed fix
- from datetime import datetime, timezone, timedelta + from datetime import datetime, timedelta json_obj = await fetch_all(last_updated, full_nodes) @@ else: - new_timestamp = last_updated if last_updated else datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + new_timestamp = last_updatedAlso applies to: 193-199
🤖 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 `@glob/cnr_utils.py` around lines 175 - 191, The cache timestamp fallback in the fetch/update flow should not use the local clock when no node timestamp is available. Update the logic around fetch_all, get_node_timestamp, and the new_timestamp assignment so it preserves last_updated (or leaves it unset) until a registry-derived timestamp is seen, instead of calling datetime.now(...). Also keep the existing max_timestamp adjustment path intact for observed node timestamps.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 124-133: The cache-mode fast paths in cnr_utils should clear the
global is_cache_loading flag before returning, since the early returns in the
normalized_mode == 'cache' branch bypass the later finally cleanup. Update the
cache handling around the dont_wait and fresh cached_data checks so the flag is
reset before each return, using the existing is_cache_loading and
manager_util.get_cache_state flow to locate the fix.
- Around line 175-191: The cache timestamp fallback in the fetch/update flow
should not use the local clock when no node timestamp is available. Update the
logic around fetch_all, get_node_timestamp, and the new_timestamp assignment so
it preserves last_updated (or leaves it unset) until a registry-derived
timestamp is seen, instead of calling datetime.now(...). Also keep the existing
max_timestamp adjustment path intact for observed node timestamps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f26781a7-7a0b-42e1-bdc8-aab687bae516
📒 Files selected for processing (1)
glob/cnr_utils.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
glob/cnr_utils.py (1)
66-72: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftUse the server update timestamp for the incremental cursor.
This helper only returns creation timestamps, but the sync fetches nodes created or updated since
last_updated. Updates to existing nodes whose creation time predates the cursor can be re-fetched forever, keeping startup sync slower than intended—an impish cache loop. Prefer the API’s updated timestamp field, falling back to creation time only when absent.🤖 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 `@glob/cnr_utils.py` around lines 66 - 72, The get_node_timestamp helper is using only creation timestamps, which breaks the incremental cursor for updated nodes. Update get_node_timestamp to prefer the server’s updated timestamp from latest_version when available, and only fall back to createdAt/created_at if the update time is missing. Make sure the sync path that relies on this helper continues to use the same function so the cursor advances on both created and updated nodes.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 66-72: The get_node_timestamp helper is using only creation
timestamps, which breaks the incremental cursor for updated nodes. Update
get_node_timestamp to prefer the server’s updated timestamp from latest_version
when available, and only fall back to createdAt/created_at if the update time is
missing. Make sure the sync path that relies on this helper continues to use the
same function so the cursor advances on both created and updated nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9917baac-312e-453f-b359-fea95587f59f
📒 Files selected for processing (1)
glob/cnr_utils.py
This comment was marked as resolved.
This comment was marked as resolved.
|
I understand that v3 is in maintenance mode and no longer accepting new feature updates. |
* for github star, download counts.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
glob/cnr_utils.py (1)
29-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLet
asyncio.TimeoutErrorbubble past_get_cnr_data.
glob/cnr_utils.py:223catches the timeout raised byasyncio.wait_for, soget_cnr_data()never reaches its cache fallback. Inforcemode, that turns a timeout into[]instead of falling back to cache — a tiny timeout gremlin.🤖 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 `@glob/cnr_utils.py` around lines 29 - 31, The timeout handling in _get_cnr_data is swallowing asyncio.TimeoutError too early, preventing get_cnr_data from reaching its cache fallback in force mode. Remove the local catch around the fetch path in _get_cnr_data (or re-raise the timeout) so the asyncio.wait_for timeout can bubble up to get_cnr_data, where the existing cache fallback logic should handle it.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 29-31: The timeout handling in _get_cnr_data is swallowing
asyncio.TimeoutError too early, preventing get_cnr_data from reaching its cache
fallback in force mode. Remove the local catch around the fetch path in
_get_cnr_data (or re-raise the timeout) so the asyncio.wait_for timeout can
bubble up to get_cnr_data, where the existing cache fallback logic should handle
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c5ee4a6-e0c6-4c75-8142-cf3089116e46
📒 Files selected for processing (1)
glob/cnr_utils.py
* Now correctly refresh cache if cache is created * Seperate cache update / cache created time * revert 7-day expiration logic
This comment was marked as resolved.
This comment was marked as resolved.
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
glob/cnr_utils.py (1)
141-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t skip initial cache creation when
dont_waitis true.Line 141 returns before
fetch_all()when there is no valid cache, so a fresh install or environment-invalidated cache returns[]and never creates the CNR cache from the defaultUnifiedManager.reload(..., dont_wait=True)path. That makes the registry vanish like an imp in a puff of smoke.🐛 Proposed fix
- if dont_wait: + if dont_wait and cached_data is not None: is_cache_loading = False - if cached_data is not None: - return cached_data.get('nodes', []) - return [] + return cached_data.get('nodes', [])🤖 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 `@glob/cnr_utils.py` around lines 141 - 145, The early return in the dont_wait branch of fetch_all is skipping initial cache population when cached_data is missing, so a fresh or invalidated install never builds the CNR cache. Update fetch_all so dont_wait still triggers the initial cache creation path and only returns immediately when an existing cache is available; use the existing fetch_all and UnifiedManager.reload flow to preserve async behavior while ensuring the cache is created at least once.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 141-145: The early return in the dont_wait branch of fetch_all is
skipping initial cache population when cached_data is missing, so a fresh or
invalidated install never builds the CNR cache. Update fetch_all so dont_wait
still triggers the initial cache creation path and only returns immediately when
an existing cache is available; use the existing fetch_all and
UnifiedManager.reload flow to preserve async behavior while ensuring the cache
is created at least once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2169d938-13d5-4d7c-a52d-a74432429b8e
📒 Files selected for processing (1)
glob/cnr_utils.py
This comment was marked as resolved.
This comment was marked as resolved.
✅ Action performedReviews resumed. |
| lock = asyncio.Lock() | ||
|
|
||
| is_cache_loading = False | ||
| force_refresh_days = 30 |
There was a problem hiding this comment.
Full-refresh in 30 days should be enough.
And if 1day is what you want, you can use 1 as everyday invalidate cache.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
glob/cnr_utils.py (1)
30-32: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the timeout fallback truly local-only.
Line 32 retries with
sync_mode='cache', but Lines 158-164 only return early whencached_dataexists; cache misses still fall through to the remote fetch path. That can turn one timeout into a second network encore—the imp gets two drums. Uselocalfallback and stop before fetching when no local cache is available.Proposed fix
except asyncio.TimeoutError: logging.error(f"[ComfyUI-Manager] A timeout occurred during the fetch process from ComfyRegistry.") - return await _get_cnr_data(sync_mode='cache', dont_wait=True) # timeout fallback + return await _get_cnr_data(sync_mode='local', dont_wait=True) # timeout fallback @@ if cached_data is not None and (sync_mode == 'local' or not is_cache_expired): is_cache_loading = False return cached_data.get('nodes', []) + + if sync_mode == 'local': + is_cache_loading = False + return []Also applies to: 154-166
🤖 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 `@glob/cnr_utils.py` around lines 30 - 32, The timeout fallback in _get_cnr_data is still able to hit the network because the retry uses sync_mode='cache', which only short-circuits when cached_data exists. Update the asyncio.TimeoutError handler to use the truly local fallback mode and ensure the local-cache path in _get_cnr_data returns immediately before any remote fetch when no cached_data is available. Keep the fix centered on _get_cnr_data and its cache handling branch so the timeout path remains offline-only.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 30-32: The timeout fallback in _get_cnr_data is still able to hit
the network because the retry uses sync_mode='cache', which only short-circuits
when cached_data exists. Update the asyncio.TimeoutError handler to use the
truly local fallback mode and ensure the local-cache path in _get_cnr_data
returns immediately before any remote fetch when no cached_data is available.
Keep the fix centered on _get_cnr_data and its cache handling branch so the
timeout path remains offline-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdfdec63-d9d5-40ec-921f-93a880ea6759
📒 Files selected for processing (1)
glob/cnr_utils.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
glob/cnr_utils.py (1)
215-224: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winParse timestamps before choosing the cursor.
max()over raw strings is only safe if every timestamp is normalized to the same precision; these API values already vary in fractional digits, so a newer value can sort behind an older one. Tiny fix, big gain—don’t let the cursor play imp.🤖 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 `@glob/cnr_utils.py` around lines 215 - 224, The cursor selection in the timestamp logic is using max() on raw timestamp strings, which can pick the wrong value when API timestamps have different fractional-second precision. Update the timestamp handling around get_node_timestamp and the max_timestamp calculation to parse each timestamp into a datetime before comparing, then choose the latest parsed value and apply the existing 10-second backoff before formatting the cursor string.
🤖 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.
Outside diff comments:
In `@glob/cnr_utils.py`:
- Around line 215-224: The cursor selection in the timestamp logic is using
max() on raw timestamp strings, which can pick the wrong value when API
timestamps have different fractional-second precision. Update the timestamp
handling around get_node_timestamp and the max_timestamp calculation to parse
each timestamp into a datetime before comparing, then choose the latest parsed
value and apply the existing 10-second backoff before formatting the cursor
string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ab97ec8-45fb-4fcf-9195-c9d245c2ed58
📒 Files selected for processing (1)
glob/cnr_utils.py
This comment was marked as resolved.
This comment was marked as resolved.
craftingmod
changed the title
1 participant
Summary
This PR significantly optimizes the startup and reload latency of the ComfyRegistry (CNR) node loader.
By replacing the full-fetch mechanism with a
server-time-based incremental sync (delta sync), it reduces the data fetched at boot time from the entire registry database (currently 4,600+ nodes across 150+ pages) to only new or updated nodes since the last run.This reduces the ComfyRegistry sync time on startup from ~50 seconds to less than 1 second under normal conditions.
Problem
Previously,
cnr_utils.pyperformed a full sync of the entire registry database page-by-page (limit=30, hitting the endpoint 150+ times with atime.sleep(0.5)gap) whenever it synced or when the daily cache expired. As the number of custom nodes in the registry continues to grow, this startup latency has organically degraded to over 50 seconds, creating a noticeable delay during ComfyUI boot. (Even if it runs on background, fetching process costs network request onapi.comfyui.organd not avoidable until finish.)How to implemented
https://docs.comfy.org/api-reference/registry/retrieves-a-list-of-nodes#parameter-timestamp
Integrated the
timestampparameter supported byGET https://api.comfy.org/nodesto request only nodes created or updated after the cached timestamp.Related issues
Addresses the performance issues mentioned in #2229 #1950 #1629.
Why PR
Watching every fetch of ComfyUI Nodes is painful while coding custom_nodes...
And after 1.5 years of issue is created, v4 isn't even closed to release on main branch.