-
Notifications
You must be signed in to change notification settings - Fork 79
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
the earthaccess.EarthAccessFile
wrapper need not subclass anything
#620
base: main
Are you sure you want to change the base?
Conversation
I need to play with this a little bit to better understand what's going on, but I may not have time until the next hack day. |
earthaccess.EarthAccessFile
wrapper need not subclass anything
@mfisher87, have you had a chance to do this? @itcarroll or @betolink, I suppose the larger question for me is, what's the point of this class to begin with? Why do we even need it? |
Thanks for checking on this one @chuckwondo! My guess on the need for this class was something to do with deserializing into a useable object in case authentication timed out. A guess only though, as I don't know when |
@jrbourbeau can explain in detail but the gist of it is that this class allows a serialization trick, if we open granules from our laptop but we are offloading an xarray operation to a Dask cluster in |
@jrbourbeau The essential question is whether removing |
I didn't notice the However, I'm questioning the whole idea of pickling the I may very well be thinking about this poorly, or simply be too inexperienced with using dask (and the like), but without seeing an example of the types of things you think this would be helpful for, offhand I would ask you why you're not simply distributing the URLs rather than the open files? Is it so that you don't have to also potentially distribute credentials across such clusters as well? |
I have not yet, sorry :X |
Fixes #610, closes #563.
This PR removes any base class from the definition of
earthaccess.EarthAccessFile
(EAF). Previously, EAF inherited fromfsspec.spec.AbstractBufferedFile
(ABF) so was capable of using methods defined on ABF. But EAF held an instance of an ABF atself.f
and handed off__getattr__
requests to that object. Under this setup,self.read
returnssuper().read
ifread
is defined on ABF (andread
is defined on ABF) elseself.read
returnsself.f.read
. That is a bug. It was probably assumed that__getattr__
would catch all method calls, but it only handles what__getattribute__
can't find.We've scraped by with this setup because
self.f
is also an ABF and either does not override ABF on a called method or the override does little more than itself callsuper()
. The latter is the case forself.f.read
whenf
is afsspec.implementations.http.HTTPFile
. It is not the case whenf
is afsspec.implementations.http.HTTPStreamFile
.This PR also updates some type hints and relevant documentation.
f
was wrong, it is an ABF not afsspec.AbstractFileSystem
ToDo if integration tests look okay:
📚 Documentation preview 📚: https://earthaccess--620.org.readthedocs.build/en/620/