[tune] Fix an edge case where DurableTrainable would not delete checkpoints in remote storage

[Yard1]

Why are these changes needed?

If both the runner and trial are present on the same node, DurableTrainable will never remove files in remote storage as it would be unable to find local files, which had already been deleted by the checkpoint_deleter. This change ensures that checkpoint_deleter doesn't remove local files by itself if the runner is on the same node.

Related issue number

Checks

• 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]

How about instead, we change the logic in DurableTrainable to always remove checkpoint in remote storage whether or not local copy is found?
try:
except:
finally:

[Yard1]

@xwjiang2010 It won't be able to find the checkpoint directory through the find method then

[xwjiang2010]

@xwjiang2010 It won't be able to find the checkpoint directory through the find method then

Yeah that's another thing I don't quite understand. find_checkpoint_dir deals with some nested checkpoint case and recursively tries to find the top layer directory that contains .is_checkpoint. For the cases I have seen, it always gives me the same output as input tho. Would be good to understand the case this function is trying to address.

[xwjiang2010]

I am ok with the direction of the PR. Thanks Antoni for spotting the bug :)

Besides my question about when to care about nested checkpoint and thus find_checkpoint_dir, if we want to proceed with this direction, can we:

• Add some high level description of the responsibilities of CheckpointManager, DurableTrainable including in the context of when Trainable and CheckpointManager happen to be on the same physical machine. This would greatly help with reasoning about the code.
• Add a test?

[krfricke]

Thanks, I think we can go ahead with this. But I agree we might want to refactor this (remote) checkpointing logic later

[richardliaw]

Is there some way we can test this PR?

[Yard1]

We could include a test for this in one of the release tests using s3, but I don't see a unit test for this possible, unless we somehow mock node ips? This would have to run on a multi-node setup

[richardliaw]

OK. How about let's keep track of this in an issue, as we should add multi-testing soon.

[xwjiang2010]

Do you know why the release test set up cannot access S3?