[mypyc] Introduce ClassBuilder and add support for attrs classes #11328

chadrik · 2021-10-13T18:28:32Z

Description

attrs classes are very similar to dataclasses when used with auto_attribs=True (which tells attrs use to type annotations), so it was fairly trivial to add support for them to mypyc.

This MR also fixes a bug with specialization of functions like dataclasses.field that when they are accessed from their module rather than the globals. this was necessary because the idiomatic way to use attrs is @attr.s and attr.ib.

I was wondering if I should skip the special treatment of attrs classes if auto_attribs is not enabled, but if the attrs class is not using annotations then the compiled module wouldn't work whether mypyc skipped it or not, so I'm not sure if it's worth the extra complexity and potential fragility of doing that check.

Test Plan

I added tests for attrs that are based off of the dataclasses tests, and I also modified the dataclasses test to demonstrate the problem with specialization of dataclasses.field.

prior to this change dataclasses.field would only be specialized if called without the module, as field()

chadrik · 2021-10-13T18:29:06Z

mypyc/ir/class_ir.py

+            "name={self.name}, module_name={self.module_name}, "
+            "is_trait={self.is_trait}, is_generated={self.is_generated}, "
+            "is_abstract={self.is_abstract}, is_ext_class={self.is_ext_class}"
+            ")".format(self=self))


Just a convenience. This is the sort of thing dataclasses are great for!

mypyc/irbuild/expression.py

chadrik · 2021-10-13T18:37:27Z

mypyc/irbuild/expression.py

    elif isinstance(callee, SuperExpr):
        return translate_super_method_call(builder, expr, callee)
    else:
-        return translate_call(builder, expr, callee)
+        return get_specialization(builder, expr, callee) or \
+               translate_call(builder, expr, callee)


This now performs specialization for all CallExpr types other than SuperExpr which does not have a fullname attribute. I could use get_specialization(builder, expr, callee, fullname=False) for SuperExpr if that's the right thing to do.

chadrik · 2021-10-13T18:38:47Z

mypyc/irbuild/specialize.py

+def get_specialization(builder: 'IRBuilder', expr: CallExpr, callee: RefExpr,
+                       typ: Optional[RType] = None, fullname: bool = True) -> Optional[Value]:
+    # TODO: Allow special cases to have default args or named args. Currently they don't since
+    # they check that everything in arg_kinds is ARG_POS.


I wasn't sure if this TODO applied to specialization in general, or if it was specific to the context where it was called. I copied it here from translate_refexpr_call

mypyc/irbuild/specialize.py

extension vs non-extension, plus various types of dataclasses

JukkaL · 2021-10-19T15:19:39Z

Thanks for the PR!

Have you tried writing a micro-benchmark (or two) to see if this will improve performance?

I was wondering if I should skip the special treatment of attrs classes if auto_attribs is not enabled, but if the attrs class is not using annotations then the compiled module wouldn't work whether mypyc skipped it or not, so I'm not sure if it's worth the extra complexity and potential fragility of doing that check.

Hmm if the compiled module won't work without auto_attribs, what about generating a compile error from mypyc in this case? Also we could suggest using auto_attribibs in a note.

(Not a full review, I will get back to this later.)

chadrik · 2021-10-20T03:44:00Z

Have you tried writing a micro-benchmark (or two) to see if this will improve performance?

To add a benchmark do I just add a new test case to run-bench.test?

I was wondering if I should skip the special treatment of attrs classes if auto_attribs is not enabled, but if the attrs class is not using annotations then the compiled module wouldn't work whether mypyc skipped it or not, so I'm not sure if it's worth the extra complexity and potential fragility of doing that check.

Hmm if the compiled module won't work without auto_attribs, what about generating a compile error from mypyc in this case? Also we could suggest using auto_attribibs in a note.

Since I made that comment I added the new ClassBuilder structure which should make it much easier to do these various specializations. I'll add in proper handling of auto_attribs.

JukkaL · 2021-10-20T09:25:40Z

To add a benchmark do I just add a new test case to run-bench.test?

You can write a throwaway microbenchmark and post the code and results here. We have some dataclass benchmarks which may be useful as examples: https://github.com/mypyc/mypyc-benchmarks/blob/master/microbenchmarks/dataclasses.py

For more information about the benchmarks, see https://github.com/mypyc/mypyc-benchmarks. It would also be great if you can add some attrs microbenchmarks there, but that's optional.

