-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable subclassing the netCDF4 backend, changing the dataset class #8338
Conversation
Failing with lots of timeouts in the MacOS py3.11 build, and all in |
@ZedThree I've restarted CI, let's see if it was just flaky. |
Thanks! Looks like it was just flaky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not super familiar with the backend stuff, but I left some comments that should make it a bit clearer.
DataStore = NetCDF4DataStore | ||
# This is to allow subclassing this backend entrypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataStore = NetCDF4DataStore | |
# This is to allow subclassing this backend entrypoint | |
# allows to subclass this backend entrypoint | |
DataStore = NetCDF4DataStore |
This is a nit but we usually comment above the code
@classmethod | ||
def DatasetClass(self) -> type: | ||
# This is to allow subclassing the datastore and retain lazy-importing | ||
import netCDF4 | ||
|
||
return netCDF4.Dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is not a
@classmethod
(because it does not return a new instance of theNetCDF4DataStore
class, also would take acls
as first argument). If then I think it would be a@staticmethod
. - I also don't like the name so much, because I first thought of
xr.Dataset
- maybeopener
(as in https://github.com/PlasmaFAIR/xarray/blob/18af8e63f98c9aa7c54ad6afe13cb583e92bacc9/xarray/backends/file_manager.py#L54) oropener_cls
orbackend_opener
or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change feels a bit weird, and only to support easy subclassing.
I'm in favor of optimizing for simpler subclassing but not if it makes code less readable.
Also this breaks several type checks (which in this case doesn't matter because netCDF4 is still untyped).
Maybe instead you could factor out the parts that require netCDF4 such that you can easily override these in the subclass?
Sorry, this is no longer needed, as we're now going to add the functionality straight to the netCDF4 Python library. xarray will then need only need a new argument adding here, instead of needing a whole new backend |
See #8288
whats-new.rst
api.rst
These changes allow me to make a pretty minimal subclass that just swaps out the
netCDF4.Dataset
class for my version: