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

include accessors in __dir__ #9985

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+36 −6
Open

include accessors in __dir__ #9985

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5314f02
add `_accessors` as a place to register accessor names
keewis Jan 25, 2025
4dc0527
include the accessor in the new set upon registry
keewis Jan 25, 2025
bc5a25b
include the accessor names in `__dir__`
keewis Jan 25, 2025
9433b64
actually register on `DataTree`
keewis Jan 25, 2025
2b7d0c8
check that the registered accessors are included in `__dir__`
keewis Jan 25, 2025
050d834
fix the wrong type hint
keewis Jan 25, 2025
9862105
changelog
keewis Jan 25, 2025
580fd3f
mark `_accessor` as a class variable
keewis Jan 26, 2025
bdc7984
access `_accessors` as a class variable
keewis Jan 26, 2025
92d9f89
Merge branch 'main' into accessors-dir
dcherian Jan 27, 2025
336eb26
use a mixin to provide the extension discoverability
keewis Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/whats-new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ Bug fixes
By `Kai Mühlbauer <https://github.com/kmuehlbauer>`_.
- Fix weighted ``polyfit`` for arrays with more than two dimensions (:issue:`9972`, :pull:`9974`).
By `Mattia Almansi <https://github.com/malmans2>`_.
- Include accessors in ``__dir__`` (:pull:`9985`).
By `Justus Magin <https://github.com/keewis>`_.

