Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#18330 closed defect (fixed)

Metaclasses for Cython

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.7
Component: cython Keywords:
Cc: nbruin Merged in:
Authors: Jeroen Demeyer Reviewers: Nils Bruin
Report Upstream: N/A Work issues:
Branch: 70908cb (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

We implement a mechanism for metaclasses in Cython. The interface is different from Python (mostly because we have to work within the limits of Cython). Once a metaclass is defined, it works both from Cython and Python.

Once Sage has figured out how to use this properly, we can propose a wishlist item to upstream Cython.

Change History (56)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/cython_pytype_ready_hook

comment:4 Changed 5 years ago by jdemeyer

  • Cc nbruin added
  • Commit set to 60a27e72a03b5e0b808bce04ecf10f2aa34f01ef
  • Status changed from new to needs_review

New commits:

60a27e7Add hook for PyType_Ready

comment:5 follow-up: Changed 5 years ago by nbruin

If we're going to provide a hook to modify type structures around PyType_Ready we should *FIRST* provide a hook that modifies the type structure just *BEFORE* PyType_Ready runs on it. The beef we have presently is with the type structure that cython produces to offer to PyType_Ready, not with the CPython CAPI mandated protocol:

The tp_traverse and tp_visit routines that cython cooks up are insufficient in these situations, and similarly with tp_hash and tp_compare and tp_richcompare: we can just fill in those fields (all of them!) prior to PyType_Ready in order to get exactly the result we want.

The way that the CPython CAPI suggests that you create a type is that you *first* create the PyType structure and *then* offer it to PyType_Ready for completion, consistency checking, and registration in the system. I don't think that they specifically forbid modifying the structure but they certainly not define the effects it might have (and since they don't specify everything that PyType_Ready does, it could change in the future, so those effects might change).

So if we're going to add a protocol to cython it must be to inject custom code *between* cython producing the first draft of the PyType structure and it being handed to PyType_Ready. We already have a way of running code after that: just put it immediately below the class definition, so I'm pretty sure an extra protocol for adding code after will never be accepted (even independent of the fact it would have undefined behaviour by the CPython CAPI).

comment:6 follow-up: Changed 5 years ago by nbruin

Further comments: __typeinit__ should (eventually) be a special method, that automatically has its argument typed as "type". At that stage the method wouldn't need to make it into the tp_dict at all (and perhaps we should be removing it presently too? These methods usually do dangerous stuff, so it would be good if the user wouldn't be able to get their hands on the thing.

I see a possible hickup with the current design where _typeinit__ would be executed prior to PyType_Ready: The head of the (statically allocated) PyType? structure isn't initialized with the appropriate type (it would need &PyType_Type but that's not a compile-time constant, so isn't allowed in the static def):

  PyVarObject_HEAD_INIT(0, 0)

This is one of the things that PyType_Ready fixes. If we're going to pass that structure around to __typeinit__ we might be in for nasty surprises. So perhaps the lined code should also set the type field of the structure prior to passing it on (simple experimentation will show if that's necessary). Otherwise, I think there shouldn't be an issue with running __typeinit__ prior to PyType_Ready.

There is a further one concerning inheritance: I don't think we want to inherit __typeinit__, so we need to be careful in how we look up the method. Perhaps we should look up the method in t.tp_dict (or probably t.tp_methods at this point, since I think PyType_Ready constructs the t.tp_dict entries from that). This would only be a temporary thing: in cython proper one would handle this by having detected the special method.

comment:7 in reply to: ↑ 6 Changed 5 years ago by jdemeyer

Replying to nbruin:

I don't think we want to inherit __typeinit__

I absolutely do think we should inherit __typeinit__. The most powerful uses are exactly those which involve inheritance. If you only want this for one type, you can easily do

def __typeinit__(cls):
    if cls is not MySpecialClass:
        return

or just use the current hacks like in coerce_dict.pyx

comment:8 in reply to: ↑ 5 Changed 5 years ago by jdemeyer

Replying to nbruin:

If we're going to provide a hook to modify type structures around PyType_Ready we should *FIRST* provide a hook that modifies the type structure just *BEFORE* PyType_Ready runs on it.

Before PyType_Ready(), there is not much that will work on a type. Even the lookup of the method __typeinit__ will very likely fail. Honestly, I don't see what is wrong with modifying a type after PyType_Ready and I actually think it is much safer after than before.

We already have a way of running code after that: just put it immediately below the class definition

Not really. There is a lot of stuff that happens between PyType_Ready and the execution of the code immediately below the class definition. In particular: all types are created first, and then the other code is executed. When inheritance is involved, you really want to create type A, patch type A, create type B (subtype of A), patch type B, ...

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:9 follow-up: Changed 5 years ago by jdemeyer

Remark that even Cython changes tp_print after calling PyType_Ready:

  if (PyType_Ready(&__pyx_type_4sage_9structure_11coerce_dict_MonoDict) < 0) {...}
  __pyx_type_4sage_9structure_11coerce_dict_MonoDict.tp_print = 0;

comment:10 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by nbruin

Replying to jdemeyer:

Remark that even Cython changes tp_print after calling PyType_Ready:

Well, that's a hack too: https://github.com/cython/cython/commit/31890343f048e58ebe7986cf56d6fbdeaf7c16b7 which was introduced after our work on WeakValueDict: https://groups.google.com/forum/#!msg/cython-users/ktsXt_o4M0Q/OwmQ_33iXLgJ

If you want to inherit __typeinit__ then you'd pretty much have to run it after PyType_Ready, because it will be virtually impossible to find where to inherit it from otherwise.

My main reason to want to avoid hacking after PyType_Ready has done its thing is to avoid making assumptions about what it does. You should probably at least run PyType_ClearCache after you're done. It looks like in Python3 there's now PyType_Modified, which would be a more targeted way of notifying Python of the fact that a type structure has been changed (the fact that they offer a routine to do so actually makes me a little more comfortable changing PyType structures later on).

I think it's worth running the pros and cons of where to run __typeinit__, and whether inheriting it will be safe enough (it smells like a bug trap to me), by the cython people. I'd imagine Stefan and Robert might have some smart things to say about it.

edit: One of the problems with "inheriting" __typeinit__ is that you can't reliably do so. It would only work when a cython class inherits it (perhaps even only cdef class?). However, subclassing using python would not give the opportunity for __typeinit__ to execute. Unreliable inheritance is, I think, worse than none.

I guess your interesting application of using inheritance is to circumvent python's tendency of not inheriting tp_hash, tp_richcompare, tp_compare in various situations (you were planning to put a default back if one of them is NULL?) That's precarious to get "right" and with unreliable inheritance basically impossible. I think the python API designers made a very conscious choice that we should probably not try to circumvent in general and only in special cases (which a non-inheriting __typeinit__ would accomplish).

Last edited 5 years ago by nbruin (previous) (diff)

comment:11 in reply to: ↑ 10 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

One of the problems with "inheriting" __typeinit__ is that you can't reliably do so. It would only work when a cython class inherits it (perhaps even only cdef class?).

There is really no such thing as a "Cython class", a class implemented in Cython is just a Python class. This mechanism indeed only works for extension types (a.k.a. cdef classes).

I don't consider this a major issue though. For a Python class, one could probably use __metaclass__ to accomplish analogous things. And besides, comparison is already implemented differently in extension types and Python classes.

I guess your interesting application of using inheritance is to circumvent python's tendency of not inheriting tp_hash, tp_richcompare, tp_compare in various situations

And also the tp_new hackery in Integer and RealDoubleElement.

I think the python API designers made a very conscious choice that we should probably not try to circumvent in general

I disagree with this. I'm certain that the Python API designers didn't consider the Sage coercion model when making this choice.

comment:12 in reply to: ↑ 11 ; follow-ups: Changed 5 years ago by nbruin

Replying to jdemeyer:

There is really no such thing as a "Cython class", a class implemented in Cython is just a Python class. This mechanism indeed only works for extension types (a.k.a. cdef classes).

Yes, that's mainly my issue: Cython merely provides a way of implementing python classes and python extension classes, so all that Cython can specify are protocols that are compatible with this CPython-CAPI specified standard. An inherited __typeinit__ doesn't fit with that: it would only be inherited "along cdef classes". For that reason I would expect that Cython will not accept a proposed inherited __typeinit__, whereas I think we do have a chance of having a non-inherited one accepted, and I think sage can live with a non-inherited one.

And also the tp_new hackery in Integer and RealDoubleElement.

In which case the extra work would consist of *not* inheriting the tp_new function because the trick used there does not conform to the Python CAPI. But that doesn't need to be inherited. That just needs to be done once. If we want Integer to be subclassable we could also subclass (perhaps as UserInteger to honour a python tradition) *once* with a __typeinit__ that undoes the tp_new hackery of Integer and offer that type for subclassing and make Integer final (I think there are tricks to make an extension subclass of a final extension class), or otherwise just document it as not subclassable.

I'm certain that the Python API designers didn't consider the Sage coercion model when making this choice.

I'm sure of it, given that the implications for equality directly violate the python rules. I also think that it doesn't become easier to write correct code if the sage library will reinstate arbitrary equality and hashing functions, for which it is vitally important to design and test them together. Having a hook that enables you to transplant them (rather than wrap, with efficiency penalty) on a one-by-one basis seems a very good solution, however.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

I also think that it doesn't become easier to write correct code if the sage library will reinstate arbitrary equality and hashing functions

Not arbitrary functions, just functions using the coercion model. I want __richcmp__ to behave just like __add__: use the coercion model to call the real implementation in _richcmp_ or _add_. The only obstruction for this is Python's strange inheritance rules for comparisons.

In the coercion model, the fact that MyWhateverElement.__add__ is just Element.__add__ works well. I think that __richcmp__ can work the same way.

More on the subject of inheritance: when you define __typeinit__ to inherit, you can easily implement it as non-inheriting by a if cls is MySpecialType clause. So the "inheritance" proposal is more general.

comment:14 in reply to: ↑ 12 Changed 5 years ago by jdemeyer

Replying to nbruin:

If we want Integer to be subclassable

Integer is already not subclassable because of the tp_new hacks. But that's not really an issue, that could easily be fixed (independently of how you change tp_new).

comment:15 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by nbruin

Replying to jdemeyer:

Not arbitrary functions, just functions using the coercion model. I want __richcmp__ to behave just like __add__: use the coercion model to call the real implementation in _richcmp_ or _add_. The only obstruction for this is Python's strange inheritance rules for comparisons.

Ah, OK. So the idea is that __richcmp__ will always do the coercion stuff prior to deferring to the element-specific _richcmp_, so that routine should be safe to inherit. But in order to inherit that (automatically), python requires that __hash__ is inherited as well. The symmetric solution is to let __hash__ defer to _hash_ but that's just silly because there's no coercion to be done. So we want tp_hash to point straight to (what would be) _hash_.

I agree that for your particular scenario an inheriting __typeinit__ would be more natural, but it would break as soon as someone wants to do this with a python subclass. So I'm still not in favor of the idea.

It is possible to do that with a non-inheriting __typeinit__: write a cdef _hash_ of the appropriate signature and install that in the __typeinit__, after PyType_Ready has had its way. It would require a a boiler plate __typeinit__ on any class that wants to override __hash__, but only those.

I'm also pretty sure that the inheritance breakage will be a no-go for upstream cython adoption.

More on the subject of inheritance: when you define __typeinit__ to inherit, you can easily implement it as non-inheriting by a if cls is MySpecialType clause. So the "inheritance" proposal is more general.

Yes, I understand that. The main problem is that the inheritance is broken because we don't have the required hook in python available. You'd end up with classes that have a __typeinit__ on them that may not have executed.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by jdemeyer

So essentially you are saying that we should not fix those 104 Cython Element classes implementing comparison, simply because we don't also fix the Python Element classes?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by nbruin

Replying to jdemeyer:

So essentially you are saying that we should not fix those 104 Cython Element classes implementing comparison, simply because we don't also fix the Python Element classes?

No, I am not saying that, not literally and not essentially. The fact that __typeinit__ would be inherited but be ineffectual sometimes makes it a hack. We should try and avoid hacks if we reasonably can, because they are a maintenance burden.I am not convinced yet that we can't avoid using a hack here.

I read the ticket description as a proposal to work out way of dealing with class customization in a way that can eventually be proposed upstream to cython. I don't think cython will be interested in accepting a hack.

If we have to resort to a hack for the __richcmp__ problem (which we very well might have to), I think a cleaner solution would be to just have something (probably in sage.structure.element) along the lines of:

def fix_richcmp(T):
    (<PyTypeObject*><type?>T).tp_richcompare=Element.tp_richcompare
    PyType_Modified(T)

and simply document that if you override __hash__ on an ElementType, you should be calling fix_richcmp on that type right after definition. I think we might be able to let that apply in both python and cython uniformly.

Plus: we get something that works uniformly across cython and python Minus: it requires 1 line of boilerplate for every time that __hash__ gets redefined. Neutral: in either case, the end result is the same Plus: the fix_richcmp thing is also a hack, but it announces itself by being present in all locations where it is used, rather than a silent __typeinit__ that is defined in an entirely different location and may or may not take effect.

further investigation: Is it a problem that in cython the call to PyType_Ready and the call to fix_richcmp get separated a little? I don't think it is, but I might be overlooking something.

comment:18 in reply to: ↑ 17 Changed 5 years ago by jdemeyer

Replying to nbruin:

Minus: it requires 1 line of boilerplate for every time that __hash__ gets redefined.

For me, this is a big minus. I want a solution which does not involve 104 times the same boilerplate.

comment:19 Changed 5 years ago by jdemeyer

I'm trying to see if I can use a __metaclass__-like mechanism for the Python case.

comment:20 Changed 5 years ago by jdemeyer

OK, I think I will go for something as close as possible to __metaclass__. This will actually change the __class__ of a type, so it will work from Python also.

comment:21 Changed 5 years ago by git

  • Commit changed from 60a27e72a03b5e0b808bce04ecf10f2aa34f01ef to 30fe4628ca4dfbdfaebe77845f13e1b3c3121dd6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

30fe462Cython metaclass support

comment:22 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Cython PyType_Ready hook to Metaclasses for Cython

comment:23 Changed 5 years ago by git

  • Commit changed from 30fe4628ca4dfbdfaebe77845f13e1b3c3121dd6 to d4ba8727eecb84936607677ae4fa984fa8a0696c

Branch pushed to git repo; I updated commit sha1. New commits:

d4ba872Add check for tp_basicsize

comment:24 follow-ups: Changed 5 years ago by nbruin

Great! I think something like this might make it into cython, at which point they can probably support __metaclass__=.... It's not immediately clear to me what goes wrong presently if you just do this via setting an attribute. Is it that class attributes aren't filled in yet when PyType_Ready runs?

You've convinced me about the main use case, the fact that our Element.tp_richcompare is completely generic and dispatches to other methods anyway, and thus doesn't particularly contribute to danger when inherited independently of tp_hash, and for that a metaclass seems like the right solution. Perhaps RichCompareInheritingMetaClass or so.

However, I'm not convinced that the metaclass mechanism is the appropriate one for you POC here: for setting tp_clear and tp_traverse. Here it's not the behaviour of the type or inheritance that we need to change, but it's simply a need to supply custom routines in PyTypeObject slots, (ideally prior to PyType_Ready, but that's relatively difficult to arrange) in a way that cython doesn't support. Once those slots are filled in, there is no issue with inheritance, so I think it's a little overkill to change the metaclass for that. It also generates an amount of boilerplate that is actually larger than the straightforward attribute reassignment we had before (which should get a PyType_Modified added to it--although slots probably don't make it into the cache that gets cleared anyway).

The cases I have seen here can either be solved with get_metaclass directly (changing inheritance behaviour) or should not use this mechanism at all (fixing slots with custom routines). So I do not see the need for the extra __typeinit__ protocol at this point. Do you see a convincing use? (I can see why for the tp_clear/tp_traverse case you want to store this information on the class and not the metaclass, but I don't see that as a metaclass usecase anyway).

comment:25 in reply to: ↑ 24 Changed 5 years ago by jdemeyer

Replying to nbruin:

Is it that class attributes aren't filled in yet when PyType_Ready runs?

Yes, that's the case (I don't know why it's done like that in Cython, but that's the way it is).

comment:26 in reply to: ↑ 24 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

However, I'm not convinced that the metaclass mechanism is the appropriate one for you POC here

If you agree with the rest of the ticket, I can remove that.

It's just that whenever I propose a new mechanism, I always like to add a simple example of using it. This is to show how to use it and to test that it actually works.

I see the following potential other use cases, neither of which is simple enough to implement it as part of this ticket:

  1. Some version of ClassCallMetaclass which can be used in Cython
  2. Patching tp_new of Integer in a cleaner way (which is more involved than the coerce_dict examples)
  3. Inheriting comparisons.

comment:27 in reply to: ↑ 26 Changed 5 years ago by nbruin

Replying to jdemeyer:

Replying to nbruin:

However, I'm not convinced that the metaclass mechanism is the appropriate one for you POC here

If you agree with the rest of the ticket, I can remove that.

Yes, as far as I can see, your proposal comes as close as possible to offering metaclass support to cython, and I think it's reasonable to have that.

It's just that whenever I propose a new mechanism, I always like to add a simple example of using it. This is to show how to use it and to test that it actually works.

Yes, it's just seems the case that by the time you hit metaclasses, any simple example is an inappropriate use. It definitely served a purpose in getting the required features figured out.

  1. Patching tp_new of Integer in a cleaner way (which is more involved than the coerce_dict examples)

I took a quick look at that allocator.pyx for that. I'm not quite convinced that metaclasses provide a substantial improvement to functionality and/or transparency in the code there (because ultimately, it's just a patching of a tp_* slot there as well, just with a more complicated/hacky routine)

comment:28 Changed 5 years ago by git

  • Commit changed from d4ba8727eecb84936607677ae4fa984fa8a0696c to 3ec2842529f4aa7763b2db0a7c6556c5bb437c14

Branch pushed to git repo; I updated commit sha1. New commits:

3ec2842Revert changes to coerce_dict

comment:29 follow-up: Changed 5 years ago by nbruin

OK, we can either leave this ticket for now, to see if TypeInitMetaclass will find a genuine application or remove it and give this ticket a positive review. We shouldn't introduce support for interfaces for which we have no proven need yet.

(the metaclasses themselves are obviously going to be useful for the richcompare inheritance)

comment:30 in reply to: ↑ 29 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

(the metaclasses themselves are obviously going to be useful for the richcompare inheritance)

I was thinking of implementing that using __typeinit__, but I guess you consider that bad design?

comment:31 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:32 Changed 5 years ago by git

  • Commit changed from 3ec2842529f4aa7763b2db0a7c6556c5bb437c14 to 6e6870bf2a50b659d6988511c1c9c031f4cb93eb

Branch pushed to git repo; I updated commit sha1. New commits:

6e6870bAdd some needed casts

comment:33 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by nbruin

Replying to jdemeyer:

Replying to nbruin:

(the metaclasses themselves are obviously going to be useful for the richcompare inheritance)

I was thinking of implementing that using __typeinit__, but I guess you consider that bad design?

How I understand the issue there, I think presently that putting everything in the metaclass.init rather than delegating to a __typeinit__ would be cleaner.

I can see potential uses for __typeinit__ if we're in a situation where things that the metaclass itself doesn't have direct access to need to be installed (so basically when we find that we have to write multiple metaclasses to do similar things on multiple classes, only because we don't have a way of passing the required info to metaclass.__init__. The process in allocator.pyx would be an example, if we were to find that metaclasses are the right tool for that.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

Replying to jdemeyer:

Replying to nbruin:

(the metaclasses themselves are obviously going to be useful for the richcompare inheritance)

I was thinking of implementing that using __typeinit__, but I guess you consider that bad design?

How I understand the issue there, I think presently that putting everything in the metaclass.init rather than delegating to a __typeinit__ would be cleaner.

OK, fine for me. What do you want to be done concretely to give this ticket positive_review?

comment:35 follow-up: Changed 5 years ago by jdemeyer

Looking at #18329, I see a non-trivial problem appearing: metaclasses are a pain when (multiple) inheritance is involved:

class MA(type):
    pass

class MB(type):
    pass

class A(object):
    __metaclass__ = MA

class B(object):
    __metaclass__ = MB

class C(A, B):
    pass

gives

TypeError: Error when calling the metaclass bases
    metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

In this case, you need to manually define a new metaclass inheriting from MA and MB.

This problem will appear in many places where ClasscallMetaclass is used on subclasses of Element.

So, either we fix this manually in all cases (there are many in combinat) or we somehow define a metametaclass to deal with this problem (it's type.__new__() which raises this exception).

comment:36 in reply to: ↑ 34 ; follow-up: Changed 5 years ago by nbruin

Replying to jdemeyer:

OK, fine for me. What do you want to be done concretely to give this ticket positive_review?

Now: remove TypeInitMetaclass. However, I think this ticket only needs to be merged tegether with its first actual use. Should little bugs become apparent, it would be a little easier to fix those too (unless you have a definite preference for having it in the main tree already).

comment:37 in reply to: ↑ 35 ; follow-ups: Changed 5 years ago by nbruin

Replying to jdemeyer:

In this case, you need to manually define a new metaclass inheriting from MA and MB.

This problem will appear in many places where ClasscallMetaclass is used on subclasses of Element.

Ouch. That means only one explicit metaclass, right? the one that inherits from ClasscallMetaclass and RichcompareFixerMetaclass. The real penalty is in the explicit specification of the metaclass on many classes.

comment:38 in reply to: ↑ 36 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

unless you have a definite preference for having it in the main tree already

I do because it means that I don't need to worry about merge conflicts.

comment:39 in reply to: ↑ 37 Changed 5 years ago by jdemeyer

Replying to nbruin:

The real penalty is in the explicit specification of the metaclass on many classes.

It's not that bad but it's annoying indeed.

comment:40 Changed 5 years ago by git

  • Commit changed from 6e6870bf2a50b659d6988511c1c9c031f4cb93eb to bc4d9573462bba9cf9e37992fc519443ca3fa9fd

Branch pushed to git repo; I updated commit sha1. New commits:

bc4d957Remove TypeInitMetaclass, add some documentation

comment:41 Changed 5 years ago by jdemeyer

For a possible native Cython implementation, it would be really cool if we could change this:

static PyTypeObject __pyx_type_4sage_9structure_11coerce_dict_MonoDict = {
       ^^^^^^^^^^^^
  PyVarObject_HEAD_INIT(0, 0)
  "sage.structure.coerce_dict.MonoDict", /*tp_name*/
  sizeof(struct __pyx_obj_4sage_9structure_11coerce_dict_MonoDict), /*tp_basicsize*/
  0, /*tp_itemsize*/
  __pyx_tp_dealloc_4sage_9structure_11coerce_dict_MonoDict, /*tp_dealloc*/

comment:42 in reply to: ↑ 38 ; follow-up: Changed 5 years ago by nbruin

Replying to jdemeyer:

Replying to nbruin:

unless you have a definite preference for having it in the main tree already

I do because it means that I don't need to worry about merge conflicts.

(this patch is just adding files so there's little chance for conflicts)

Slight oversight: still a reference to __typeinit__ surviving:

+/*
+ * This function calls PyType_Ready(t) and then calls t.__typeinit__(t)
+ * as if that was a class method. The __typeinit__ method can then be
+ * used for example to make changes to the tp_foo slots.
+ */

comment:43 in reply to: ↑ 42 Changed 5 years ago by jdemeyer

Replying to nbruin:

this patch is just adding files so there's little chance for conflicts

It's mostly that, but not only.

comment:44 Changed 5 years ago by git

  • Commit changed from bc4d9573462bba9cf9e37992fc519443ca3fa9fd to 70908cbbaa5b4ec6c6f5dc7332d0f7c1379b511e

Branch pushed to git repo; I updated commit sha1. New commits:

70908cbComment

comment:45 Changed 5 years ago by nbruin

  • Reviewers set to Nils Bruin
  • Status changed from needs_review to positive_review

comment:46 in reply to: ↑ 37 Changed 5 years ago by jdemeyer

Replying to nbruin:

The real penalty is in the explicit specification of the __metaclass__ on many classes.

I thought I had a solution for this problem and I worked half a day today to implement it. It works fine in Python 2 (and Python < 3.2.3) but not in later Python versions and not in Cython 0.22.

This is the subtle difference in metaclasses: consider the statement

class C(A, B):
    pass

In Python < 3.2.3, the metaclass of the first base is used, i.e. the statement is essentially parsed as

C = type(A)("C", (A,B), {})

This means that type(A).__new__ could avoid a metaclass conflict.

But in recent Python versions, the metaclass is computed first and

C = metaclass("C", (A,B), {})

is called, where metaclass is the result of calling _PyType_CalculateMetaclass(), which also checks for conflicts. This means that a metaclass now has absolutely no chance to avoid a conflict.

comment:47 Changed 5 years ago by jdemeyer

I think the best solution is to define one metaclass which can do everything, depending on certain attributes on the class.

For example, the metaclass could check for a __fix_nested_class_pickling__ method and, if that method returns true, do whatever NestedClassMetaclass does. It could check for __classcall__ and related methods, it could check for a __inherit_richcmp__ method...

comment:48 follow-up: Changed 5 years ago by nbruin

I don't understand what the problem is. This works for me in Python 3.3.2:

class MA(type):
    def __init__(self,*args):
      print("calling MA on ",self," with ",args)

class MB(type):
    def __init__(self,*args):
      print("calling MB on ",self," with ",args)

class MC(MA,MB):
    def __init__(self,*args):
      print("calling MC on ",self," with ",args)

class A(object, metaclass=MA):
    pass

class B(object, metaclass=MB):
    pass

class C(A,B,metaclass=MC):
    pass

Indeed, we need to specify a metaclass for C explicitly, but we need to do that in 2.7 too. Can you be a little more specific what the actual problem is?

comment:49 in reply to: ↑ 48 ; follow-up: Changed 5 years ago by jdemeyer

Replying to nbruin:

Indeed, we need to specify a metaclass for C explicitly, but we need to do that in 2.7 too.

Using the C API, I can arrange things such that we don't need to specify C explicitly. My solution constructs a new metaclass dynamically if there is a conflict.

Using the notation of the example above, in Python <3.2.3, it's type(A).tp_new which is responsible for setting up the correct metaclass, and this can be customized. In Python >=3.2.3, the metaclass is computed by the __build_class__ builtin function, before any tp_new can get its hands on the class.

comment:50 Changed 5 years ago by jdemeyer

And like I said before, Cython always constructs classes in the Python-3.2.3 way.

comment:51 in reply to: ↑ 49 ; follow-ups: Changed 5 years ago by nbruin

Replying to jdemeyer:

Using the notation of the example above, in Python <3.2.3, it's type(A).tp_new which is responsible for setting up the correct metaclass, and this can be customized. In Python >=3.2.3, the metaclass is computed by the __build_class__ builtin function, before any tp_new can get its hands on the class.

Yes, and python3 has a good reason for doing that: there's also the __prepare__ hook (which is not useful for cython) and the choice made there is to not pick the __prepare__ that comes first in the mro (which previously was apparently the case for __new__) but to raise an error in case of ambiguity instead. The python language designers might have erred on the side of caution there, but that's the choice they made and that we have to live with.

You get a very clear error as soon as you construct a class that has ambiguous metaclasses. It's very clear what to do: create your own metaclass where you resolve the mro via inheritance on the metaclasses, rather than let the system deduce an mro based on the suggested mro of the base classes and their metaclasses. Given the intricacies of new and init you probably have to manually compose these things anyway (because python would just pick one of them)

In my opinion, the penalty in the form of some extra __metaclass__=... lines (100 of them in total according to your count I think?) is better than inventing some "universal" metaclass that grows all kinds of special methods to indicate which metaclass features should be activated.

comment:52 in reply to: ↑ 51 Changed 5 years ago by jdemeyer

Replying to nbruin:

In my opinion, the penalty in the form of some extra __metaclass__=... lines (100 of them in total according to your count I think?) is better than inventing some "universal" metaclass that grows all kinds of special methods to indicate which metaclass features should be activated.

I think it all depends on how many of these metaclasses will need to be combined. With n base metaclasses, you can have 2^n combinations. For now, I agree it will certainly be the simplest solution. However, if in the future we find 5 more different use-cases for metaclasses, we might need to reconsider...

comment:53 in reply to: ↑ 51 Changed 5 years ago by jdemeyer

Replying to nbruin:

In my opinion, the penalty in the form of some extra __metaclass__=... lines (100 of them in total according to your count I think?) is better than inventing some "universal" metaclass

Actually, what do you have against this "universal metaclass"? What is your opinion on the current ClasscallMetaclass? I ask because ClassCallMetaclass is already a simple version of such a universal metaclass (it checks for 4 special methods of a class and then does something if those methods exist).

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:54 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/cython_pytype_ready_hook to 70908cbbaa5b4ec6c6f5dc7332d0f7c1379b511e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:55 Changed 5 years ago by jdemeyer

  • Commit 70908cbbaa5b4ec6c6f5dc7332d0f7c1379b511e deleted

Relevant comment from src/sage/structure/dynamic_class.py:

    # The metaclass of a class must derive from the metaclasses of its
    # bases. The following handles the case where one of the base
    # class is readilly in the ClasscallMetaclass. This
    # approach won't scale well if we start using metaclasses
    # elsewhere in Sage.

comment:56 Changed 4 years ago by jpflori

Any plan to use this for the hooked tp_new? Would it allow to use __new__ for Integer?

Note: See TracTickets for help on using tickets.