fix(aria2): resolve comfy_path via common.context in aria2_download_url - #3246
Open
ReinisLusis wants to merge 1 commit into
Open
fix(aria2): resolve comfy_path via common.context in aria2_download_url#3246ReinisLusis wants to merge 1 commit into
ReinisLusis wants to merge 1 commit into
Conversation
`aria2_download_url` did a bare `import manager_core as core`, which resolves only in the v3 flat layout, so every aria2 download fails with ModuleNotFoundError under the pip package. v4 binds `comfy_path` in `common/context.py` rather than in `manager_core`, so the package import has to reach `context`. The `except` arm is what the flat layout does today, which keeps the file correct on `main`. Refs Comfy-Org#3064
|
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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
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.
aria2_download_urlincomfyui_manager/common/manager_downloader.py:79doesimport manager_core as core. That flat import resolves only in the v3custom_nodes/layout; under the pip package the module iscomfyui_manager.glob.manager_core, so it raisesModuleNotFoundError.When it breaks. Only when
COMFYUI_MANAGER_ARIA2_SERVERis set —download_urlreachesaria2_download_urlbehind that check. For those users every model install and every CNR node install fails instantly, and aria2 is the only resumable download path Manager has.Why the one-liner in the thread is not enough.
from comfyui_manager.glob import manager_core as corefixes the import, but the next two lines readcore.comfy_path, and v4 binds that name incomfyui_manager/common/context.py, not inmanager_core. Onmanager-v4(bd4ede2)comfyui_manager.glob.manager_coreimports fine andhasattr(manager_core, 'comfy_path')isFalse, so the one-liner tradesModuleNotFoundErrorforAttributeError. The import has to reachcontext.Why
try/exceptrather than a plain package import.main:glob/manager_downloader.pyandmanager-v4:comfyui_manager/common/manager_downloader.pyare byte-identical today, and onmainthe flat import is the correct one —glob/is not a package there, so a relative import is not available either. Theexceptarm is exactly whatmaindoes now. That makes this hunk correct in both layouts, so it does not have to be revisited when the v4 tree reachesmain, and it answers the branch question left open in the thread rather than asking it again.Verified at
manager-v4bd4ede2 andmainf82970b: the patched file compiles andcomfy_pathresolves in both layouts (package arm through the realcommon/context.py, flat arm through the realglob/manager_core.py).Not addressed here: the
/modelsre-rooting a few lines below, raised as a follow-up on the same issue. That one changes behaviour for the separate-container aria2 setup documented indocs/en/use_aria2.mdand needs a decision on which topology is supported, so it is left out of this fix.Refs #3064