-
Notifications
You must be signed in to change notification settings - Fork 7.1k
fix: handle dual task errors with read-only args #59507
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
Conversation
ray: handle dual task errors with read-only args - avoid writing to user-defined args when building RayTaskError hybrids - fall back to RayTaskError-only with warning if subclassing fails - add regression test covering read-only args user exceptions
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.
Code Review
This pull request refactors how RayTaskError handles the args attribute of wrapped user exceptions. Previously, RayTaskError directly assigned to self.args, which could cause issues if the user's custom exception defined args as a read-only property. The changes introduce a private attribute _ray_task_error_args and implement args as a property with a setter, allowing RayTaskError to manage its own arguments without conflicting with potentially read-only user-defined args. Additionally, a new exception handler was added to as_instanceof_cause to log warnings when subclassing fails due to user exceptions overriding attributes like args. A new test case was added to verify that RayTaskError correctly wraps and propagates exceptions that have read-only args properties.
codope
left a comment
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.
Overall LGTM. Just a small suggestion for more thorough testing.
[core][test] Added both assertions to test_task_error_with_read_only_args_property assert isinstance(exc_info.value, ReadOnlyArgsError) to confirm dual inheritance assert exc_info.value.args == (exc_info.value.cause,) to confirm the args shim works
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
codope
left a comment
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.
Description
ray: handle dual task errors with read-only args
Related issues
Fixes #59437