Replace EarthAccessFile with classes inheriting from specific AbstractBufferedFile subclasses #828
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #610
This PR provides an "explicit is better than implicit" solution to fixing the method resolution order (MRO) on the (now former)
EarthAccessFile
(#610). The bug was thatEarthAccessFile
tried to proxy boths3fs.S3File
andfsspec.implmentations.http.HTTPFile
, but failed to proxy those methods also defined on the superclassfsspec.spec.AbstractBufferedFile
from which all three inherited. My first attempted fix (PR #620) removedfsspec.spec.AbstractBufferedFile
from the MRO ofEarthAccessFile
. While this did proxy methods correctly, usage was broadly broken because eachEarthAccessFile
was no longer an instance of the file-like object proxied (which was important, in particular forxarray.open_dataset
!).Why proxy at all? The file-like objects that
earthaccess.open
provides must be pickleable AND the un-pickling process needs to be able to change the particular type of file-like object used, an essential part of the hand-off of file-like objects between xarray running locally and dask workers in the cloud.What this PR does:
class EarthAccessFile(fsspec.spec.AbstractBufferedFile)
with a suite of (currently three) classes that could be returned byfs.open
wherefs
is either anS3FileSystem
or anHTTPFileSystem
.EarthaccessS3File(EarthaccessMixin, S3File)
EarthaccessHTTPFile(EarthaccessMixin, HTTPFile)
EarthaccessHTTPStreamFile(EarthaccessMixin, HTTPStreamFile)
EarthaccessMixin
to implement the__reduce__
method for custom pickling__reduce__
callable, themake_instance
function, to avoid "double pickling" (no moredumps
)NOTE: Because #620 was merged, the "Files changed" in this PR are misleading relative to the original implementation. The best commit to reference for comparison is from right before the merge.
Still in progress:
Pull Request (PR) draft checklist - click to expand
contributing documentation
before getting started.
title such as "Add testing details to the contributor section of the README".
Example PRs: #763
example
closes #1
. SeeGitHub docs - Linking a pull request to an issue.
CHANGELOG.md
with details about your change in a section titled## Unreleased
. If such a section does not exist, please create one. FollowCommon Changelog for your additions.
Example PRs: #763
README.md
with details of changes to theearthaccess interface, if any. Consider new environment variables, function names,
decorators, etc.
Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!
Pull Request (PR) merge checklist - click to expand
Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the
@nsidc/earthaccess-support
team in a comment and wewill help you out!
Request containing "pre-commit.ci autofix" to automate this.
📚 Documentation preview 📚: https://earthaccess--828.org.readthedocs.build/en/828/