Skip to content

Fix method resolution bug when base class methods override getattr #1015

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

alexandervladsemenov
Copy link

@alexandervladsemenov alexandervladsemenov commented May 23, 2025

This PR closes #610.

The issue was that the getattr method does not override methods from the base class. As a result, EarthAccessFile methods were inherited from the base class fsspec.spec.AbstractBufferedFile.

  • To address this, getattribute is used instead of getattr. The implementation is designed to avoid infinite recursion.

  • The base class has been changed from fsspec.spec.AbstractBufferedFile to io.IOBase to ensure correct reading from in-memory files using xarray.

  • An additional test has been added to verify in-memory reading with xarray.


📚 Documentation preview 📚: https://earthaccess--1015.org.readthedocs.build/en/1015/

Copy link

github-actions bot commented May 23, 2025

Binder 👈 Launch a binder notebook on this branch for commit e4e7c82

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit d5377fc

Binder 👈 Launch a binder notebook on this branch for commit 8a7b220

Binder 👈 Launch a binder notebook on this branch for commit cb7d3d7

@itcarroll
Copy link
Collaborator

Thanks for the PR! I will work out how to make the integration-tests run on GitHub Actions.

@betolink @mfisher87 I'm surprised there is no previous test using xarray.open_*, so where should such a test go? This PR has it with tests/unit/test_store but it seems to me like a new file called tests/integration/test_xarray.py would be better.

@itcarroll
Copy link
Collaborator

itcarroll commented Jun 3, 2025

Have confirmation from in zulipchat that test_earthaccess_xarray_access should be moved under tests/integration. I suggest a test_xarray module, and renaming to test_open_dataset.

@chuckwondo chuckwondo changed the title Impelement a fix to address the method resolution bug when base class methods ovveride getattr Impelement a fix to address the method resolution bug when base class methods override getattr Jun 11, 2025
@chuckwondo chuckwondo changed the title Impelement a fix to address the method resolution bug when base class methods override getattr Fix method resolution bug when base class methods override getattr Jun 11, 2025
@alexandervladsemenov
Copy link
Author

Have confirmation from in zulipchat that test_earthaccess_xarray_access should be moved under tests/integration. I suggest a test_xarray module, and renaming to test_open_dataset.
@itcarroll I've created "test_xarray" integration test and pushed an update.

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.

method resolution surprise on EarthAccessFile
2 participants