Make sure errors and progress messages go to STDERR #1665
Conversation
Signed-off-by: Daniel J Walsh <[email protected]>
rhatdan
requested review from
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver and
engelmi
as code owners
Reviewer's GuideThis PR redirects all user-facing progress and error outputs from stdout to stderr by replacing print calls with perror, consolidates and enhances download logic in HttpClient with retry/backoff and checksum verification, and removes duplicate constants and download functions. Sequence diagram for download_file with retries and error handlingsequenceDiagram
participant User
participant download_file
participant HttpClient
participant console
participant verify_checksum
User->>download_file: call download_file(url, dest_path, ...)
loop up to max_retries
download_file->>HttpClient: init(url, headers, output_file, show_progress)
alt HTTPError (404/416)
download_file-->>User: raise error
else URLError
download_file->>console: error("Network Error: ...")
else TimeoutError
download_file->>console: warning("TimeoutError ...")
else RuntimeError
download_file->>console: warning("RuntimeError ...")
else IOError
download_file->>console: warning("I/O Error ...")
else Exception
download_file->>console: error("Unexpected error ...")
download_file-->>User: raise error
end
download_file->>download_file: sleep (exponential backoff)
end
alt retries exceeded
download_file-->>User: raise ConnectionError
end
Class diagram for updated HttpClient and download logicclassDiagram
class HttpClient {
+__init__()
+init(url, headers, output_file, show_progress)
+perform_download(file, show_progress)
+human_readable_time(seconds)
+set_resume_point(output_file)
+print_progress(progress_prefix, progress_bar, progress_suffix)
+update_progress(chunk_size)
+calculate_speed(now_downloaded, start_time)
}
class File
class console
class download_file {
+download_file(url: str, dest_path: str, headers: dict[str, str] | None = None, show_progress: bool = True)
}
class download_and_verify {
+download_and_verify(url: str, target_path: str, max_retries: int = 2)
}
HttpClient <.. download_file : uses
download_file <.. download_and_verify : uses
console <.. download_file : uses
console <.. download_and_verify : uses
File <.. HttpClient : uses
verify_checksum <.. download_and_verify : uses
Class diagram for perror and error output redirectionclassDiagram
class console {
+error(msg)
+warning(msg)
}
class perror {
+perror(msg, ...)
}
console <.. perror : uses
note for perror "perror replaces print for error/progress messages, outputs to stderr"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rhatdan - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `ramalama/http_client.py:154` </location>
<code_context>
+def download_file(url: str, dest_path: str, headers: dict[str, str] | None = None, show_progress: bool = True):
</code_context>
<issue_to_address>
The retry loop in download_file may not handle KeyboardInterrupt gracefully.
Explicitly handle KeyboardInterrupt to ensure a clean exit and avoid logging it as an 'Unexpected error.'
Suggested implementation:
```python
def download_file(url: str, dest_path: str, headers: dict[str, str] | None = None, show_progress: bool = True):
"""
Downloads a file from a given URL to a specified destination path.
Args:
url (str): The URL to download from.
dest_path (str): The path to save the downloaded file.
headers (dict): Optional headers to include in the request.
show_progress (bool): Whether to show a progress bar during download.
Raises:
KeyboardInterrupt: If the download is interrupted by the user.
```
```python
Raises:
"""
max_retries = 3
for attempt in range(max_retries):
try:
# ... (existing download logic)
break
except KeyboardInterrupt:
print("\nDownload interrupted by user. Exiting cleanly.")
return
except Exception as e:
if attempt < max_retries - 1:
print(f"Error occurred: {e}. Retrying ({attempt + 1}/{max_retries})...")
continue
else:
print("Unexpected error:", e)
raise
```
You will need to merge the new retry loop and exception handling with your existing download logic inside `download_file`. Make sure to place the `except KeyboardInterrupt` before the generic `except Exception` to ensure it is handled explicitly and not logged as an 'Unexpected error.'
</issue_to_address>
### Comment 2
<location> `ramalama/common.py:64` </location>
<code_context>
return True
if user_input in ["no", "n"]:
return False
- print("Invalid input. Please enter 'yes' or 'no'.")
+ perror("Invalid input. Please enter 'yes' or 'no'.")
</code_context>
<issue_to_address>
Switching to perror for user prompts may not be ideal for all user-facing messages.
Consider keeping this prompt on stdout to avoid confusing users with non-error messages on stderr.
</issue_to_address>
### Comment 3
<location> `ramalama/model.py:845` </location>
<code_context>
def print_pull_message(self, model_name):
- print(f"Downloading {model_name} ...")
- print(f"Trying to pull {model_name} ...")
+ # Write messages to stderr
+ perror(f"Downloading {model_name} ...")
+ perror(f"Trying to pull {model_name} ...")
</code_context>
<issue_to_address>
Both pull messages are now sent to stderr, which may not be necessary.
Evaluate if informational messages like 'Downloading' and 'Trying to pull' should be sent to stderr, as this may impact script output parsing. Typically, only errors or warnings are sent to stderr.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def download_file(url: str, dest_path: str, headers: dict[str, str] | None = None, show_progress: bool = True): | ||
| """ | ||
| Downloads a file from a given URL to a specified destination path. | ||
|
|
||
| Args: | ||
| url (str): The URL to download from. | ||
| dest_path (str): The path to save the downloaded file. | ||
| headers (dict): Optional headers to include in the request. | ||
| show_progress (bool): Whether to show a progress bar during download. | ||
|
|
There was a problem hiding this comment.
suggestion: The retry loop in download_file may not handle KeyboardInterrupt gracefully.
Explicitly handle KeyboardInterrupt to ensure a clean exit and avoid logging it as an 'Unexpected error.'
Suggested implementation:
def download_file(url: str, dest_path: str, headers: dict[str, str] | None = None, show_progress: bool = True):
"""
Downloads a file from a given URL to a specified destination path.
Args:
url (str): The URL to download from.
dest_path (str): The path to save the downloaded file.
headers (dict): Optional headers to include in the request.
show_progress (bool): Whether to show a progress bar during download.
Raises:
KeyboardInterrupt: If the download is interrupted by the user. Raises:
"""
max_retries = 3
for attempt in range(max_retries):
try:
# ... (existing download logic)
break
except KeyboardInterrupt:
print("\nDownload interrupted by user. Exiting cleanly.")
return
except Exception as e:
if attempt < max_retries - 1:
print(f"Error occurred: {e}. Retrying ({attempt + 1}/{max_retries})...")
continue
else:
print("Unexpected error:", e)
raiseYou will need to merge the new retry loop and exception handling with your existing download logic inside download_file. Make sure to place the except KeyboardInterrupt before the generic except Exception to ensure it is handled explicitly and not logged as an 'Unexpected error.'
ramalama/common.py
Outdated
| @@ -61,7 +54,7 @@ def confirm_no_gpu(name, provider) -> bool: | |||
| return True | |||
| if user_input in ["no", "n"]: | |||
| return False | |||
| print("Invalid input. Please enter 'yes' or 'no'.") | |||
There was a problem hiding this comment.
question: Switching to perror for user prompts may not be ideal for all user-facing messages.
Consider keeping this prompt on stdout to avoid confusing users with non-error messages on stderr.
| # Write messages to stderr | ||
| perror(f"Downloading {model_name} ...") | ||
| perror(f"Trying to pull {model_name} ...") |
There was a problem hiding this comment.
question: Both pull messages are now sent to stderr, which may not be necessary.
Evaluate if informational messages like 'Downloading' and 'Trying to pull' should be sent to stderr, as this may impact script output parsing. Typically, only errors or warnings are sent to stderr.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rhatdan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on improving the clarity and correctness of output by redirecting error and progress messages to STDERR. This change ensures that important diagnostic information is separated from regular output, making it easier to debug and monitor the application. The introduction of the perror function promotes code reuse and consistency across the codebase.
Highlights
- STDERR Usage: Ensured that all error messages and progress messages are now directed to STDERR instead of STDOUT.
- Common Function: Introduced a common
perrorfunction to handle STDERR output, replacing directprintcalls. - File Modifications: Modified multiple files including
chat.py,cli.py,common.py,engine.py,hf_style_repo_base.py,http_client.py,huggingface.py,model.py,model_store/snapshot_file.py,oci.py,ollama.py,ollama_repo_utils.py, andrag.pyto useperrorfor error and progress messages. - Code Cleanup: Removed unused imports and code blocks from
common.pyto improve code readability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request focuses on improving error handling and standardizing output streams by directing progress and error messages to stderr. It also centralizes download logic within the HttpClient class, enhancing code modularity and maintainability.
Signed-off-by: Daniel J Walsh <[email protected]>
Summary by Sourcery
Ensure all progress and error messages are written to stderr and centralize download logic with retries, checksum verification, and HTTP error handling in HttpClient
New Features:
Enhancements: