perf(python): Add __slots__ to most Polars classes

[Object905]

Closes #13198

In this PR are "safe" additions of __slots__. Mostly for classes that are not usually subclassed by users - pl.Expr, its subclasses and build in namespaces.
polars.datatypes.classes also really good candidates, since they are instantiated now, but not included in this PR (yet?).

Adding __slots__ for LazyFrame (and DataFrame/Series) turned out to be a breaking change because of subclassing.
Using ldf.__class__ = SubClassedLazyFrame, like in test_preservation_of_subclasses is an error because of different object layouts. And even if subclass were to define __slots__ - they can only be empty, without additional attributes on instance to keep this assigment valid.
That can't be solved without specific conversion mechanism to subclasses to properly create a new instance (like pl.LazyFrame.as_subclass(SubClassedLazyFrame, *args, **kwargs)) where _ldf attribute itself would be passed/assigned to new instance of subclass.
Also using ldf.__class__ = SubClassedLazyFrame a dirty hack anyway and if polars were to properly support subclassing of frames, I think something similar should be in place - then __slots__ on them can be added safely.

Deeper description about __slots__ and benefits\downsides can be found here.
Also in issue were not mentioned memory usage benefits aside from slightly faster attribute access.
(sqlalchemy claimed 46% less memory usage for it's internal structures). Even though in latest python versions this gap should be smaller, due to more compact dict.

[Object905]

I think this were not required even before, but it served as "cache" on instance of expression.
Now it gives error because instance does not have __dict__ and _accessor goes straight to descriptor - and it's read only.

But since expressions are frequently recreated, then this "cache" were only working on chained namespace access like
pl.col("foo").namespace_foo.func().namespace_foo.other_func()

[stinodego]

I'd appreciate if @alexander-beedie could sign off on this change before this gets merged. I don't really see exactly what is going on here.

[stinodego]

Turns out this did have user impact:
#14851

For this reason, and some related issues popping up, I will be reverting this PR.

[stinodego]

Thanks for the PR. Could you provide some data on the performance impact of this change? I think I'm in favor of this change, but it would be nice to have some numbers to support it.

About subclassing: we don't support this. That test is from a while ago and we no longer guarantee preserving the class on calling a method.

[Object905]

It's hard to measure exactly, because of variety of possible use cases. But speedup is small, which is anticipated.

In my production use case constructing pl.Expr and pl.LazyFrame with all the glue takes ~20% of request runtime, while collecting them only about 10% because dataframes are pretty small (100-2000 rows).
And given that this change is sadly close to noise.

Anyway here is some synthetic bench.

upstream

In [5]: %timeit -n 5000 pl.when(pl.col("foo") == 1).then("bar").when(pl.col("foo") == 2).then(pl.col("bar") / 2).when(pl.col("bar") == p
   ...: l.col("foo")).then("baz").otherwise(pl.col("bar").list.eval(pl.element().str.starts_with("ni")))
34.4 µs ± 480 ns per loop (mean ± std. dev. of 7 runs, 5,000 loops each)

In [2]: %timeit -n 10000 -r 100 pl.col("foo").alias("bar")
1.41 µs ± 43.8 ns per loop (mean ± std. dev. of 100 runs, 10,000 loops each)

In [3]: sys.getsizeof(pl.col.foo)
Out[3]: 48

this PR

In [7]: %timeit -n 5000 pl.when(pl.col("foo") == 1).then("bar").when(pl.col("foo") == 2).then(pl.col("bar") / 2).when(pl.col("bar") == p
   ...: l.col("foo")).then("baz").otherwise(pl.col("bar").list.eval(pl.element().str.starts_with("ni")))
33.5 µs ± 505 ns per loop (mean ± std. dev. of 7 runs, 5,000 loops each)

In [3]: %timeit -n 10000 -r 100 pl.col("foo").alias("bar")
1.33 µs ± 41.3 ns per loop (mean ± std. dev. of 100 runs, 10,000 loops each)

In [3]: sys.getsizeof(pl.col.foo)
Out[3]: 40

Also docs are failing to build because of this, because sphinx tries to get __dict__ on namespaces to check for classmethod/staticmethod.
If it's acceptable, I will add workaround for docs and __slots__ to other wrappers (LazyFrame/DataFrame/Series) later, otherwise might as well close this PR.

[Object905]

Ready to be merged or closed 😐

[alexander-beedie]

Ready to be merged or closed 😐

If you can rebase against the latest code I'll see about reviewing this 👌

[Object905]

@alexander-beedie
Sorry for disappearance. Rebased main.

[deanm0000]

Does this prevent monkey patching?

I use pl.Expr.some_func = lambda x: x #some cool thing from time to time and it seems __slots__ would prevent that.

Also, what about this syntax pl.col.a instead of pl.col('a')?

[Object905]

@deanm0000
pl.Expr.some_func = ... modifies class __dict__, which is still in place. But this would prevent

instance = pl.col("a")
instance.some_func = ...

Also suggesting to take a look at pipe method on expression for this, it also plays well with typing.

pl.col.a works, because its implemented with getattr.

[stinodego]

Thanks, I left some comments.

I think Series and DataFrame can also get a __slots__ attribute. We do not guarantee subclass preservation, so I removed the related tests in another PR.

The way I see it, the biggest blocker for this PR for now is the Sphinx issue, which we should be able to address. Or we could add slots for these namespace classes later.

[stinodego]

I'd appreciate if @alexander-beedie could sign off on this change before this gets merged. I don't really see exactly what is going on here.

[stinodego]

I'm not a fan of this construction. I would expect the sphinx_accessor to take care of this, but it doesn't. We should fix that directly rather than introducing this check everywhere.

It seems to be a problem with sphinx-autosummary-accessors.

[Object905]

Agree on this. Will try to look into this.
I've tried monkey-patching, but dynamically adding __dict__ is way harder when __slots__ are in place, because it modifies object layout.
Problem seems to be that autosummary makes objects into "modules" and sphinx expects module objects to have dict, so exception thrown from sphinx itself.
Maybe will try to make subclass with __dict__ dynamically.

[Object905]

Created pull request to handle this xarray-contrib/sphinx-autosummary-accessors#124

[stinodego]

That's great! They have not released in a year though, so I don't expect this to be picked up quickly.

I'd suggest removing the namespace slots for now - we can add them again when the issue is resolved.

[alexander-beedie]

@alexander-beedie
Sorry for disappearance. Rebased main.

Thx; will look at the weekend

[codecov]

Codecov Report

Attention: Patch coverage is 82.79570% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 80.78%. Comparing base (0e63403) to head (4ed1b8b).
Report is 15 commits behind head on main.

❗ Current head 4ed1b8b differs from pull request most recent head 7da83bd. Consider uploading reports for the commit 7da83bd to get more accurate results

Files	Patch %	Lines
py-polars/polars/expr/array.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/binary.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/categorical.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/datetime.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/list.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/meta.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/name.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/string.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/expr/struct.py	66.66%	0 Missing and 1 partial ⚠️
py-polars/polars/series/array.py	66.66%	0 Missing and 1 partial ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13236      +/-   ##
==========================================
- Coverage   80.92%   80.78%   -0.15%     
==========================================
  Files        1328     1326       -2     
  Lines      173446   173123     -323     
  Branches     2455     2455              
==========================================
- Hits       140369   139865     -504     
- Misses      32605    32771     +166     
- Partials      472      487      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stinodego]

I removed the namespace slots for now. Let's merge this and see if we get any user feedback on this. If not, that would be a good thing :)

[lorentzenchr]

@stinodego May I ask what the decision criteria for inclusion finally was? I'm really just curious because I considered something similar in scikit-learn and measurement showed no performance improvement at all, quite similar to #13236 (comment) did neither.

[stinodego]

@stinodego May I ask what the decision criteria for inclusion finally was? I'm really just curious because I considered something similar in scikit-learn and measurement showed no performance improvement at all, quite similar to #13236 (comment) did neither.

I like that this clearly documents/enforces what the instance variables are. That is the most important thing for me.

I found out later that this changes the behavior of getattr and I'm not sure I'm happy with that:

class A:
    __slots__ = ("x",)

print(getattr(A, "x", None))
# <member 'x' of 'A' objects>       <-- without slots this would print None

But besides that, I don't see any big downsides so might as well give this a try. There may be some users out there that hit a specific case where the potential performance benefits actually help them.

[lorentzenchr]

@stinodego Thanks for the explanation.