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

backend interface, now it uses subclassing #4836

Merged
merged 9 commits into from
Jan 28, 2021
Merged

backend interface, now it uses subclassing #4836

merged 9 commits into from
Jan 28, 2021

Conversation

aurghs
Copy link
Collaborator

@aurghs aurghs commented Jan 21, 2021

Currently, the interface between the backend and xarray is the class/container BackendEntrypoint, that must be instantiated by the backend.
With this pull request, BackendEntrypoint is replaced by AbstractBackendEntrypoint. The backend will inherit from this class.

Reason for these changes:

  • This type of interface is more standard.

  • Tests updated

  • Passes pre-commit run --all-files

@aurghs
draft
95a1ea2
@aurghs
working version
a1dc61e
@aurghs
Merge branch 'master' into replace-BackendEntrypoints-with-AbstractBa…
fba3636 
…ckendEntrypoint

# Conflicts:
#	xarray/backends/cfgrib_.py
#	xarray/backends/common.py
#	xarray/backends/h5netcdf_.py
#	xarray/backends/netCDF4_.py
#	xarray/backends/plugins.py
#	xarray/backends/pseudonetcdf_.py
#	xarray/backends/pydap_.py
#	xarray/backends/pynio_.py
#	xarray/backends/scipy_.py
#	xarray/backends/store.py
#	xarray/backends/zarr.py
@aurghs
fix: instantiate BackendEtrypoints
855bc83
@aurghs
rename AbstractBackendEntrypoint in BackendEntrypoint
f40ad35
@aurghs
fix plugins tests
c05cdb3
@aurghs
style
3c9f91a
@aurghs
style
c9843f8
@aurghs
raise NotImplemetedError if BackendEntrypoint.open_dataset is not imp…
2820f09 
…lemented
@aurghs aurghs changed the title WIP: backend interface, now use subclassing WIP: backend interface, now it uses subclassing Jan 21, 2021
jhamman
jhamman reviewed Jan 21, 2021
View reviewed changes
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all looking reasonable to me. Want to get @shoyer's eyes on this though since he was part of the last discussion about the interface class implemented here.

xarray/backends/common.py

BACKEND_ENTRYPOINTS: Dict[str, BackendEntrypoint] = {}

BACKEND_ENTRYPOINTS: Dict[str, Type[BackendEntrypoint]] = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a typing perspective, wont this will require all backends to subclass from BackendEntrypoint? So while we seem to have decided to not to explicitly check for inheritance, this would seem to require it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm.. you are right, and in fact this is a point in favour of inheritance. So we can control typing. Otherwise that would be Any that feels less correct to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, type checking is optional, and I doubt type-checkers are aware of entry-points :). So this would seem to imply that restriction only for xarray. That said, I'm not majorly concerned about making users inherit from a class. They already have a hard dependency on xarray, so there isn't a strong need for using a protocol.

We could also (optionally?) inherit BackendEntrypoint from typing.Protocol, although that requires Python 3.9 or typing_extensions

Copy link
Collaborator Author

@aurghs aurghs Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any advantage in forcing the user to inherit from our class.
typing.Protocol seems to be the best solution, but I would avoid adding new requirements only for this.

