[Tune] Fix storage client creation when sync function tpl is not provided (#26714)

[rohit-annigeri]

Why are these changes needed?

It solves issue of restore from object failing to work during ray tune with sync config and PBT scheduler.

Related issue number

Closes #26714

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

[krfricke]

Thanks @rohit-annigeri - actually that part is correct. Your change will use the legacy sync client which is deprecated and even removed on master.

The correct fix here is to not try to sync the directory if it already exists locally. This regression actually exists in master as well. Here is a draft PR fixing it: #26725
I'll add tests to that PR to make sure this doesn't come up again and that everything works.

Could you apply that patch to 1.13.0 and see if it fixes your problem?

Thanks!

[rohit-annigeri]

Could you apply that patch to 1.13.0 and see if it fixes your problem?
So that would merge to releases/1.13.0 ?
Also verified that my fix solves the problem.

[krfricke]

I mean if you apply the changes from #26725 to releases/1.13.0, does that also solve the problem?

The current fix in this PR disables syncing via pyarrow fs completely (as self.uses_cloud_checkpointing will be True), which is a regression.

The correct location to add the check would be at https://github.com/ray-project/ray/blob/releases/1.13.0/python/ray/tune/trainable.py#L536 to not trigger syncing if the directory already exists.

I can also update this PR if you prefer.

We will have to open it for releases/1.13.1 in any case. I see you already updated that, thanks!

Thanks!

[rohit-annigeri]

I think I follow. I've updated the PR to reflect the change. Let me know if it makes sense. Not really sure if this is the best way to handle the exception

[rohit-annigeri]

One side affect while "fixing" this, now it seems to sync all tmp43save_to_object folders to s3 as well.

[krfricke]

This looks good to me. We will merge this once we commence work on 1.13.1. I may update this PR with test cases when updating #26725

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[rohit-annigeri]

hey @krfricke is this PR still good to go ?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[krfricke]

Generally yes, but there is currently no timeline for 1.13.1

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.