[tune] Add retry logic for restoring trials.

[xwjiang2010]

Signed-off-by: xwjiang2010 [email protected]

Why are these changes needed?

This is an advanced setting. Consider the following scenario: Due to scheduling glitches, sometimes a restoring
trial may be scheduled onto a dying node. By setting this env var to a positive number, the trial can be restored
several times and hopefully one of the times it will not be put on a dying node. This retry behavior won't increment
the per trial failure number, which is compared against max_failures.

Related issue number

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[xwjiang2010]

@krfricke actually do you know why we don't physically delete those checkpoint_0000x folders when tune decides to restore from scratch? I think this a potential bug source.

[krfricke]

I think we should move this whole logic into the trial to avoid strong coupling between the trial runner and the trial. Basically we want to avoid that the trial runner tells the trial how to update it's internal state.

Instead, we should make the information available to the trial so that it can act based on this. For this case, we would want to know what part of the trial failed.

I can think of a few ways to do that:

1. We introduce TuneRestoreError, TuneTrainingError, TuneActorError, and wrap exc in it before passing it to write_error_log.
2. We pass something like an ErrorType enum

I think I prefer option 1, but open to suggestions.

The trial can then decide which counters to increase and how to write the error log. For instance, it would be helpful to log restore errors as Restore failure # {} to distinguish this from training errors in the log file.

For a first step to land it until branch cut, we can just distinguish between restore and non-restore errors, if that's easier.

As a minor refactor note, I think we should rename write_error_log to handle_error and only call it when an error is actually set (i.e. move the if exc into the trial runner and remove the Optional annotation in the method).

What do you think?

[xwjiang2010]

Yeah this makes sense.
I was prioritizing making the cut. But let me try how extensive the change may be.

[xwjiang2010]

Ok updated. PTAL!

[krfricke]

LGTM, minor nit

[richardliaw]

approve for docs

[krfricke]

Suggested change

	def test_retore_retry(retry_num):
	def test_retore_retry(ray_start_4_cpus, retry_num):

should shut down ray gracefully