search

fsspec / filesystem_spec

A specification that python filesystems should adhere to.
BSD 3-Clause "New" or "Revised" License
1.05k stars 362 forks source link

Fix broken async reference file system `_cat_file` method #1734

Closed mpiannucci closed 1 month ago

mpiannucci commented 1 month ago

Adds missing return in _cat_file method of ReferenceFileSystem

Im not sure how to best test this because i dont think any of the tests from test_reference use an async filesystem and s3 maybe shouldnt be tested here?

Closes https://github.com/fsspec/filesystem_spec/issues/1735

martindurant commented 1 month ago

Can we make a test? It will need to run in an async func, with an async backend (http?) - can we guarantee something that won't disappear?

mpiannucci commented 1 month ago

The async function part is easy, are you ok with pointing to s3 data via http similar to the kerchunk tests? Thats the easiest thing i can think of doing for now

martindurant commented 1 month ago

I suppose we can reconsider if it starts to fail. The download should be small though (part of a file only), as the GHA CI runners are pretty weak.

mpiannucci commented 1 month ago

Okay I have a test added and working well!

mpiannucci commented 1 month ago

@martindurant do you have any idea why this test is failing? It works fine locally...

martindurant commented 1 month ago

I also ran it successfully locally. Is there a chance the server explicitly won't take connections from CI?

You might try 

    fs={"https": fss}

(because the default protocol is "http", so maybe another filesystem instance is being silently created)

martindurant commented 1 month ago

Or, perhaps passing remote_protocol . Actually, it is https? I might have been confused.

mpiannucci commented 1 month ago

Let see if that works... maybe http was blocked

mpiannucci commented 1 month ago

Well i am stumped

martindurant commented 1 month ago

@mpiannucci : so now it passes. I don't know why, but an http FS must have leaked from another (async) test case. Probably worth tracking that down at some point, or to make a FS that lasts all session or make sure that when an FS cleans itself, it also gets removed from the instance cache.

mpiannucci commented 1 month ago

Thanks for the help!