-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[core] Add more message when accessing a dead actor. #34697
Conversation
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
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.
LGTM. just NITs for error message improvement
name = "exported_internal", | ||
srcs = glob( | ||
[ | ||
"src/ray/internal/internal.cc", |
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.
Guess it is unrelated changes? Looks okay to include in this PR to me.
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.
Actually it is... There is some method used in this file got impacted by this PR. So when I compile it just failed :(
I notice it seems not useful, so just delete that file.
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
## Why are these changes needed? When any ray worker access the actor which doesn't belong to the cluster, it'll crash. This is really bad user experience because: - The user will lose ray worker or driver. - The error message is not very useful. This PR fixed it by avoiding crash and throw exception instead. When the user accidentially access an actor which doesn't belong to this cluster, it'll get an Exception with message: `It might be dead or it's from a different cluster`. This PR also deleted some code that's not useful. Signed-off-by: Jack He <[email protected]>
## Why are these changes needed? When any ray worker access the actor which doesn't belong to the cluster, it'll crash. This is really bad user experience because: - The user will lose ray worker or driver. - The error message is not very useful. This PR fixed it by avoiding crash and throw exception instead. When the user accidentially access an actor which doesn't belong to this cluster, it'll get an Exception with message: `It might be dead or it's from a different cluster`. This PR also deleted some code that's not useful.
Why are these changes needed?
When any ray worker access the actor which doesn't belong to the cluster, it'll crash. This is really bad user experience because:
This PR fixed it by avoiding crash and throw exception instead. When the user accidentially access an actor which doesn't belong to this cluster, it'll get an Exception with message:
It might be dead or it's from a different cluster
.This PR also deleted some code that's not useful.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.