include accessors in `dir` #9985

keewis · 2025-01-25T19:57:59Z

We were not including accessors in __dir__ so far, which would mean that dynamic code completion was not suggesting any accessors.

This can't make use of _cache, which is related but an instance variable, and will only be populated on the first access of each accessor (by _CachedAccessor).

xref Functions or accessors for serialization API? zarr-developers/VirtualiZarr#389
Tests added
User visible changes (including notable bug fixes) are documented in whats-new.rst

keewis · 2025-01-25T22:11:27Z

(the failing mypy tests are unrelated)

Otherwise this is ready for reviewing

max-sixty

Looks great, thanks!

keewis · 2025-01-26T16:51:36Z

xarray/core/common.py

@@ -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)


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.

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

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

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.

xarray/core/dataarray.py

headtr1ck · 2025-01-26T21:16:24Z

xarray/core/common.py

@@ -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)


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

headtr1ck · 2025-01-26T21:18:04Z

xarray/core/dataset.py

@@ -709,6 +709,7 @@ class Dataset(
    _close: Callable[[], None] | None
    _indexes: dict[Hashable, Index]
    _variables: dict[Hashable, Variable]
+    _accessors: ClassVar[set[str]] = set()


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.

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)

TomNicholas · 2025-01-28T19:56:06Z

xarray/tests/test_extensions.py

+        assert "demo" in dir(xr.DataTree)
+        assert "demo" in dir(xr.Dataset)
+        assert "demo" in dir(xr.DataArray)


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

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

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?

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.

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?

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.

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

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

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.

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?

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.

keewis added 4 commits January 25, 2025 20:49

add _accessors as a place to register accessor names

5314f02

include the accessor in the new set upon registry

4dc0527

include the accessor names in __dir__

bc5a25b

actually register on DataTree

9433b64

keewis mentioned this pull request Jan 25, 2025

Functions or accessors for serialization API? zarr-developers/VirtualiZarr#389

Open

check that the registered accessors are included in __dir__

2b7d0c8

keewis changed the title ~~include accessors in `__dir__~~ include accessors in __dir__ Jan 25, 2025

keewis added 2 commits January 25, 2025 21:08

fix the wrong type hint

050d834

changelog

9862105

max-sixty approved these changes Jan 25, 2025

View reviewed changes

keewis added 2 commits January 26, 2025 15:59

mark _accessor as a class variable

580fd3f

access _accessors as a class variable

bdc7984

keewis commented Jan 26, 2025

View reviewed changes

headtr1ck reviewed Jan 26, 2025

View reviewed changes

dcherian and others added 2 commits January 27, 2025 13:07

Merge branch 'main' into accessors-dir

92d9f89

use a mixin to provide the extension discoverability

336eb26

TomNicholas reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include accessors in `dir` #9985

include accessors in `dir` #9985

keewis commented Jan 25, 2025 •

edited

Loading

keewis commented Jan 25, 2025 •

edited

Loading

max-sixty left a comment

keewis Jan 26, 2025

headtr1ck Jan 26, 2025

keewis Jan 27, 2025 •

edited

Loading

keewis Jan 28, 2025

headtr1ck Jan 26, 2025

headtr1ck Jan 26, 2025

keewis Jan 27, 2025 •

edited

Loading

TomNicholas Jan 28, 2025

keewis Jan 28, 2025 •

edited

Loading

keewis Jan 28, 2025 •

edited

Loading

TomNicholas Jan 28, 2025

keewis Jan 28, 2025 •

edited

Loading

keewis Jan 29, 2025

chuckwondo Jan 29, 2025

benbovy Jan 31, 2025

headtr1ck Jan 31, 2025

chuckwondo Jan 31, 2025

include accessors in __dir__ #9985

Are you sure you want to change the base?

include accessors in __dir__ #9985

Conversation

keewis commented Jan 25, 2025 • edited Loading

keewis commented Jan 25, 2025 • edited Loading

max-sixty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keewis Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keewis Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keewis Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

keewis Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keewis Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

include accessors in `dir` #9985

include accessors in `dir` #9985

keewis commented Jan 25, 2025 •

edited

Loading

keewis commented Jan 25, 2025 •

edited

Loading

keewis Jan 27, 2025 •

edited

Loading

keewis Jan 27, 2025 •

edited

Loading

keewis Jan 28, 2025 •

edited

Loading

keewis Jan 28, 2025 •

edited

Loading

keewis Jan 28, 2025 •

edited

Loading