[Data] Fix filesystem compatibility check for fsspec-wrapped PyFileSystem by xyuzh · Pull Request #61850 · ray-project/ray

xyuzh · 2026-03-18T22:55:47Z

Summary

Fix _is_filesystem_compatible_with_scheme to properly inspect the inner fsspec protocol when a filesystem is wrapped in PyFileSystem, preventing incorrect rejection of user-provided fsspec filesystems (e.g., s3fs for s3:// URIs).
Upgrade OSError log level from debug to warning in download_bytes_threaded for better observability.
Add comprehensive unit tests for _is_filesystem_compatible_with_scheme covering native PyArrow filesystems, fsspec-wrapped filesystems with various protocols, and edge cases.

Test plan

Added TestIsFilesystemCompatibleWithScheme test class with tests for native S3/local filesystems, fsspec S3/GCS/HTTP wrappers, tuple protocols, scheme mismatches, bare paths, and unknown schemes.
Run existing Ray Data unit tests to verify no regressions.

…stem The `_is_filesystem_compatible_with_scheme` function was not correctly identifying fsspec filesystems wrapped in PyFileSystem. This caused mismatches where a user-provided fsspec S3 filesystem would be rejected for s3:// URIs. Now the inner fsspec protocol is inspected to determine compatibility. Also upgrades OSError log level from debug to warning in download_bytes_threaded.

The direct `fs_type in expected_types` check matched any PyFileSystem (type_name "py") against http/https schemes, since the scheme map includes "py" for those. This short-circuited before the inner fsspec protocol check could reject non-HTTP wrappers like s3fs. Move the direct match after the PyFileSystem branch so fsspec wrappers always go through protocol inspection first.

gemini-code-assist

Code Review

This pull request fixes a filesystem compatibility check for fsspec-wrapped filesystems, improves logging for OSError, and adds comprehensive unit tests. The changes look good, but I've found a potential issue in the new compatibility check logic that doesn't correctly handle RetryingPyFileSystem. I've provided a suggestion to fix this.

RetryingPyFileSystem has type_name "RetryingPyFileSystem", not "py", so it was falling through to the direct match and failing. Include it in the condition so it enters the fsspec protocol inspection branch.

…lity When RetryingPyFileSystem wraps a native PyArrow filesystem (e.g., S3FileSystem), unwrap() returns a non-PyFileSystem instance. The isinstance(actual_fs, PyFileSystem) check then fails, skipping inner protocol matching and falling through to return False — never reaching the native type_name check. After unwrapping, check if the result is a non-PyFileSystem and fall back to direct type_name matching.

Import from ray.data._internal.util, consistent with the rest of the codebase.

github-actions · 2026-04-09T00:49:17Z

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

You can always ask for help on our discussion forum or Ray's public slack channel.

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

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>

bveeramani · 2026-04-13T03:31:51Z

    assert resolved_paths[0] == path


+class TestIsFilesystemCompatibleWithScheme:


For future reference, I think we could improve these tests by testing against real implementations like S3FileSystem rather than mocks with fake type_name.

The current set of tests are brittle:

They assume _is_filesystem_compatible_with_scheme's implementation uses type_name. If that implementation detail changes, these tests break.

They assume the type_name of PyArrow filesystem implementations. If PyArrow changes these, our software will break but it won't be caught by these tests

Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 0e1c8e8. Configure here.}

cursor · 2026-04-13T07:29:29Z

+
+        actual_fs = filesystem
+        if isinstance(actual_fs, RetryingPyFileSystem):
+            actual_fs = actual_fs.unwrap()


Redundant unwrapping duplicates earlier computation

Low Severity

The actual_fs computation on lines 206–208 exactly duplicates the unwrapped computation on lines 191–195 — both unwrap RetryingPyFileSystem to get the inner filesystem. After both run, actual_fs always equals unwrapped. Having two separate variables that track the same value introduces unnecessary complexity and a maintenance risk: if one unwrapping path is updated but the other is forgotten, a subtle bug would be introduced. The code could simply reuse unwrapped instead of re-deriving actual_fs from filesystem.

Additional Locations (1)

python/ray/data/datasource/path_util.py#L190-L198

