[tune] Fix hyperband r calculation and stopping

[krfricke]

Why are these changes needed?

This PR fixes two flaws in the current hyperband implementation.

1. Bug in the r calculation.

#1620 introduced a minimum constraint in the r calculation during successive halving with r = min(r, max_t - prev_r). It's unclear to me where this is coming from (cc @richardliaw), but in my opinion this is flawed.

E.g. for s=1, max_t=8 and eta=2, we get r0 = 4. Then r = r0 * 2 = 8. With the current formula, we then get r1 = min(r, max_t - r0) = min(8, 8-4) = 4. Thus, r0 = r1 and the bracket already "finished" after 4 iterations and should terminate all trials. Or in other words, none of the trials in this bracket will ever proceed.

I believe the correct fix here is to set r = min(r, max_t). I couldn't find a reference implementation for comparison, but it logically makes sense and seems to match the formula behavior described in the paper.

2. Stopping of "overstepped" trials.

The first bug revealed a second shortcoming in the current implementation. When a trial reports a timestep that is higher than r_i and r_(i+1), it can hang forever. This is because "good" trials are only continued if bracket.continue_trial(t) returns True. However, if the trial already overstepped r_(i+1), this can return False (specifically when stop_last_trials=True). In that case, a paused or running trial will not be terminated nor continued, and instead hang forever.

This second case is fixed in this PR by introducing another clause in the processing of "good" trials that checks for this condition.

Related issue number

Closes #37605

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 added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• 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 :(

[justinvyu]

I think these fixes make sense. I'm not so familiar with our hyperband implementation so would need to spend more time on this for a full review.

If tests are passing, happy to get this in now (if aiming for 2.7)

[justinvyu]

May lose the checkpoint here if we stop while saving?

[krfricke]

This change is avoiding the situation when the trial is in the middle of pausing:

            if should_checkpoint:
                self._cached_trial_decisions[trial.trial_id] = TrialScheduler.PAUSE
                future_result = self._schedule_trial_save(
                    trial=trial, storage=CheckpointStorage.PERSISTENT
                )
                trial.temporary_state.saving_to = future_result

where the status is technically still "RUNNING" while the save is resolving.

I think it's fine to miss the checkpoint here. The trial is bad anyway and is going to be early-terminated.

[krfricke]

I think these fixes make sense. I'm not so familiar with our hyperband implementation so would need to spend more time on this for a full review.

If tests are passing, happy to get this in now (if aiming for 2.7)

Tests seem to be passing fine - and yes, let's pick for 2.7 as it fixes behavior relating to the new storage path