@alexamici alexamici merged commit 8cc34cb into pydata:master Jan 28, 2021
@alexamici alexamici changed the title WIP: backend interface, now it uses subclassing backend interface, now it uses subclassing Jan 28, 2021
dcherian added a commit to TomNicholas/xarray that referenced this pull request Jan 29, 2021
@dcherian
Merge branch 'master' into integrate_dim_arg
170cc62 
* master:
  WIP: backend interface, now it uses subclassing  (pydata#4836)
  weighted: small improvements (pydata#4818)
  Update related-projects.rst (pydata#4844)
  iris update doc url (pydata#4845)
  Faster unstacking (pydata#4746)
  Allow swap_dims to take kwargs (pydata#4841)
  Move skip ci instructions to contributing guide (pydata#4829)
  fix issues in drop_sel and drop_isel (pydata#4828)
dcherian added a commit to dcherian/xarray that referenced this pull request Jan 29, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into dataset/quiver
e0f227f 
* upstream/master:
  speed up the repr for big MultiIndex objects (pydata#4846)
  dim -> coord in DataArray.integrate (pydata#3993)
  WIP: backend interface, now it uses subclassing  (pydata#4836)
  weighted: small improvements (pydata#4818)
  Update related-projects.rst (pydata#4844)
  iris update doc url (pydata#4845)
  Faster unstacking (pydata#4746)
  Allow swap_dims to take kwargs (pydata#4841)
  Move skip ci instructions to contributing guide (pydata#4829)
  fix issues in drop_sel and drop_isel (pydata#4828)
  Bugfix in list_engine (pydata#4811)
  Add drop_isel (pydata#4819)
  Fix RST.
  Remove the references to `_file_obj` outside low level code paths, change to `_close` (pydata#4809)
dcherian added a commit to dcherian/xarray that referenced this pull request Feb 3, 2021
@dcherian
Merge branch 'master' into groupby-plot
fd202df 
* master: (458 commits)
  Add units if "unit" is in the attrs. (pydata#4850)
  speed up the repr for big MultiIndex objects (pydata#4846)
  dim -> coord in DataArray.integrate (pydata#3993)
  WIP: backend interface, now it uses subclassing  (pydata#4836)
  weighted: small improvements (pydata#4818)
  Update related-projects.rst (pydata#4844)
  iris update doc url (pydata#4845)
  Faster unstacking (pydata#4746)
  Allow swap_dims to take kwargs (pydata#4841)
  Move skip ci instructions to contributing guide (pydata#4829)
  fix issues in drop_sel and drop_isel (pydata#4828)
  Bugfix in list_engine (pydata#4811)
  Add drop_isel (pydata#4819)
  Fix RST.
  Remove the references to `_file_obj` outside low level code paths, change to `_close` (pydata#4809)
  fix decode for scale/ offset list (pydata#4802)
  Expand user dir paths (~) in open_mfdataset and to_zarr. (pydata#4795)
  add a version info step to the upstream-dev CI (pydata#4815)
  fix the ci trigger action (pydata#4805)
  scatter plot by order of the first appearance of hue (pydata#4723)
  ...
dcherian added a commit to DWesl/xarray that referenced this pull request Feb 11, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into read_grid_mapping…
b65e579 
…_and_bounds_as_coords

* upstream/master: (51 commits)
  Ensure maximum accuracy when encoding and decoding cftime.datetime values (pydata#4758)
  Fix `bounds_error=True` ignored with 1D interpolation (pydata#4855)
  add a drop_conflicts strategy for merging attrs (pydata#4827)
  update pre-commit hooks (mypy) (pydata#4883)
  ensure warnings cannot become errors in assert_ (pydata#4864)
  update pre-commit hooks (pydata#4874)
  small fixes for the docstrings of swap_dims and integrate (pydata#4867)
  Modify _encode_datetime_with_cftime for compatibility with cftime > 1.4.0 (pydata#4871)
  vélin (pydata#4872)
  don't skip the doctests CI (pydata#4869)
  fix da.pad example for numpy 1.20 (pydata#4865)
  temporarily pin dask (pydata#4873)
  Add units if "unit" is in the attrs. (pydata#4850)
  speed up the repr for big MultiIndex objects (pydata#4846)
  dim -> coord in DataArray.integrate (pydata#3993)
  WIP: backend interface, now it uses subclassing  (pydata#4836)
  weighted: small improvements (pydata#4818)
  Update related-projects.rst (pydata#4844)
  iris update doc url (pydata#4845)
  Faster unstacking (pydata#4746)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Feb 12, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into fix/4107
8ce7175 
* upstream/master: (24 commits)
  Compatibility with dask 2021.02.0 (pydata#4884)
  Ensure maximum accuracy when encoding and decoding cftime.datetime values (pydata#4758)
  Fix `bounds_error=True` ignored with 1D interpolation (pydata#4855)
  add a drop_conflicts strategy for merging attrs (pydata#4827)
  update pre-commit hooks (mypy) (pydata#4883)
  ensure warnings cannot become errors in assert_ (pydata#4864)
  update pre-commit hooks (pydata#4874)
  small fixes for the docstrings of swap_dims and integrate (pydata#4867)
  Modify _encode_datetime_with_cftime for compatibility with cftime > 1.4.0 (pydata#4871)
  vélin (pydata#4872)
  don't skip the doctests CI (pydata#4869)
  fix da.pad example for numpy 1.20 (pydata#4865)
  temporarily pin dask (pydata#4873)
  Add units if "unit" is in the attrs. (pydata#4850)
  speed up the repr for big MultiIndex objects (pydata#4846)
  dim -> coord in DataArray.integrate (pydata#3993)
  WIP: backend interface, now it uses subclassing  (pydata#4836)
  weighted: small improvements (pydata#4818)
  Update related-projects.rst (pydata#4844)
  iris update doc url (pydata#4845)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoyer shoyer shoyer left review comments

@jhamman jhamman jhamman left review comments

@alexamici alexamici alexamici approved these changes

Assignees
No one assigned
Labels
grant-czi topic-backends
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aurghs @alexamici @shoyer @jhamman