^{Reviewed by Cursor Bugbot for commit 0e1c8e8. Configure here.}

…stem (ray-project#61850) ## Summary - Fix `_is_filesystem_compatible_with_scheme` to properly inspect the inner fsspec protocol when a filesystem is wrapped in `PyFileSystem`, preventing incorrect rejection of user-provided fsspec filesystems (e.g., s3fs for `s3://` URIs). - Upgrade OSError log level from `debug` to `warning` in `download_bytes_threaded` for better observability. - Add comprehensive unit tests for `_is_filesystem_compatible_with_scheme` covering native PyArrow filesystems, fsspec-wrapped filesystems with various protocols, and edge cases. ## Test plan - [x] Added `TestIsFilesystemCompatibleWithScheme` test class with tests for native S3/local filesystems, fsspec S3/GCS/HTTP wrappers, tuple protocols, scheme mismatches, bare paths, and unknown schemes. - [ ] Run existing Ray Data unit tests to verify no regressions. --------- Signed-off-by: Balaji Veeramani <balaji@anyscale.com> Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Co-authored-by: Balaji Veeramani <balaji@anyscale.com> Co-authored-by: Balaji Veeramani <bveeramani@berkeley.edu>

xyuzh requested a review from a team as a code owner March 18, 2026 22:55

Remove unrelated logger.warning change and extra docstring

ff95a7b

xyuzh assigned xyuzh and bveeramani Mar 18, 2026

xyuzh requested a review from bveeramani March 18, 2026 23:02

cursor Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread python/ray/data/datasource/path_util.py Outdated

gemini-code-assist Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread python/ray/data/datasource/path_util.py

Handle RetryingPyFileSystem type_name in compatibility check

c54f841

RetryingPyFileSystem has type_name "RetryingPyFileSystem", not "py", so it was falling through to the direct match and failing. Include it in the condition so it enters the fsspec protocol inspection branch.

cursor Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread python/ray/data/datasource/path_util.py

ray-gardener Bot added the data Ray Data-related issues label Mar 19, 2026

cursor Bot reviewed Mar 19, 2026

View reviewed changes

Comment thread python/ray/data/tests/unit/test_path_util.py Outdated

Fix wrong import path for RetryingPyFileSystem in test

1334980

Import from ray.data._internal.util, consistent with the rest of the codebase.

cursor Bot reviewed Mar 19, 2026

View reviewed changes

Comment thread python/ray/data/tests/unit/test_path_util.py Outdated

xyuzh added the go add ONLY when ready to merge, run all tests label Mar 25, 2026

Mengyan1 mentioned this pull request Mar 31, 2026

[Data] Add obstore async download backend for download operator #61735

Merged

github-actions Bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Apr 9, 2026

Merge branch 'master' into fix-fsspec-filesystem-compatibility-check

62554fa

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>

bveeramani approved these changes Apr 13, 2026

View reviewed changes

bveeramani enabled auto-merge (squash) April 13, 2026 03:32

Fix tests

0e1c8e8

Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>

github-actions Bot disabled auto-merge April 13, 2026 07:21

cursor Bot reviewed Apr 13, 2026

View reviewed changes

bveeramani merged commit bd60896 into ray-project:master Apr 13, 2026
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Data] Fix filesystem compatibility check for fsspec-wrapped PyFileSystem#61850

[Data] Fix filesystem compatibility check for fsspec-wrapped PyFileSystem#61850
bveeramani merged 8 commits into
ray-project:masterfrom
xyuzh:fix-fsspec-filesystem-compatibility-check

xyuzh commented Mar 18, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 9, 2026

bveeramani Apr 13, 2026

cursor Bot left a comment

cursor Bot Apr 13, 2026

Uh oh!

Labels

2 participants

		assert resolved_paths[0] == path


		class TestIsFilesystemCompatibleWithScheme:

Uh oh!

Conversation

xyuzh commented Mar 18, 2026

Summary

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 9, 2026

bveeramani Apr 13, 2026

Choose a reason for hiding this comment

cursor Bot left a comment

Choose a reason for hiding this comment

cursor Bot Apr 13, 2026

Choose a reason for hiding this comment

Redundant unwrapping duplicates earlier computation

Uh oh!

Labels

2 participants