[Train] Report only once in SklearnTrainer

[Yard1]

Signed-off-by: Antoni Baum antoni.baum@protonmail.com

Why are these changes needed?

Sets "done": True in the SklearnTrainer report so that it only print the results only once.

Related issue number

Closes #29226

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 :(

[amogkam]

@Yard1 is this a bug in Tune? Report is only being called once right?

[Yard1]

@amogkam It's more a quirk of Tune - obviously, Tune doesn't know ahead of time if the report its getting will be the last one (unless "done" is explicitly passed in there) - in which case, the functional trainable does the following:

        If the RunnerThread finishes without reporting "done",
        Tune will automatically provide a magic keyword __duplicate__
        along with a result with "done=True". The TrialRunner will handle the
        result accordingly (see tune/trial_runner.py).

I suppose a more generic fix to this issue would be to not print the duplicate result, as duplicate printing is the main issue here. Logic should be otherwise correct in all cases.

[amogkam]

Yeah I guess my main question is does this duplicate printing apply to function trainables as well? Since I've never seen any of our function trainable examples report with "done": True.

If that's the case, then I think we should avoid the duplicate printing on the Tune side.

[Yard1]

Yeah the duplicate printing with functional trainable is the exact issue here. It's not unique to SklearnTrainer, but it's easy to miss it with trainers that report multiple times (because it's just one more result printout out of dozens or hundreds). Also, we print out a result only if n seconds have passed since last printout OR if this is a result with "done": True, so it's not uncommon for the last result with "done": False to not be printed.

[amogkam]

Sounds good. Can we also fix the duplicate printing on the Tune side as well?

[Yard1]

@amogkam followup in tune #30597