Skip to content
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

Improve errors raised by ds.groupby() of unsupported key type #21610

Merged
merged 7 commits into from
Jan 16, 2022

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Jan 14, 2022

Why are these changes needed?

This raises errors early in the driver instead of in the workers. Btw @jjyao we should probably fix our error retry policy to not retry non-IO errors, is there a way to do that today? Perhaps we can raise a fatal type of error for non-IO errors in our tasks.

@jjyao
Copy link
Collaborator

jjyao commented Jan 14, 2022

@ericl I remember when we initially discussed the implementation of retry_exceptions. We started it with a bool type and expected that in the future we will support a list of exception types as well.

The current semantics of bool type is that if it's true, all exceptions are retried, but I think we can change the semantics to be only retrying IOError and subclasses. Alternatively, if we don't want to change the semantics of bool type, we can support list of exceptions and do retry_exceptions=[IOError] by default in dataset.

@clarkzinzow
Copy link
Contributor

@jjyao If it's not much extra effort, I'd vote for adding support for a list of retryable exceptions. Any complications around having Python objects (classes) in task options? Should be able to serialize them as pickle bytes, store those bytes opaquely in the task spec, and eventually deserialize them for the retrying logic during task execution, right?

Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 15, 2022
@ericl
Copy link
Contributor Author

ericl commented Jan 15, 2022

Will follow-up with a PR cleaning up errors for aggregate() and sort() as well.

@ericl ericl merged commit a971774 into ray-project:master Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants