-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add exception chaining #37423
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
base: master
Are you sure you want to change the base?
Add exception chaining #37423
Conversation
- Add 'from e' to exception re-raises in CloudSQLEnrichmentHandler - Add exception chaining in processes.py for OSError and CalledProcessError - Improve logging in core.py to preserve traceback context This improves debuggability by preserving the full exception chain, following Python PEP 3134 best practices. Fixes apache#37422
Summary of ChangesHello @shaheeramjad, 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 significantly improves the debuggability of the Python SDK by implementing explicit exception chaining ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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
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 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
|
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
assign set of reviewers |
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
assign to next reviewer |
|
HI @damccorm, |
| except Exception as e: | ||
| transaction.rollback() | ||
| raise RuntimeError(f"Database operation failed: {e}") | ||
| raise RuntimeError(f"Database operation failed: {e}") from e |
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.
If we're chaining the exception, do we still need to include the exception in the RuntimeError message?
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.
Same question exists below in this file as well
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.
With from e, the cause is in cause and the traceback, so we don’t need to include the exception in the message.
I kept it in because in many environments only the top-level exception message is logged or shown (e.g. in alerts or dashboards). In those cases, "Database operation failed" alone doesn’t tell you the real error, while f"Database operation failed: {e}" stays useful. It’s also a common Python pattern to add context and still include the original error in the wrapper message.
If you prefer to rely on the chain and keep messages minimal, I’m happy to change to e.g. raise RuntimeError("Database operation failed") from e and remove {e} from the message. Happy to follow whatever convention you prefer for this project.
Description
This PR adds exception chaining (
raise ... from) throughout the Python SDK to preserve error context and improve debuggability. When exceptions are re-raised without chaining, the original traceback context is lost, making it difficult to identify root causes of failures.Changes Made
CloudSQLEnrichmentHandler (
transforms/enrichment_handlers/cloudsql.py)from eto exception re-raises in_execute_query()methodProcess Utilities (
utils/processes.py)OSErrorandCalledProcessErrorincall(),check_call(), andcheck_output()functionsCore Transforms (
transforms/core.py)exc_info=Truewhen failure callbacks failBenefits
Example
Before:
After:
Now when this fails, developers see:
Testing
Checklist
raise ... from) throughout codebase to preserve error context #37422)CHANGES.mdwith noteworthy changesRelated
.pylintrcline 139:#TODO(https://github.com/apache/beam/issues/21169)raise ... from) throughout codebase to preserve error context #37422