[Train][Tune] Support reading train result from cloud storage

[ahmed-mahran]

This is to support reading Result object from remote/cloud storage systems like s3 and gcs.

Why are these changes needed?

Result can already be stored in remote/cloud storage. There should be a functionality to read it back.

result = Result.from_path("s3://storage_path/experiment_dir/trial_dir/", storage_filesystem=...)
# result.best_checkpoints

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

[ahmed-mahran]

@woshiyyya would you take a look please

[ahmed-mahran]

@justinvyu @krfricke

[justinvyu]

Thanks, this is looking really good! I'll do a review sometime later today.

[justinvyu]

Thanks for the great PR 🤩

I have a few comments, and I can help you out with it.

[ahmed-mahran]

Thanks @justinvyu for the thorough review! I'll try to address your comments early next week.

[ahmed-mahran]

@justinvyu, I think I've addressed your feedback.

[justinvyu]

Thanks! Apart from needing to fix the lint errors, I just have some small comments.

[justinvyu]

I think I'm okay with adding this helper, but also it might be clearer to just do the filesystem operations directly? We can add this in the future if it turns out we need to read a file as text directly from cloud very often.

[ahmed-mahran]

I thought it would be cleaner and clearer to do it this way especially the logic is used twice (for json and csv). I was going to make it inner function in from_path function. I can move as a static method in Result like the old _validate_trial_dir.

[justinvyu]

Good work! Approved and pending tests passing. 🙌

[justinvyu]

Ok, the mock_s3_bucket_uri fixture is actually not in the conftest for ray.air.tests. Let's move this unit test to ray.train.tests instead and add the conftest as a dependency.

1. Move

   ray/python/ray/air/BUILD

   Line 253 in 2e7448e

   py_test(

   to https://github.com/ray-project/ray/blob/master/python/ray/train/BUILD (sorted by alphabetical order)
2. Add conftest as a dependency:

py_test(
    name = "test_result",
    size = "medium",
    srcs = ["tests/test_result.py"],
    tags = ["team:ml", "exclusive"],
    deps = [":ml_lib", ":conftest"]
)

[justinvyu]

Sorry, it is :train_lib instead of :ml_lib 😅