Comfy-Org / comfy-cli

Command Line Interface for Managing ComfyUI
https://docs.comfy.org/comfy-cli/getting-started
GNU General Public License v3.0
291 stars 47 forks source link

Fix for hanging when run in an off-line environment #183

Open jason-weirather opened 2 months ago

jason-weirather commented 2 months ago

Proposed fix for the issue where running in an off-line environment hangs on check_for_updates.

Adds a default 10 second timeout to the request Shortened to 3 second timeout in the launcher and env command calls where check_for_updates is not a requirement.

Consistent with the previous request code, if it times out it will fail quietly and return false and the current version.

Issue being addressed: https://github.com/Comfy-Org/comfy-cli/issues/175

Also added PEP 585 type hints to functions in the updates module.

telamonian commented 2 months ago

Overall LGTM! Just one small nit with emitting a warning instead of failing silently on pypi request timeout/failure

jason-weirather commented 2 months ago

@telamonian Thank you for you for the review, I noticed the package does have a logging module so I have propose a change that uses that. While testing, I ran into another hang in my testing environment that I think uncovered a bug and try to address that.

Summary of Changes:

  1. Logging:

    • I added a logger.warning() call in place of silent failures for the timeout handling.
  2. Testing Environment:

    • During testing, I isolated the system by creating an internal network to simulate a no-internet environment:
      docker network create --internal no-internet

      This revealed that the code was hanging when trying to reach out to the Mixpanel tracking module (mp.track). I confirmed that the comfy env command had tracking disabled, but the tracking events were still being fired, causing the hang.

  3. Tracking Issues:

    • I identified two issues:
      1. mp.track Hanging: The mp.track function can hang indefinitely if it cannot communicate with the tracking server. Adding a timeout to mp.track would be a solid improvement, but for now, I focused on ensuring it doesn't run when tracking is disabled.
      2. Tracking Variable check always evaluates to enabled: Configuration manager returns a string, and the tracking module checks this as if it were a bool if not enable_tracking:.
  4. Fixes Applied:

    • I updated the configuration management module to ensure proper handling of boolean values:
      • The get function now has a type_cast argument, defaulting to str, but can cast to bool or other types as needed.
      • The set function now ensures that all values are stored as strings (since the configuration module works with strings).
      • If the configuration value is None, it is returned immediately as None, preserving the current behavior where the user is prompted to confirm tracking preferences on first use.

Apologies for the 11 commits, I kept thinking I had this sorted then found a little more thread.