Skip to content

upstream fsspecstore failures #2853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
d-v-b opened this issue Feb 21, 2025 · 7 comments
Closed

upstream fsspecstore failures #2853

d-v-b opened this issue Feb 21, 2025 · 7 comments

Comments

@d-v-b
Copy link
Contributor

d-v-b commented Feb 21, 2025

Getting failures in tests/test_store/test_fsspec.py::test_wrap_sync_filesystem, not sure what the underlying issue. The relevant test is here:

def test_wrap_sync_filesystem():
"""The local fs is not async so we should expect it to be wrapped automatically"""
from fsspec.implementations.asyn_wrapper import AsyncFileSystemWrapper
store = FsspecStore.from_url("local://test/path")
assert isinstance(store.fs, AsyncFileSystemWrapper)
assert store.fs.async_impl

Does anyone know why this might fail for python 3.13? this is probably due to unreleased changes in fsspec

@martindurant any ideas for how we should change this test?

@martindurant
Copy link
Member

I think fsspecStore should explicitly pass asynchronous=True when using AsyncFileSystemWrapper. I note that this isn't actually a failure but a warning, but still.

The trouble is, that the intent is to call the filesystems in async contexts (coroutine), but creating the FS is happening in blocking code. To get this specific test to pass, you could simply mark it async, but I think it's best to be explicit.

(previously, AsyncFileSystemWrapper was always asynchronous, but it ought to be callable from blocking code too, not least by kerchunk)

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 21, 2025

can you explain what this test actually does? from what I can tell AsyncFileSystemWrapper is imported but unused. Does something magic happen at import time there?

@martindurant
Copy link
Member

Is is used in the isinstance, no? The point is, that although a file: URL is passed, you get an async wrapped instance back, not LocalFileSystem.

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 22, 2025

aha somehow my eyes slid over the isinstance. in any case, this failure is blocking all PRs now so fixing this is very high priority

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 22, 2025

regardless of the resolution of this particular test failure, I'm pretty sure PRs should not be blocked because of changes in our upstream dependencies

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 23, 2025

in the interest of speed I committed the suggested fix to an open PR: eb6b2d8

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 23, 2025

closed via #2851

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants