Skip to content

Conversation

mcmonkey4eva
Copy link
Contributor

@mcmonkey4eva mcmonkey4eva commented Oct 4, 2024

Reduces the risk of errors like this:
image

By avoiding makings call to github API if the frontend is already downloaded locally for a specific version anyway. Only not-yet-installed versions or latest still makes github API calls this way.
The check is a very simple - if you're not default, and have a version starting with "v", you want a specific frontend version, so we can just check that specific folder exists, and use it if so.

Prior to this PR, this type of error could potentially happen at any time when launching comfy, from either GitHub API limits like the user above saw, or from network connectivity issues, or etc.
The API limit error risk was increased by the fact that the existing code reads a list of all versions by making multiple API calls in sequence, ie it may eat more API calls that needed. There's potential for further improvement here by way of removing that full-list-read and replacing it with a direct single version check.
Note that a fresh install with an undownloaded specific version still needs to connect to github to load that version, so that users actual case is not necessarily cured other than by waiting for the api limit to drop.

Also reports a notice in logs when github calls are happening (feels weird that that was happening silently before now)

Also moves the download to a folder with a .tmp suffix until everything's extracted as otherwise you can CTRL+C the comfy window while it's downloading and get stuck in a state where comfyui thinks its has the frontend downloaded but it doesn't actually.

I tested - downloader code handles this case well already
(also rmdir was wrong func anyway, needed shutil.rmtree if it had content)
@mcmonkey4eva mcmonkey4eva added Bug Something is confirmed to not be working properly. Good PR This PR looks good to go, it needs comfy's final review. labels Oct 4, 2024
@comfyanonymous comfyanonymous merged commit c695c4a into comfyanonymous:master Oct 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is confirmed to not be working properly. Good PR This PR looks good to go, it needs comfy's final review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants