fix: report failed task.#315
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
Summary of ChangesHello @k82cn, 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 system's ability to handle and report task failures originating from exceptions within services. By ensuring that detailed error messages are captured and logged when a task fails, it enhances debugging capabilities and provides clearer insights into the root cause of issues. A new end-to-end test validates this improved error handling, alongside minor code quality enhancements for the e2e test suite. Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request improves error handling by ensuring that when a task fails due to an exception in the service, it is correctly reported with a FAILED state and the corresponding error message is captured in the task's events. Previously, this would result in an internal error. The changes involve modifying the gRPC shim to handle non-zero return codes by creating a TaskResult with a Failed state, and updating the storage layer to record the error message from failed tasks into the event log. To validate this fix, a new end-to-end test has been added, which uses a dedicated test service that intentionally raises an exception.
My review includes a couple of suggestions for the new test code to improve its robustness and readability, such as avoiding redundant conditions and replacing unsafe bare except blocks.
| try: | ||
| sessions = flamepy.list_sessions() | ||
| for sess in sessions: | ||
| try: | ||
| if sess.application == FLM_ERROR_SVC_APP: | ||
| flamepy.close_session(sess.id) | ||
| except: | ||
| pass | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Using bare except: pass is a dangerous pattern as it silences all exceptions, including system-exiting ones like KeyboardInterrupt and SystemExit. This can hide significant issues during test teardown and make debugging very difficult. It's recommended to catch specific exceptions (e.g., flamepy.FlameError) or at least Exception to avoid unintentionally suppressing critical errors. Consider logging the exception instead of just passing.
| try: | |
| sessions = flamepy.list_sessions() | |
| for sess in sessions: | |
| try: | |
| if sess.application == FLM_ERROR_SVC_APP: | |
| flamepy.close_session(sess.id) | |
| except: | |
| pass | |
| except: | |
| pass | |
| try: | |
| sessions = flamepy.list_sessions() | |
| for sess in sessions: | |
| try: | |
| if sess.application == FLM_ERROR_SVC_APP: | |
| flamepy.close_session(sess.id) | |
| except Exception: | |
| pass | |
| except Exception: | |
| pass |
| error_events = [ | ||
| e | ||
| for e in refreshed_task.events | ||
| if e.code == flamepy.TaskState.FAILED or e.code == 3 |
There was a problem hiding this comment.
The condition e.code == flamepy.TaskState.FAILED or e.code == 3 is redundant. flamepy.TaskState.FAILED is an IntEnum member with a value of 3. Using only the enum member is sufficient and improves readability.
| if e.code == flamepy.TaskState.FAILED or e.code == 3 | |
| if e.code == flamepy.TaskState.FAILED |
Signed-off-by: Klaus Ma <klausm@nvidia.com>
No description provided.