Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases #210

Closed
dghubble opened this issue Sep 18, 2017 · 28 comments

Comments

@dghubble
Copy link

dghubble commented Sep 18, 2017

Some time in the last 12 hours, our build system using gsutil started failing and it appears to be related to six 1.11.0, which was just released.

$ gsutil
...
 File "myproject/venv/lib/python2.7/site-packages/apitools/base/protorpclite/messages.py", line 1168, in <module>
    class Field(six.with_metaclass(_FieldMeta, object)):
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

gsutil declares a dependency on six>=1.9.0 (ugh) so it pulls in the just released 1.11.0. Rolling back to six 1.10.0 and gsutil runs alright. I'm not really involved in Python or gsutil, so maybe gsutil needs to fix something, no idea, but it seems like a breakage that's not called out in the release notes or maybe something to fix.

cc @jdemeyer @benjaminp
Possibly introduced in #191

@cpoppema
Copy link

Just stumbled onto this as well. Not using gsutil. Another package (django-extensions) has six>=1.2 as a dependency. For now, adding 1.10.0 to the requirements myself as a temporary workaround.

@jdemeyer
Copy link
Contributor

A minimal non-working example would be nice.

@cpoppema
Copy link

Looking at the 3rd party package code that triggers the error, it boils down to this:

import six


class Meta(type):
    pass


class A(six.with_metaclass(Meta)):
    __metaclass__ = Meta

Removing the __metaclass__ prevents the TypeError.

@jdemeyer
Copy link
Contributor

Removing the metaclass prevents the TypeError.

Obviously, that is the right fix. The whole point of using six.with_metaclass is that you do not need __metaclass__ (which is not supported in Python 3).

That being said, if this broke actual usage (even if that actual usage is arguably wrong), it makes sense to try to fix this.

@jdemeyer
Copy link
Contributor

jdemeyer commented Sep 18, 2017

IOError: [Errno 2] No such file or directory: '/opt/test/lib/python2.7/site-packages/six-1.10.0.dist-info/METADATA'

This is obviously unrelated to this issue, it seems more like an installation issue.

@cpoppema
Copy link

I'll create a pull request to remove the __metaclass__ from my example. Not sure if @dghubble can spot a similar pattern or if it's caused by something different.

@jdemeyer
Copy link
Contributor

I could probably manage to fix the

class A(six.with_metaclass(Meta)):
    __metaclass__ = Meta

example but it will lead to more complicated code for six.with_metaclass.

I would like some input from the six maintainers on how to proceed.

@florenthemmi
Copy link

florenthemmi commented Sep 18, 2017

I have the same issue with some Google libraries:

  File "/Users/florenthemmi/dev/env/lib/python2.7/site-packages/apache_beam/internal/gcp/json_value.py", line 23, in <module>
    from apitools.base.py import extra_types
  File "/Users/florenthemmi/dev/env/lib/python2.7/site-packages/apitools/base/py/__init__.py", line 21, in <module>
    from apitools.base.py.base_api import *
  File "/Users/florenthemmi/dev/env/lib/python2.7/site-packages/apitools/base/py/base_api.py", line 31, in <module>
    from apitools.base.protorpclite import message_types
  File "/Users/florenthemmi/dev/env/lib/python2.7/site-packages/apitools/base/protorpclite/message_types.py", line 25, in <module>
    from apitools.base.protorpclite import messages
  File "/Users/florenthemmi/dev/env/lib/python2.7/site-packages/apitools/base/protorpclite/messages.py", line 1165, in <module>
    class Field(six.with_metaclass(_FieldMeta, object)):
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

@jdemeyer
Copy link
Contributor

The issue in google-apitools is the same:

# TODO(rafek): Prevent additional field subclasses.
class Field(six.with_metaclass(_FieldMeta, object)):
    """Definition for message field."""

    __initialized = False  # pylint:disable=invalid-name
    __variant_to_type = {}  # pylint:disable=invalid-name

    # TODO(craigcitro): Remove this alias.
    #
    # We add an alias here for backwards compatibility; note that in
    # python3, this attribute will silently be ignored.
    __metaclass__ = _FieldMeta

@drewjaja
Copy link

drewjaja commented Sep 18, 2017

Got the same issue using a Django package.

File "/usr/local/lib/python2.7/site-packages/autocomplete_light/shortcuts.py", line 8, in <module>
   from .forms import *
 File "/usr/local/lib/python2.7/site-packages/autocomplete_light/forms.py", line 438, in <module>
   class ModelForm(six.with_metaclass(*bases)):
 File "/usr/local/lib/python2.7/site-packages/autocomplete_light/forms.py", line 283, in __new__
   attrs)
 File "/usr/local/lib/python2.7/site-packages/django/forms/models.py", line 208, in __new__
   new_class = super(ModelFormMetaclass, mcs).__new__(mcs, name, bases, attrs)
 File "/usr/local/lib/python2.7/site-packages/django/forms/forms.py", line 41, in __new__
   new_class = super(DeclarativeFieldsMetaclass, mcs).__new__(mcs, name, bases, attrs)
 File "/usr/local/lib/python2.7/site-packages/django/forms/widgets.py", line 153, in __new__
   new_class = super(MediaDefiningClass, mcs).__new__(mcs, name, bases, attrs)
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

@jdemeyer
Copy link
Contributor

Which version of django-autocomplete-light is that?

@drewjaja
Copy link

django-autocomplete-light 2.3.3

@jdemeyer
Copy link
Contributor

Same issue with django-autocomplete-light 2.3.3:

class ModelForm(six.with_metaclass(*bases)):
    """
    ModelForm override using our metaclass that adds our various mixins.

    .. py:attribute:: autocomplete_fields

        A list of field names on which you want automatic autocomplete fields.

    .. py:attribute:: autocomplete_exclude

        A list of field names on which you do not want automatic autocomplete
        fields.

    .. py:attribute:: autocomplete_names

        A dict of ``field_name: AutocompleteName`` to override the default
        autocomplete that would be used for a field.

    Note: all of ``autocomplete_fields``, ``autocomplete_exclude`` and
    ``autocomplete_names`` understand generic foreign key and generic many to
    many descriptor names.
    """
    __metaclass__ = ModelFormMetaclass

@ivanpricewaycom
Copy link

same for me using the module boxsdk.

just need to:
pip install six==1.11.0 boxsdk==2.0.0a8
python -c "import boxsdk"

et bim:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/site-packages/boxsdk/__init__.py", line 6, in <module>
    from .client import *  # pylint:disable=wildcard-import,redefined-builtin
  File "/usr/local/lib/python2.7/site-packages/boxsdk/client/__init__.py", line 5, in <module>
    from .client import Client
  File "/usr/local/lib/python2.7/site-packages/boxsdk/client/client.py", line 12, in <module>
    from ..object.events import Events
  File "/usr/local/lib/python2.7/site-packages/boxsdk/object/events.py", line 15, in <module>
    class EventsStreamType(with_metaclass(ExtendableEnumMeta, TextEnum)):
  File "/usr/local/lib/python2.7/site-packages/boxsdk/util/compat.py", line 77, in with_metaclass
    return type.__new__(TemporaryMetaSubclass, str('temporary_class'), bases, {})
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

using six 1.10.0 there is no problem.

@jdemeyer
Copy link
Contributor

boxsdk is more interesting because it uses six.with_metaclass in an unusual way to define a variant of with_metaclass:

def with_metaclass(meta, *bases, **with_metaclass_kwargs):
    """Extends the behavior of six.with_metaclass.
    [...]
    """
    temporary_class = six.with_metaclass(meta, *bases, **with_metaclass_kwargs)
    temporary_metaclass = type(temporary_class)

    class TemporaryMetaSubclass(temporary_metaclass):
        @classmethod
        def __prepare__(cls, name, this_bases, **kwds):  # pylint:disable=unused-argument
            return meta.__prepare__(name, bases, **kwds)

    return type.__new__(TemporaryMetaSubclass, str('temporary_class'), bases, {})

This is using with_metaclass internals, so it shouldn't be a surprise that it breaks.

It would be better if boxsdk would have a complete self-contained implementation if they don't like six.with_metaclass. The current mixture of six.with_metaclass with custom code is what causes the trouble.

@andy-maier
Copy link

andy-maier commented Sep 18, 2017

I have the same issue with the pywbem project. I can circumvent the issue by pinning the six version to 0.10.0, for the time being.

File six_meta_issue.py that is a minimal complete module that reproduces the issue:

import six

class MetaPywbemLoggers(type):
    loggers = {}

class PywbemLoggers(six.with_metaclass(MetaPywbemLoggers)):
    __metaclass__ = MetaPywbemLoggers

With six 1.11.0:

$ PYTHONPATH=. python -c "import six_meta_issue"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "six_meta_issue.py", line 6, in <module>
    class PywbemLoggers(six.with_metaclass(MetaPywbemLoggers)):
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

@jdemeyer
Copy link
Contributor

class PywbemLoggers(six.with_metaclass(MetaPywbemLoggers)):
    __metaclass__ = MetaPywbemLoggers

As discussed above, this is wrong: you should not combine six.with_metaclass and __metaclass__. However, as many people seem to do this, it's becoming clear that a fix is needed.

andy-maier added a commit to pywbem/pywbem that referenced this issue Sep 18, 2017
Details:

The `PywbemLoggers` class was defined by inheriting from
`six.with_metaclass(MetaPywbemLoggers)` and it also defined
the `__metaclass__` attribute.

Version 0.11.0 of the `six` package does not deal with this anymore
and raises:

  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

This change removes setting the `__metaclass__` attribute, which
is not needed anyway when using `six.with_metaclass()`, and which
is (or will at some point?) no longer supported in Python 3.

See also discussion in six issue #210:
benjaminp/six#210

Signed-off-by: Andreas Maier <[email protected]>
@andy-maier
Copy link

@jdemeyer Hi, I'm sorry for having overlooked the advice above.

After removing the setting of the __metaclass__ attribute, it seems to work on all Python versions we test with. Travis is slow currently, but locally it worked fine. Consider the issue solved for us.

Having said that, six is so commonly used, and this error is so easy to overlook, that a more tolerant way of dealing with this (maybe by raising a DeprecationWarning?) would be good.

@jdemeyer
Copy link
Contributor

I have a fix for the __metaclass__ issue at #211.

@jdemeyer
Copy link
Contributor

How feasible would it be to get a bugfix release out for six that resolves this?

What is "this"?

There is no bug in six.with_metaclass: in all the cases that I saw, it's the code using six.with_metaclass that is wrong. Of course, if several projects make the same mistake (combining six.with_metaclass and __metaclass__), it makes sense to support that. This is what I did in #211.

For the record in case it's not clear: I am not a six developer/maintainer, it just happens that I created the pull request #191 that started all this...

asfgit pushed a commit to apache/beam that referenced this issue Sep 20, 2017
@dghubble
Copy link
Author

dghubble commented Sep 20, 2017

With so many different projects impacted by #191, often its best to revert the commit in question and cut a point release to mitigate the issue. Reverts are easier to get in. Later, you can write and discuss a new implementation with the maintainers and if there really is a breaking change, draft a changelog note about it.

cuttlefish added a commit to cuttlefish/geonode that referenced this issue Sep 20, 2017
davisc pushed a commit to planetfederal/geonode that referenced this issue Sep 22, 2017
tacaswell added a commit to tacaswell/pims that referenced this issue Sep 25, 2017
danielballan pushed a commit to soft-matter/pims that referenced this issue Sep 25, 2017
travislbrundage pushed a commit to travislbrundage/geonode that referenced this issue Oct 20, 2017
travislbrundage pushed a commit to travislbrundage/geonode that referenced this issue Oct 23, 2017
travislbrundage pushed a commit to travislbrundage/geonode that referenced this issue Oct 24, 2017
@edmorley
Copy link

edmorley commented Nov 5, 2017

With so many different projects impacted by #191, often its best to revert the commit in question and cut a point release to mitigate the issue. Reverts are easier to get in. Later, you can write and discuss a new implementation with the maintainers and if there really is a breaking change, draft a changelog note about it.

Please can we release a version with the revert? This is impacting several projects (latest of which being SecurityInnovation/PGPy#217), and whilst it's their fault for incorrect usage, it's clearly a surprisingly common pattern, and has caught people off-guard due to it occurring in a minor version without a mention of breakage in the release notes.

Many thanks :-)

@jdemeyer
Copy link
Contributor

jdemeyer commented Nov 6, 2017

Or just apply my patch from #211.

@edmorley
Copy link

edmorley commented Nov 6, 2017

The quoted text explains why a revert would be preferable in the meantime :-)

@jdemeyer
Copy link
Contributor

jdemeyer commented Nov 6, 2017

in the meantime

I think we're already past that timeframe :-)

Anyway, I'm not disagreeing here: I just wanted to re-advertise my fix in case somebody missed it. In any case, it will be a maintainer of six who has to make the decision.

@saurabh-vyas
Copy link

saurabh-vyas commented Jul 17, 2018

Still learning my way about use of metaclass

so IIUC, we should not be using
__metaclass__ = NSG

but we are fine to use
data_type = getattr(data, "__metaclass__", type(data))

or the later also should not be used?
Please if anyone can clarify?

Thanks in adv !

@dghubble
Copy link
Author

Cleaning up my open issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants