Opened 9 years ago

Closed 6 years ago

#16074 closed enhancement (fixed)

Metaclass syntax changed in Python 3

Reported by: Wilfried Luebbe Owned by:
Priority: major Milestone: sage-8.0
Component: python3 Keywords: python3
Cc: Travis Scrimshaw, Jeroen Demeyer Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: f1ca05f (Commits, GitHub, GitLab) Commit: f1ca05f39db7d8ca8afc379808858c568619b9eb
Dependencies: Stopgaps:

Status badges

Description (last modified by Frédéric Chapoton)

The tool 2to3 changes the code to the new Py3 syntax.
But the code has to depend on the Python version!

There are 30 affected modules.

This ticket is tracked as a dependency of meta-ticket ticket:16052.

REFERENCE: http://stackoverflow.com/questions/18513821/python-metaclass-understanding-the-with-metaclass

Change History (28)

comment:1 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:2 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:3 Changed 7 years ago by Jeroen Demeyer

Component: distributionpython3
Description: modified (diff)
Summary: Python 3 preparation: The metaclass syntax has changed in PyMetaclass syntax changed in Python 3

comment:4 Changed 6 years ago by Frédéric Chapoton

Description: modified (diff)
Milestone: sage-6.4sage-7.6

comment:5 Changed 6 years ago by Frédéric Chapoton

first half in #22474 (combinat folder)

comment:6 Changed 6 years ago by Frédéric Chapoton

another easy step in #22479

then there remains more difficult cases

comment:7 Changed 6 years ago by Jeroen Demeyer

Cython files: #22537

comment:8 Changed 6 years ago by Frédéric Chapoton

Cc: Travis Scrimshaw Jeroen Demeyer added

@tscrim: I have not been able to fix the use of metaclass in

* src/sage/algebras/clifford_algebra.py
* src/sage/algebras/commutative_dga.py
* src/sage/modular/modform_hecketriangle/graded_ring_element.py

The expected python3 replacement is

from six import add_metaclass

@add_metaclass(name_of_metaclass)
class something

But this does not work for the three mentioned files.

Last edited 6 years ago by Frédéric Chapoton (previous) (diff)

comment:9 Changed 6 years ago by Jeroen Demeyer

For reference, can you at least push the non-working branch?

comment:10 Changed 6 years ago by Frédéric Chapoton

Branch: public/16074
Commit: c9bb275589209c5da4b16c475c076f12a2557816
Milestone: sage-7.6sage-8.0

here it is


New commits:

c9bb275metaclass tentative for clifford algebras

comment:11 Changed 6 years ago by Frédéric Chapoton

failing as follows

  File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/algebras/clifford_algebra.py", line 2163, in <module>
    class ExteriorAlgebraDifferential(ModuleMorphismByLinearity, UniqueRepresentation):
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

comment:12 Changed 6 years ago by Travis Scrimshaw

@add_metaclass probably does something with the metaclass. Unless it is because of our heirarchy of metaclass...but I doubt it is that. It might be a decorator that needs to be added to the subclasses as well.

comment:13 Changed 6 years ago by Jeroen Demeyer

The problem is the decorator, because it works in 2 steps:

  1. The class is created the usual way.
  1. The decorator modifies the class to have a new metaclass.

The problem is that step 1. already fails, so the decorator cannot do anything.

comment:14 Changed 6 years ago by Jeroen Demeyer

I'll look into this.

comment:15 in reply to:  8 Changed 6 years ago by Jeroen Demeyer

Replying to chapoton:

@tscrim: I have not been able to fix the use of metaclass in

* src/sage/algebras/clifford_algebra.py
* src/sage/algebras/commutative_dga.py
* src/sage/modular/modform_hecketriangle/graded_ring_element.py

There are some other files with __metaclass__, such as src/sage/structure/unique_representation.py. Should I try to fix these on this ticket too or is there a different ticket?

comment:16 Changed 6 years ago by Frédéric Chapoton

Please fix the metaclass issue in all places where you can. There is no other ticket. I only listed the algebraic use cases. The rest is more like "sage infrastructure".

comment:17 Changed 6 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer

Hmm, I agree that it's not easy :-) I'm making some progress, but I'm not quite there yet.

comment:18 Changed 6 years ago by git

Commit: c9bb275589209c5da4b16c475c076f12a255781636009e4e7fef4cf8136695279af1e8b17cc7cd5c

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

f61a1aeReplace __metaclass__ with six.with_metaclass
677cc66Check for metaclass by checking types instead of __metaclass__
36009e4six.with_metaclass: use type.__call__ instead of type.__new__

comment:19 Changed 6 years ago by git

Commit: 36009e4e7fef4cf8136695279af1e8b17cc7cd5c9635fb408451c3a6aca1d26deafed6461ac62f01

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

b2beb04six.with_metaclass: override __call__ instead of __new__
b2108e6Replace __metaclass__ with six.with_metaclass
9635fb4Check for metaclass by checking types instead of __metaclass__

comment:20 Changed 6 years ago by Jeroen Demeyer

Status: newneeds_review

comment:21 Changed 6 years ago by Jeroen Demeyer

Done. The main effort here is fixing sage.misc.six.with_metaclass (the first of 3 commits).

After this ticket, there is no remaining Sage code using __metaclass__. Some doctests still use __metaclass__, but I suggest to fix that later.

comment:22 Changed 6 years ago by Travis Scrimshaw

I don't agree with this change (and subsequent changes from this):

  • src/sage/misc/six.py

    diff --git a/src/sage/misc/six.py b/src/sage/misc/six.py
    index 8273d91..5f03a96 100644
    a b from __future__ import absolute_import 
    88from six import *
    99
    1010
    11 def with_metaclass(meta, *bases):
     11def with_metaclass(*args):
    1212    """
    1313    Create a base class with a metaclass.
    1414

as later we require the first argument to be the meta class. So if someone happened to pass nothing, it would get an error message starting much later in the code. IMO, it also obfuscates the code a little bit too.

comment:23 Changed 6 years ago by git

Commit: 9635fb408451c3a6aca1d26deafed6461ac62f01f1ca05f39db7d8ca8afc379808858c568619b9eb

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

f1ca05fRestore (meta, *bases) arguments for with_metaclass()

comment:24 in reply to:  22 Changed 6 years ago by Jeroen Demeyer

Replying to tscrim:

I don't agree with this change

Fixed.

comment:25 Changed 6 years ago by Jeroen Demeyer

I wanted to submit these fixes to with_metaclass upstream to six. However, when looking at the original six code, I understood what was going wrong and it is really simple.

I also realized that it was #18503 which is the cause for the metaclass breakage. In other words, #18503 did the wrong thing and actually made everything more complicated.

It turns out that #18503 can be fixed in a much simpler way, see https://github.com/benjaminp/six/pull/191 and then this issue (#16074) simply does not occur.

comment:26 Changed 6 years ago by Jeroen Demeyer

That being said, I think the current solution on this ticket is structurally better than upstream's with_metaclass: upstream is overriding __new__ with __call__ which is a bit fishy. Instead, I am overriding __call__ with __call__ which makes more sense.

comment:27 Changed 6 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Makes sense to me. It's somewhat less pretty than the decorator, but this works for now and gets us closer to Python3.

comment:28 Changed 6 years ago by Volker Braun

Branch: public/16074f1ca05f39db7d8ca8afc379808858c568619b9eb
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.