__slots__

[crusaderky]

What changes:

• Most classes now define __slots__
• removed _initialized property
• Enforced checks that all subclasses must also define __slots__. For third-party subclasses, this is for now a DeprecationWarning and should be changed into a hard crash later on.
• 22% reduction in RAM usage
• 5% performance speedup for a DataArray method that performs a _to_temp_dataset roundtrip

DISCUSS: support for third party subclasses is very poor at the moment (#1097). Should we skip the deprecation altogether?

Performance benchmark:

import timeit
import psutil
import xarray

a = xarray.DataArray([1, 2], dims=['x'], coords={'x': [10, 20]})
RUNS = 10000
t = timeit.timeit("a.roll(x=1, roll_coords=True)", globals=globals(), number=RUNS)
print("{:.0f} us".format(t / RUNS * 1e6))

p = psutil.Process()
N = 100000
rss0 = p.memory_info().rss
x = [
    xarray.DataArray([1, 2], dims=['x'], coords={'x': [10, 20]})
    for _ in range(N)
]
rss1 = p.memory_info().rss
print("{:.0f} bytes".format((rss1 - rss0) / N))

Output:

test	env	master	slots
DataArray.roll	py35-min	332 us	360 us
DataArray.roll	py37	354 us	337 us
RAM usage of a DataArray	py35-min	2755 bytes	2074 bytes
RAM usage of a DataArray	py37	1970 bytes	1532 bytes

The performance degradation on Python 3.5 is caused by the deprecation mechanism - see changes to common.py.

I honestly never realised that xarray objects are measured in kilobytes (vs. 32 bytes of underlying buffers!)

[max-sixty]

V interesting!

Is there any downside? IIRC objects don't have a __dict__, so attributes can't be set, which I don't think matters in this case at all? Is there any risk that downstream packages break?

[shoyer]

I'm glad to see you were able to get rid of _initialized and speed things up!

It might be (harmless) premature optimization to use __slots__ for every internal xarray class. But certainly doing it for class involved in constructing every xarray object (e.g., Variable/DataArray) makes sense.

[shoyer]

To clarify, in the future (Python 3.6+ only, post deprecation warning) will we be able to remove assignment to cls.__setattr__ from __init_subclass__? I'd rather avoid dynamically building methods if possible.

[crusaderky]

Post deprecation warning and after dropping support for py35, there will only be __setattr__, statically implemented with what is now the body of _setattr_slots. __init_subclass__ will retain only the health check.

[crusaderky]

@max-sixty any package that for any reason expects xarray objects to have a __dict__ will break overnight. Any package that subclasses from xarray will get a warning.

[shoyer]

We definitely do not guarantee implementation details like xarray objects having a __dict__. A warning for subclasses is already going above and beyond IMO.

…

On Fri, Aug 23, 2019 at 2:42 PM crusaderky ***@***.***> wrote: @max-sixty <https://github.com/max-sixty> any package that for any reason expects xarray objects to have a __dict__ will break overnight. Any package that subclasses from xarray will get a warning. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3250?email_source=notifications&email_token=AAJJFVUD2QSIIOF5DHLCGUTQGBDR5A5CNFSM4IO7AUA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BI4IA#issuecomment-524455456>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJJFVUYHG37O2SWYURYRDDQGBDR5ANCNFSM4IO7AUAQ> .

[crusaderky]

@shoyer this is theoretically done, however the design I had to come up with to preserve the current statefulness of user-defined accessors displeases me and I'd rather clean it up. See #3268 for design decision.

[crusaderky]

Added automated unit tests for the automated check on __slots__ for all objects inside the xarray library. I could not find a straightforward way to automatically test that objects defined outside of the library log FutureWarning instead - I tested it by hand.

[crusaderky]

Python 3.7:

In [1]: class A(xarray.DataArray): 
   ...:     pass 
   ...:                                                                                                                                                                                                                                                        
miniconda3/envs/xarray-py37/bin/ipython:1: FutureWarning: xarray subclass A should explicitly define __slots__

In [2]: a = A()                                                                                                                                                                                                                                                

In [3]: a.x = 1                                                                                                                                                                                                                                                
miniconda3/envs/xarray-py37/bin/ipython:1: FutureWarning: Setting attribute 'x' on a 'A' object. Explicitly define __slots__ to suppress this warning for legitimate custom attributes and raise an error when attempting variables assignments.

In [4]: a.x                                                                                                                                                                                                                                                   
Out[4]: 1

In [5]: a.__dict__                                                                                                                                                                                                                                             
Out[5]: {'x': 1}

Python 3.5: Same, but without the first FutureWarning.

[crusaderky]

Ready for merge if there are no additional comments

[shoyer]

thanks @crusaderky !

[max-sixty]

👍 Thanks @crusaderky

[crusaderky]

👍