[v4] Perf(cnr): Optimize ComfyRegistry fetching using incremental caching#3048
[v4] Perf(cnr): Optimize ComfyRegistry fetching using incremental caching#3048craftingmod wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR changes CNR synchronization to use ChangesCNR sync/cache refactor
Sequence Diagram(s)sequenceDiagram
participant UnifiedManager.reload
participant get_cnr_data
participant RegistryAPI
participant CacheFile
UnifiedManager.reload->>get_cnr_data: sync_mode=cache_mode
get_cnr_data->>CacheFile: load cached JSON
alt cache usable
CacheFile-->>get_cnr_data: cached nodes
else cache needs refresh
get_cnr_data->>RegistryAPI: GET /nodes pages with timestamp filter
RegistryAPI-->>get_cnr_data: node pages
get_cnr_data->>CacheFile: save_to_cache(metadata and nodes)
end
get_cnr_data-->>UnifiedManager.reload: cnrs
I patched the cache with a sync-mode charm, 🚥 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 |
This comment was marked as resolved.
This comment was marked as resolved.
✅ Action performedReview finished.
|
![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 `@comfyui_manager/common/cnr_utils.py`:
- Around line 176-177: The common CNR fetching path is still coupled to
comfyui_manager.glob.manager_core just to read verbose, which breaks
independence for legacy.manager_core.reload(). Update the shared fetching logic
in cnr_utils.fetch_all() so it no longer imports glob.manager_core; instead,
pass verbose into fetch_all() from the caller or read it from a common config
helper used by both legacy.manager_core and glob.manager_core. Keep the change
localized around fetch_all and any callers that currently rely on its implicit
config access.
- Around line 154-170: The cache-mode path in the CNR registry refresh logic
only flips is_cache_loading and never uses the module-level lock, so concurrent
callers can all miss the cache and refresh /nodes in parallel. Update the
refresh-and-save flow in the cache handling around the existing cached_data
checks to run inside async with lock, and re-check cached_data plus expiration
after acquiring the lock before fetching or writing. Apply the same
synchronization pattern to the related refresh path referenced by the other
affected block so duplicate registry refreshes are prevented consistently.
- Around line 123-133: In get_cnr_data() and the cached_data invalidation branch
in cnr_utils.py, local mode is being treated like a hard cache miss when the
registry cache is stale or the environment changes. Update the logic so
sync_mode='local' preserves and returns the existing stale cached_data instead
of clearing full_nodes/cached_data, while keeping the current invalidation
behavior for non-local modes. Use the existing get_cnr_data(), cached_data,
full_nodes, and force_refresh_days checks to locate and gate this fallback.
🪄 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: aeffccac-423d-4cda-a461-fbed80f16843
📒 Files selected for processing (3)
comfyui_manager/common/cnr_utils.pycomfyui_manager/glob/manager_core.pycomfyui_manager/legacy/manager_core.py
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 (2)
comfyui_manager/common/cnr_utils.py (2)
212-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t let a cache-write failure hide a successful fetch.
manager_util.save_to_cache()can raise on file open/write, and this broadexceptwill then log a sync/network failure and return stale or empty cached data even thoughfetch_all()already produced fresh nodes. Split the save path from the fetch fallback so a disk hiccup doesn’t make the fresh result vanish.🤖 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 `@comfyui_manager/common/cnr_utils.py` around lines 212 - 256, The cache save failure is being handled by the same broad exception path as fetch failures in the sync logic. In the function that calls fetch_all() and then manager_util.save_to_cache(), separate the cache write from the network fetch fallback so a save error does not discard already-fetched nodes; if save_to_cache() fails, keep and return json_obj['nodes'] instead of falling back to cached_data or [].
216-230: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winParse the timestamps before taking the max.
createdAt/created_atare RFC 3339 date-time strings, and raw lexicographic ordering only holds while every value stays in the same UTCZshape; an offset or precision change can movelast_updatedbackwards and make the next incremental fetch miss newer 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 `@comfyui_manager/common/cnr_utils.py` around lines 216 - 230, The incremental timestamp logic in cnr_utils.py should compare real datetimes instead of raw timestamp strings, because get_node_timestamp values like createdAt/created_at may not sort correctly lexicographically once formats vary. Update the timestamp collection and max_timestamp selection in the block using get_node_timestamp, max, and datetime.fromisoformat so each value is parsed to a datetime first, then choose the latest one and format it back for new_timestamp. Keep the existing fallback behavior for parse failures and when no timestamps are present.
🤖 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 `@comfyui_manager/common/cnr_utils.py`:
- Around line 212-256: The cache save failure is being handled by the same broad
exception path as fetch failures in the sync logic. In the function that calls
fetch_all() and then manager_util.save_to_cache(), separate the cache write from
the network fetch fallback so a save error does not discard already-fetched
nodes; if save_to_cache() fails, keep and return json_obj['nodes'] instead of
falling back to cached_data or [].
- Around line 216-230: The incremental timestamp logic in cnr_utils.py should
compare real datetimes instead of raw timestamp strings, because
get_node_timestamp values like createdAt/created_at may not sort correctly
lexicographically once formats vary. Update the timestamp collection and
max_timestamp selection in the block using get_node_timestamp, max, and
datetime.fromisoformat so each value is parsed to a datetime first, then choose
the latest one and format it back for new_timestamp. Keep the existing fallback
behavior for parse failures and when no timestamps are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b5edcdff-bcfa-412b-9c98-7d080e80a0ef
📒 Files selected for processing (4)
comfyui_manager/common/cnr_utils.pycomfyui_manager/common/manager_util.pycomfyui_manager/glob/manager_core.pycomfyui_manager/legacy/manager_core.py
1 participant
Summary
Implentation of #3031 in
manager-v4branch.v4is still fetching from zero without any cache on startup, just hiding logging.