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.

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

list available backends and basic descriptors #7000

Merged
merged 42 commits into from
Oct 17, 2022

Conversation

JessicaS11
Copy link
Contributor

@JessicaS11 JessicaS11 commented Sep 6, 2022

Wanted to garner maintainer feedback on the basic implementation before I add any docs. I will need some guidance for adding tests.

  • xref ENH: list_engines function #6577
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@headtr1ck
Copy link
Collaborator

I know that it is more work but I think it would be more beneficial to have this information formated nicely than simply returning some lists.
Maybe similar to a pandas dataframe?

@JessicaS11
Copy link
Contributor Author

I know that it is more work but I think it would be more beneficial to have this information formated nicely than simply returning some lists.

Sounds good - I'll make that happen!

@TomNicholas
Copy link
Contributor

Hey @JessicaS11! Nice to see you here. This is a going to be a great first-time contribution!

I know that it is more work but I think it would be more beneficial to have this information formated nicely than simply returning some lists.
Maybe similar to a pandas dataframe?

One approach might be to add a nice __str__, __repr__, (or even _repr_html_) to the BackendEntrypoint class. That way printing a list (or better a dict) of the engine-specific subclasses would automatically look decent. Something roughly like

class BackendEntryPoint:
    ...

    def __str__(self) -> str:
        txt += self.backend_description
        if self.backend_url:
            txt += f"\nFind more info at {self.backend_url}"
        return txt

What might be clearer is if this backend class knew it's own name, but I'm not sure it can, hmmm

xarray/backends/common.py Outdated Show resolved Hide resolved
xarray/backends/common.py Outdated Show resolved Hide resolved
xarray/backends/api.py Outdated Show resolved Hide resolved
@headtr1ck
Copy link
Collaborator

One approach might be to add a nice __str__, __repr__, (or even _repr_html_) to the BackendEntrypoint class. That way printing a list (or better a dict) of the engine-specific subclasses would automatically look decent.

That would not display in a nice table though. But might be anyway nice to add ;)

What might be clearer is if this backend class knew it's own name, but I'm not sure it can, hmmm

What about type(self).__name__

@TomNicholas
Copy link
Contributor

TomNicholas commented Sep 8, 2022

That would not display in a nice table though. But might be anyway nice to add ;)

So what would the return type need to be to get a nice table representation, but also allow you to select out individual backend objects? A pandas object containing the backend objects? Some kind of BackendList class?

What about type(self).__name__

Correct me if I'm wrong, but if a subclassed ZarrBackend object was added as an entrypoint under the key "zarr", wouldn't type(self).__name__ return "ZarrBackend" rather than "zarr", even though "zarr" is what the user would actually have to pass to the engine kwarg of open_dataset to use that backend? Not ideal :/

@headtr1ck
Copy link
Collaborator

Correct me if I'm wrong, but if a subclassed ZarrBackend object was added as an entrypoint under the key "zarr", wouldn't type(self).__name__ return "ZarrBackend" rather than "zarr", even though "zarr" is what the user would actually have to pass to the engine kwarg of open_dataset to use that backend? Not ideal :/

You're right. But this mapping has to be somewhere.

xarray/backends/common.py Outdated Show resolved Hide resolved
@JessicaS11
Copy link
Contributor Author

That would not display in a nice table though. But might be anyway nice to add ;)

Agreed. I added a __str__ to the BackendEntrypoint class, but...

So what would the return type need to be to get a nice table representation, but also allow you to select out individual backend objects? A pandas object containing the backend objects? Some kind of BackendList class?

... I haven't dreamed up an answer to this question yet. We could set it up so that __str__ instead returns a pandas series, but I don't see another way we can have a pretty __str__ that will also directly feed into a nicely formated table for avail_engines().

@TomNicholas
Copy link
Contributor

TomNicholas commented Sep 28, 2022

Does anyone know if the backentrypoints are supposed to have an available attribute?

Looks like they are - every subclass of BackendEntryPoint seems to define .available. In each case it points to the corresponding has_[package] flag. I think it might be safe to set .available to default to True in the base class? That way if the backend requires no special dependencies it would by default return True. If we think that makes sense then I'm happy to add a commit doing that so this can be merged.

EDIT: (Actually we should wait to put tests in too really)

doc/whats-new.rst Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

dcherian commented Oct 3, 2022

We discussed this with Jessica in the xarray community meeting today

Did you discuss adding xr.show_engines() or are we pointing all users to xr.backends.list_engines()?

@headtr1ck
Copy link
Collaborator

Looks like they are - every subclass of BackendEntryPoint seems to define .available. In each case it points to the corresponding has_[package] flag. I think it might be safe to set .available to default to True in the base class? That way if the backend requires no special dependencies it would by default return True. If we think that makes sense then I'm happy to add a commit doing that so this can be merged.

EDIT: (Actually we should wait to put tests in too really)

In #7114 I have added this class attribute with default = True. Maybe we can merge this first?

Which tests did you want to see?

@headtr1ck
Copy link
Collaborator

@JessicaS11 if you merge master and solve the conflicts this should be enough for a merge :)

@dcherian dcherian added the plan to merge Final call for comments label Oct 13, 2022
@headtr1ck
Copy link
Collaborator

Couple of minor comments, then we can merge.

@JessicaS11
Copy link
Contributor Author

Couple of minor comments, then we can merge.

@headtr1ck GitHub is unhelpfully not showing me any comments. Can you share a screen shot or something so I can address them? Thanks!

xarray/backends/common.py Show resolved Hide resolved
xarray/backends/common.py Show resolved Hide resolved
xarray/backends/common.py Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
@headtr1ck
Copy link
Collaborator

@headtr1ck GitHub is unhelpfully not showing me any comments. Can you share a screen shot or something so I can address them? Thanks!

Forgot to click the ok button, haha

dcherian and others added 3 commits October 17, 2022 09:31
* main:
  Add import ASV benchmark (pydata#7176)
  Rework docs about scatter plots (pydata#7169)
  Fix some scatter plot issues (pydata#7167)
  Fix doctest warnings, enable errors in CI (pydata#7166)
  fix broken test (pydata#7168)
  Add typing to plot methods (pydata#7052)
  Fix warning in doctest (pydata#7165)
Co-authored-by: Mick <mick.niklas@gmail.com>
@dcherian
Copy link
Contributor

@headtr1ck the typing changes didn't work. Can you send in a separate PR for that please?

@dcherian dcherian enabled auto-merge (squash) October 17, 2022 16:04
@headtr1ck
Copy link
Collaborator

@headtr1ck the typing changes didn't work. Can you send in a separate PR for that please?

Ok, weird. But can do :)

@dcherian dcherian merged commit e911306 into pydata:main Oct 17, 2022
@JessicaS11 JessicaS11 deleted the listbackends branch June 20, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants