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.

Sign up for GitHub

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

Jump to bottom

method resolution surprise on EarthAccessFile #610

Open
itcarroll opened this issue Jun 25, 2024 · 5 comments · Fixed by #620 · May be fixed by #828
Open

method resolution surprise on EarthAccessFile #610

itcarroll opened this issue Jun 25, 2024 · 5 comments · Fixed by #620 · May be fixed by #828
Labels
type: bug Something isn't working

Comments

@itcarroll
Copy link
Collaborator

itcarroll commented Jun 25, 2024 •
edited
Loading

There's something funny going on with the wrapping of fsspec.spec.AbstractBufferedFile, where the method resolution isn't coming out as expected.

The following gives me an AssertionError

import earthaccess

auth = earthaccess.login()
results = earthaccess.search_data(short_name="VIIRSJ1_L2_OC", count=1)
files = earthaccess.open(results)
with files[0] as g:
    assert g.read is getattr(g.f, "read")

I don't know about the internals, but getattr(g, "read") and g.__getattribute__("read") both follow the MRO to the read method of fsspec.spec.AbstractBufferedFile. We don't want that! We want the read method, given by g.__getattr__("read") as fsspec.implementations.http.HTTPFile.read as resulting from getattr(g.f) in the class definition.

earthaccess/earthaccess/store.py

Lines 28 to 34 in b7443fe

class EarthAccessFile(fsspec.spec.AbstractBufferedFile):
def __init__(self, f: fsspec.AbstractFileSystem, granule: DataGranule) -> None:
self.f = f
self.granule = granule
def __getattr__(self, method: str) -> Any:
return getattr(self.f, method)

Is it possible that __getattr__ should be __getattribute__?

Sorry for the multiple edits; confused myself with two fs
@itcarroll itcarroll changed the title method resolution surprise on EarthAccessGranule method resolution surprise on EarthAccessFile Jun 25, 2024
@itcarroll itcarroll mentioned this issue Jun 25, 2024
Merged
@itcarroll
Copy link
Collaborator Author

No, don't use __getattribute__ b/c it leads to infinite recursion. New idea: this is not a class that should be subclassing at all.

@itcarroll itcarroll mentioned this issue Jun 26, 2024
Merged
2 tasks
@itcarroll itcarroll added the type: bug Something isn't working label Jun 27, 2024
@github-actions github-actions bot mentioned this issue Jul 1, 2024
Closed
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in earthaccess project Sep 23, 2024
@itcarroll
Copy link
Collaborator Author

When #620 removed the inherited fsspec.spec.AbstractBufferedFile class from earthaccess.EarthAccessFile, it broke the ability of xarray.backends.list_engines()["h5netcdf"].guess_can_open to recognize these pointers. It comes back to MRO, and the fact that EarthAccessFile is no longer an instance of io.IOBase.

@itcarroll itcarroll reopened this Sep 24, 2024
@itcarroll
Copy link
Collaborator Author

It would be very helpful if we had an integration test for the behavior that the EarthAccessFile class enables.

@itcarroll
Copy link
Collaborator Author

itcarroll commented Sep 25, 2024 •
edited
Loading

A change to repair damage done in #620 is to reintroduce a base class on EarthAccessFile (and I think io.IOBase might work), but it doesn't fix this issue.

This was referenced Sep 30, 2024
Draft
Merged
@itcarroll
Copy link
Collaborator Author

Merged #832 in order to make way for a release. Note that the test surfacing this bug has been marked as an expected fail.

earthaccess/tests/unit/test_store.py

Lines 132 to 141 in fb78806

@pytest.mark.xfail(
reason="This test reproduces a bug (#610) which has not yet been fixed."
)
def test_earthaccess_file_getattr():
fs = fsspec.filesystem("memory")
with fs.open("/foo", "wb") as f:
earthaccess_file = EarthAccessFile(f, granule="foo")
assert f.tell() == earthaccess_file.tell()
# cleanup
fs.store.clear()

nikki-t added a commit that referenced this issue Oct 1, 2024
@nikki-t
Remove #610 (reverted) from CHANGELOG
7ee213d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug Something isn't working
Projects
earthaccess project
Status: ✅ Done
Milestone
No milestone
1 participant
@itcarroll