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

Add fsspec http support #1228

Merged
merged 4 commits into from
Nov 4, 2022

Conversation

braingram
Copy link
Contributor

Opening an http filesystem file using fsspec returns an object that is incompatible with generic_io. The fsspec object, when treated like a generic_io.RealFile, fails on both the _size and name/uri calculations.

generic_io.GenericFile._size does not appear to be used so this PR removes it from all GenericFile classes.

fd.name is used to calculate the uri (when not otherwise supplied). fsspec http files do not have a name so a different method of defining uri (such as providing one at the time of opening) will need to be used when a uri is necessary. This PR adds a hasattr check for name to avoid an exception during creation of the RealFile.

This is calculated but never used. The current calculation for
generic_io.RealFile does not work with fsspec when used over http
so drop _size calculation.
@braingram
Copy link
Contributor Author

@WilliamJamieson do you know of any uses of generic_io.GenericFile._size? This appears to be calculated for several of the GenericFile subclasses but I'm not seeing a use anywhere in the code.

@WilliamJamieson
Copy link
Contributor

@WilliamJamieson do you know of any uses of generic_io.GenericFile._size? This appears to be calculated for several of the GenericFile subclasses but I'm not seeing a use anywhere in the code.

No

@braingram braingram marked this pull request as ready for review November 2, 2022 20:57
@WilliamJamieson
Copy link
Contributor

The jwst failure is unrelated and should be fixed by spacetelescope/jwst#7331.

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

These changes seem reasonable, do they enable the example in #1146 to work properly now?

If so can you create a test based on that example and add it to test_asdf.py?

@braingram
Copy link
Contributor Author

These changes seem reasonable, do they enable the example in #1146 to work properly now?

If so can you create a test based on that example and add it to test_asdf.py?

Both examples, the local and http filesystems, should now work with tests for both (this PR adds the http filesystem support and tests). Let me know if there's anything else you'd like me to add to this PR otherwise it looks good to me.

@WilliamJamieson WilliamJamieson merged commit 7a64cb5 into asdf-format:master Nov 4, 2022
@braingram braingram deleted the feature/fsspec_http branch November 7, 2022 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants