[train][tune] Safely check if the storage filesystem is pyarrow.fs.S3FileSystem

[ccoulombe]

Why are these changes needed?

When PyArrow does not have support, checking if the instance is pyarrow.fs.S3FileSystem raises an exception.

>>> import pyarrow.fs
>>> pyarrow.fs.S3FileSystem
...
ImportError: The pyarrow installation is not built with support for 'S3FileSystem'

Fix by checking if the S3FileSystem is importable before doing this typecheck.

Related issue number

Fixes #48187

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 :(
  • Tested by a hotfix locally

[rkooo567]

Thanks for the contribution! Btw, it has a test failure;

[rkooo567]

can you fix that? I guess there are times the destination_file_system is None

[justinvyu]

@ccoulombe I'd prefer not to do the string check. Let's do this instead:

try:
    from pyarrow.fs import S3FileSystem
except ImportError:
    S3FileSystem = None

if S3FileSystem and isinstance(..., S3FileSystem):
    ...

[ccoulombe]

@justinvyu

I'd prefer not to do the string check.

Sure, but out of curiosity : why ?

fyi, type_name is used elsewhere in the code base to check for the type of FS

[justinvyu]

String check may be less stable than checking for the class since pyarrow could change the type name. Other pyarrow.fs.FileSystem subclasses could also potentially collide with the same type name (ex: the fsspec s3fs, although this one is fine because there's a fsspec:: prefix). But I would rather just guarantee that this logic is only done for this specific pyarrow implementation since it's a bug in pyarrow.fs.S3FileSystem specifically.

Yeah, type_name should probably be removed for type checks, but I think the only other usage is for collecting telemetry.

[ccoulombe]

@justinvyu Updated, and I signed off the commits afterwards. Cheers

[justinvyu]

Thanks!