Skip to content
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

Fix broken async reference file system _cat_file method #1734

Merged
merged 10 commits into from
Oct 23, 2024

Conversation

mpiannucci
Copy link
Contributor

@mpiannucci mpiannucci commented Oct 22, 2024

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 #1735

@mpiannucci mpiannucci changed the title Fix broken async reference file system _cat_file method Fix broken async reference file system _cat_file method Oct 22, 2024
@martindurant
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Okay I have a test added and working well!

@mpiannucci
Copy link
Contributor Author

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

@martindurant
Copy link
Member

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
Copy link
Member

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

@mpiannucci
Copy link
Contributor Author

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

@mpiannucci
Copy link
Contributor Author

Well i am stumped

@martindurant
Copy link
Member

@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.

@martindurant martindurant merged commit 001be4a into fsspec:master Oct 23, 2024
11 checks passed
@mpiannucci
Copy link
Contributor Author

Thanks for the help!

"version": 1,
"refs": {
"reference_time/0": [
"https://noaa-nwm-retro-v2-0-pds.s3.amazonaws.com/full_physics/2017/201704010000.CHRTOUT_DOMAIN1.comp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to require the network; should this test get a vcr?

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

Successfully merging this pull request may close these issues.

Calling _cat_file from a ReferenceFileSystem always returns None
3 participants