Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jason-weirather
Copy link

@jason-weirather jason-weirather commented Sep 14, 2024

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:
#175

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

… set optionally. added PEP 585 type hints to functions in module.
@@ -21,24 +30,37 @@ def check_for_newer_pypi_version(package_name, current_version):

return False, current_version
except requests.RequestException:
# print(f"Error checking latest version: {e}")
# Fail quietly on timeout or any request exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Fail quietly on timeout or any request exception
print(f"Warning: unable to fetch {package_name} version metadata from Pypi. Retaining current version {current_version}")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably shouldn't fail silently

@telamonian
Copy link
Member

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

@jason-weirather
Copy link
Author

jason-weirather commented Sep 18, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants