Skip to content

Conversation

@jiazhangx
Copy link

The current code logs exceptions with logger.error(e) before rethrowing them via raise e, which has issues:

  1. The SDK's internal error logs, as intermediate-layer records, may duplicate users' upper-level logs, causing redundancy. Such
    logging should be handled by users themselves.
  2. logger.error(e) may print empty logs.
image

It is recommended to remove these logs or lower their level to debug.

@gemini-code-assist
Copy link

Summary of Changes

Hello @jiazhangx, 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 refines the SDK's exception handling mechanism by eliminating redundant error logging. Previously, exceptions were logged internally before being re-raised, leading to potential duplication with user-level logs and occasional empty log messages. The change ensures that the SDK focuses on re-throwing exceptions for upstream handling, improving the clarity and efficiency of error reporting without generating unnecessary internal log noise.

Highlights

  • Redundant Error Logging: Removed logger.error(e) calls from exception handlers before re-raising the exception. This change prevents duplicate log entries for users and addresses instances where logger.error(e) might print empty logs, streamlining error reporting within the SDK.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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

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

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes redundant error logging from an exception handler in dashscope/api_entities/http_request.py, which is a good practice for an SDK as it leaves logging decisions to the application developer. The change is correct and aligns with the goal described.

While the change is correct, it's incomplete. The same pattern of logging an error and then re-raising it exists in other places. For consistency and to fully resolve the issue, I recommend applying the same fix to:

  • The _handle_aio_request method in the same file (dashscope/api_entities/http_request.py at lines 163-168).
  • The _handle_request method in dashscope/api_entities/aiohttp_request.py (at lines 237-242).

Additionally, during this refactoring, it would be a good opportunity to review the use of except BaseException and consider changing it to except Exception to avoid catching system-exiting exceptions like KeyboardInterrupt, which is a common best practice in Python.

@kevinlin09
Copy link
Collaborator

Thank you for your pull request. We added logging when exceptions occur in the SDK to provide clearer error information and prevent users from missing critical details in cases where they haven't added their own logging—otherwise, relevant error logs might be absent entirely.

Additionally, we noticed your observation about the logged messages appearing empty, which could indeed indicate an issue. Could you please help by providing a way to reproduce this behavior, or even better, submit a direct fix? Thank you!

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