-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: main
Are you sure you want to change the base?
Fix for hanging when run in an off-line environment #183
Conversation
… set optionally. added PEP 585 type hints to functions in module.
comfy_cli/update.py
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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}") |
There was a problem hiding this comment.
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
Overall LGTM! Just one small nit with emitting a warning instead of failing silently on pypi request timeout/failure |
…g default settings this warning should be displayed
…he fully offline mode since its getting hit dispite disabiling tracking
…stants to a string which is assumed to a bool causing it to always evaluate to true
…uration, so don't cast to a bool when checking that
@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:
Apologies for the 11 commits, I kept thinking I had this sorted then found a little more thread. |
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.