chadrik · 2021-10-21T17:19:22Z

You can write a throwaway microbenchmark and post the code and results here. We have some dataclass benchmarks which may be useful as examples: https://github.com/mypyc/mypyc-benchmarks/blob/master/microbenchmarks/dataclasses.py

Ah, I see. I wasn't aware of that repo. Yeah, it should be trivial to add some new attrs benchmarks based on the existing dataclass code. I expect the the results to be similar.

chadrik · 2021-10-22T05:11:50Z

Here are the bench results. Note that the attrs results are from my local machine whereas the dataclass results are from the mypyc-benchmakrs report (because I'm lazy)

Benchmark	dataclass	attrs
method	18.10x	26.551x
attr_access	13.66x	22.586x
create	1.39x	1.458x
as_dict_key	0.98x	1.120x

I'll commit the benchmarks once this gets merged.

chadrik · 2021-11-05T04:02:20Z

I just went over this PR again and I think it is in pretty good shape and ready for review.

JukkaL

Thanks for doing some benchmarking! The results look great.

I did a first review pass and looks good overall, just some minor comments. I'll do a final pass next week (this is quite a big PR!).

mypyc/irbuild/util.py

mypyc/irbuild/classdef.py

JukkaL · 2021-11-05T17:42:04Z

mypyc/irbuild/specialize.py

+    # they check that everything in arg_kinds is ARG_POS.
+
+    if isinstance(callee, (MemberExpr, NameExpr)):
+        name: Optional[str] = callee.fullname if callee.fullname else callee.name


We should only use callee.name if it's a MemberExpr and typ is defined. I think that this logic is a bit too complicated and it would perhaps be better to have separate functions for get_function_specialization and get_method_specialization (only the latter would accept typ or receiver_type).

have a look and see if I did this right.

mypyc/irbuild/specialize.py

mypyc/test-data/run-attrs-non-auto.test

JukkaL · 2021-11-05T17:48:52Z

mypyc/irbuild/classdef.py

+
+
+class ClassBuilder:
+    """Create IR for a class definition."""


Thanks for refactoring these! This now looks cleaner.

Can you mention that this is effectively an abstract class in the docstring, even though there are no abstract members?

done. I also went ahead and marked the methods as abstract: even though they all return None a subclass would effectively be broken if it did not implement these (with the possible exception of finalize)

Microbenchmarks for this PR: python/mypy#11328

chadrik · 2021-11-10T18:58:33Z

Hi, just pinging on this one

97littleleaf11

I like the abstraction of the ClassBuilder, which makes the code structure clearer and is easy to maintain.

mypyc/test/test_run.py

mypyc/irbuild/classdef.py

mypyc/irbuild/specialize.py

97littleleaf11 · 2021-11-17T09:02:48Z

mypyc/irbuild/expression.py

@@ -209,7 +209,8 @@ def transform_call_expr(builder: IRBuilder, expr: CallExpr) -> Value:
        callee = callee.analyzed.expr  # Unwrap type application

    if isinstance(callee, MemberExpr):
-        return translate_method_call(builder, expr, callee)
+        return apply_method_specialization(builder, expr, callee) or \


I am not sure whether it's fine to adding a specialization here, since it's slightly different from the original semantics.

I'm pretty sure this change is required for the feature to work. What can we do to test that it's safe?

Then val = apply_method_specialization(builder, expr, callee, receiver_typ) inside translate_method_call might be redundant?

I confirmed that both need to be present in order to work. This second call is handling the case where receiver_typ is present.

mypyc/test/test_run.py

97littleleaf11

Thanks again for your PR! Only some small suggestions on tests.

97littleleaf11 · 2021-11-18T06:49:52Z

mypyc/irbuild/util.py

@@ -13,6 +13,7 @@
    'dataclasses.dataclass',
    'attr.s',
    'attr.attrs',
+    'attr.define',


Add tests for this

I ended up removing the attr.define support. I did some tests and something about the way the mypyc support for attrs and dataclasses works is tripping up attrs auto-detection of annotations. I'm wary of delaying this PR further because I'm afraid I'll run out of steam, and we can always add attr.define support later.

97littleleaf11 · 2021-11-18T06:51:46Z

mypyc/test/test_run.py

@@ -54,15 +54,11 @@
    'run-dunders.test',
    'run-singledispatch.test',
    'run-attrs-non-auto.test',
+    'run-attrs.test',


Maybe we could just use one test file for these cases?

chadrik · 2021-11-25T04:43:03Z

Ok, I think this is all ready to go!

chadrik · 2021-11-26T17:18:03Z

Thanks!

…hon#11328) Create a class structure for building different types of classes, extension vs non-extension, plus various types of dataclasses. Specilize member expressions. Prior to this change dataclasses.field would only be specialized if called without the module, as field() Add support for attrs classes and build proper __annotations__ for dataclasses. Co-authored-by: Jingchen Ye <97littleleaf11@gmail.com>

chadrik added 2 commits October 13, 2021 08:58

mypyc: member expressions should be specialized

87763b2

prior to this change dataclasses.field would only be specialized if called without the module, as field()

mypyc: add support for attrs classes

b359136

chadrik commented Oct 13, 2021

View reviewed changes

fix tests on python 3.6

b254fe8

chadrik commented Oct 13, 2021

View reviewed changes

mypyc/irbuild/specialize.py Outdated Show resolved Hide resolved

chadrik added 3 commits October 13, 2021 19:43

fixes

c5f3ab2

mypyc: build proper __annotations__ for dataclasses

06745b5

mypyc: Create a class structure for building different types of classes

5d511bf

extension vs non-extension, plus various types of dataclasses

TH3CHARLie requested review from msullivan and JukkaL and removed request for msullivan October 18, 2021 07:58

minor improvements

0a29c2d

chadrik mentioned this pull request Nov 5, 2021

Add micro-benchmark for attrs classes mypyc/mypyc-benchmarks#33

Merged

JukkaL reviewed Nov 5, 2021

View reviewed changes

chadrik added 2 commits November 5, 2021 23:08

Address review notes

a26d9a5

Improve tests

4600dde

JukkaL pushed a commit to mypyc/mypyc-benchmarks that referenced this pull request Nov 8, 2021

Add micro-benchmark for attrs classes (#33)

d254df9

Microbenchmarks for this PR: python/mypy#11328

97littleleaf11 self-requested a review November 10, 2021 20:02

97littleleaf11 approved these changes Nov 17, 2021

View reviewed changes

97littleleaf11 changed the title ~~mypyc: Add support for attrs classes~~ [mypyc] Introduce ClassBuilder and add support for attrs classes Nov 17, 2021

Address review notes

cd914bf

97littleleaf11 approved these changes Nov 18, 2021

View reviewed changes

Address review notes. Remove attr.define

ec896ad

Refactor and add tests

6538411

97littleleaf11 merged commit 1bcfc04 into python:master Nov 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mypyc] Introduce ClassBuilder and add support for attrs classes #11328

[mypyc] Introduce ClassBuilder and add support for attrs classes #11328

chadrik commented Oct 13, 2021

chadrik Oct 13, 2021

chadrik Oct 13, 2021

chadrik Oct 13, 2021

JukkaL commented Oct 19, 2021

chadrik commented Oct 20, 2021

JukkaL commented Oct 20, 2021

chadrik commented Oct 21, 2021

chadrik commented Oct 22, 2021

chadrik commented Nov 5, 2021

JukkaL left a comment

JukkaL Nov 5, 2021

chadrik Nov 6, 2021

JukkaL Nov 5, 2021

chadrik Nov 6, 2021

chadrik commented Nov 10, 2021

97littleleaf11 left a comment

97littleleaf11 Nov 17, 2021

chadrik Nov 18, 2021

97littleleaf11 Nov 18, 2021

chadrik Nov 25, 2021

97littleleaf11 left a comment

97littleleaf11 Nov 18, 2021

chadrik Nov 25, 2021

97littleleaf11 Nov 18, 2021

chadrik Nov 25, 2021

chadrik commented Nov 25, 2021

chadrik commented Nov 26, 2021

[mypyc] Introduce ClassBuilder and add support for attrs classes #11328

[mypyc] Introduce ClassBuilder and add support for attrs classes #11328

Conversation

chadrik commented Oct 13, 2021

Description

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL commented Oct 19, 2021

chadrik commented Oct 20, 2021

JukkaL commented Oct 20, 2021

chadrik commented Oct 21, 2021

chadrik commented Oct 22, 2021

chadrik commented Nov 5, 2021

JukkaL left a 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

chadrik commented Nov 10, 2021

97littleleaf11 left a 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

97littleleaf11 left a 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

chadrik commented Nov 25, 2021

chadrik commented Nov 26, 2021