Documentation
~~~~~~~~~~~~~
Expand Down
2 changes: 1 addition & 1 deletion xarray/core/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ def __dir__(self) -> list[str]:
for item in source
if isinstance(item, str)
}
return sorted(set(dir(type(self))) | extra_attrs)
return sorted(set(dir(type(self))) | type(self)._accessors | extra_attrs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this has become a bit messy: while _accessors is a class variable, it is only available on its subclass. To really make sense we'd have to override DataTree.__dir__, DataArray.__dir___ and Dataset.__dir__ with

def __dir__(self):
    return sorted(set(super().__dir__()) | type(self)._accessors)

or create a new mixin containing _accessors and have all three classes inherit from that (after AttrAccessMixin, so super().__dir__() dispatches to that).

However, set(dir(type(self))) already uses a similar trick (defer to the subclass type's __dir__) so we may also choose to ignore the typing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What stops you from adding _accessors: ClassVar[set[str]] to this mixing class?

Copy link
Collaborator Author

@keewis keewis Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

mostly that this is a mixin specialized on providing attribute-based syntax (using .) for entries in attrs. I guess it would be best to have another mixin for the accessors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've created a separate mixin, which allows a separation of concerns (and makes removing the AttrAccessMixin easier, should we ever wish to). The downside is that now we have one more base class / mixin.


def _ipython_key_completions_(self) -> list[str]:
"""Provide method for the key-autocompletions in IPython.
Expand Down
2 changes: 2 additions & 0 deletions xarray/core/dataarray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from typing import (
TYPE_CHECKING,
Any,
ClassVar,
Generic,
Literal,
NoReturn,
Expand Down Expand Up @@ -421,6 +422,7 @@ class DataArray(
_indexes: dict[Hashable, Index]
_name: Hashable | None
_variable: Variable
_accessors: ClassVar[set[str]] = set()

__slots__ = (
"__weakref__",
Expand Down
3 changes: 2 additions & 1 deletion xarray/core/dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from operator import methodcaller
from os import PathLike
from types import EllipsisType
from typing import IO, TYPE_CHECKING, Any, Generic, Literal, cast, overload
from typing import IO, TYPE_CHECKING, Any, ClassVar, Generic, Literal, cast, overload

import numpy as np
from pandas.api.types import is_extension_array_dtype
Expand Down Expand Up @@ -709,6 +709,7 @@ class Dataset(
_close: Callable[[], None] | None
_indexes: dict[Hashable, Index]
_variables: dict[Hashable, Variable]
_accessors: ClassVar[set[str]] = set()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using a mutable default is bad practice.
But the only issue I can imagine is custom subclasses that mess with this? So maybe it's fine.

Copy link
Collaborator Author

@keewis keewis Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, the only issue would be that subclasses inherit this and try to modify this. However, if they do it would probably be better to just have them override the class variable.

(Additionally, as far as I understand the class is an instance of the metaclass, so having the usual "sentinel then switch to mutable on first access" doesn't make sense for class variables)


__slots__ = (
"__weakref__",
Expand Down
3 changes: 2 additions & 1 deletion xarray/core/datatree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
Mapping,
)
from html import escape
from typing import TYPE_CHECKING, Any, Literal, NoReturn, Union, overload
from typing import TYPE_CHECKING, Any, ClassVar, Literal, NoReturn, Union, overload

from xarray.core import utils
from xarray.core._aggregations import DataTreeAggregations
Expand Down Expand Up @@ -455,6 +455,7 @@ class DataTree(
_attrs: dict[Hashable, Any] | None
_encoding: dict[Hashable, Any] | None
_close: Callable[[], None] | None
_accessors: ClassVar[set[str]] = set()

__slots__ = (
"_attrs",
Expand Down
1 change: 1 addition & 0 deletions xarray/core/extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ def decorator(accessor):
stacklevel=2,
)
setattr(cls, name, _CachedAccessor(name, accessor))
cls._accessors.add(name)
return accessor

return decorator
Expand Down
4 changes: 4 additions & 0 deletions xarray/tests/test_extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ def __init__(self, xarray_obj):
def foo(self):
return "bar"

assert "demo" in dir(xr.DataTree)
assert "demo" in dir(xr.Dataset)
assert "demo" in dir(xr.DataArray)
Comment on lines +37 to +39
Copy link
Member

Choose a reason for hiding this comment

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

Does this PR also allow completion of the actual accessor methods, or just the accessor namespace? If so then we should test that too.

Copy link
Collaborator Author

@keewis keewis Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it does, but but how would you test that? This is functionality of the auto-completer...

Edit: to be clear, I think auto-completion of the namespace was already working before this PR

Copy link
Collaborator Author

@keewis keewis Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

actually, I just tried dir(ds) on main, and it included the accessor... so it might be that this PR is not necessary? Can anyone confirm that? Or did I manage to confuse myself?

Copy link
Member

Choose a reason for hiding this comment

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

it does, but but how would you test that?

Surely just

ds = xr.Dataset()
assert "foo" in dir(ds.demo)

?

I just tried dir(ds) on main, and it included the accessor... so it might be that this PR is not necessary? Can anyone confirm that? Or did I manage to confuse myself?

I just tried seeing if xr.Dataset().d would suggest xr.Dataset().dt (because that's a built-in accessor) and for me it does not.

Copy link
Collaborator Author

@keewis keewis Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

dt and str are only on DataArray

Edit: And assert "foo" in dir(ds.demo) should be the same as assert "foo" in dir(DemoAccessor), no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are any of you using a different IDE that does show the expected code-completion suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

I think @keewis is right, most IDEs nowadays support auto-completion via LSP servers based on (somewhat strict) static code analysers such as pyright / pylance. So I'm afraid there's nothing much we can do to support auto-completion for dynamic attributes.

IDE configs using jedi under the hood may support it, although IIUC jedi operates under different modes (Interpreter vs. Script) so I'm not sure auto-completion for dynamic attributes will always work (probably only in the Interpreter mode?).

within the IDE there's still something that has to be evaluated with regard to decorators in order for the final definition of the things that are decorated to be available to the IDE.

Decorators are just syntax sugar for functions applied on other functions or classes, so if the decorator function is properly typed (i.e., its return type is clear or can be statically inferred) any static code analyser will be able to provide auto-completions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried to enable this for mypy here: #7117
But failed because the documentation on mypy plugins is non existent.

Maybe something similar is possible for pyright, pylance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the insight @benbovy. Sadly, for me at least, this seems to confirm one of the reasons that I'm not a fan of dynamically registered accessors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@max-sixty, if I remember correctly, you're also using VSCode? Do you have any additional insight here?

Sorry, just saw this.

if I understand correctly, VSCode builds its code completion on type stubs and type inference (through pylance), which means that dynamic attributes like accessors may not be possible to autocomplete.

This is also my understanding — a kernel will give better info than an IDE because it's doing something like calling dir(foo) rather than statically analyzing the code...


dt: xr.DataTree = xr.DataTree()
assert dt.demo.foo == "bar"

Expand Down
Loading