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.

Sign up for GitHub

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

Jump to bottom

[tune] Fix Pausing and Error Propogation #2815

Merged
merged 6 commits into from
Sep 4, 2018
Merged

[tune] Fix Pausing and Error Propogation #2815

merged 6 commits into from
Sep 4, 2018

Conversation

richardliaw
Copy link
Contributor

What do these changes do?

Addresses #2814 and #2810.

  • Manages error handling with recovery, even when error occurs at ray.get.
  • Also introduces tests for pausing via trial executor.

Related issue number

Above.

@ericl
Copy link
Contributor

ericl commented Sep 3, 2018

Can you also change the smoke test iterations for the pbt test to something like 20? That catches the bug and also runs reasonably fast.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7958/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7961/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7968/
Test PASSed.

@richardliaw
Copy link
Contributor Author

@joyyoj Can you take a look please?

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7972/
Test FAILed.

@richardliaw
Copy link
Contributor Author

jenkins retest this please

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7974/
Test FAILed.

@richardliaw
Copy link
Contributor Author

Jenkins retest this please

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7975/
Test PASSed.

@joyyoj
Copy link
Contributor

joyyoj commented Sep 4, 2018

the changes make sense.

@richardliaw richardliaw mentioned this pull request Sep 4, 2018
Closed
ericl
ericl approved these changes Sep 4, 2018
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

lgtm based on tests and @joyyoj 's approval (I did not look carefully at the unpause crash, it looks complicated)

One minor suggestion on the error handling that might give a cleaner interface.

python/ray/tune/trial_executor.py
"""
raise NotImplementedError("Subclasses of TrialExecutor must provide "
"fetch_one_result() method")
raise NotImplementedError
Copy link
Contributor

Choose a reason for hiding this comment

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

So one other way of doing this is to have fetch_one_result() return an exception object for the result if there is an error. This avoids having a separate call just for this error handling.

@ericl ericl merged commit 72542c9 into ray-project:master Sep 4, 2018
@ericl
Copy link
Contributor

ericl commented Sep 4, 2018

Merging now since this is a fairly severe regression. Comments can be done in a follow-up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericl ericl ericl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@richardliaw @ericl @AmplabJenkins @joyyoj