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 syntax is not py3k-compatible #2121

Closed
wchargin opened this issue Apr 18, 2019 · 12 comments · Fixed by #2242
Closed

Metaclass syntax is not py3k-compatible #2121

wchargin opened this issue Apr 18, 2019 · 12 comments · Fixed by #2242

Comments

@wchargin
Copy link
Contributor

Low-priority.

Our codebase assigns to __metaclass__ to set metaclasses, but this
only works in Python 2. The portable way is to use six.add_metaclass,
which works as a decorator. The following occurrences are broken in
Python 3:

$ git grep '__metaclass__ ='
tensorboard/backend/event_processing/db_import_multiplexer.py:  __metaclass__ = abc.ABCMeta
tensorboard/plugins/base_plugin.py:  __metaclass__ = ABCMeta
tensorboard/plugins/beholder/video_writing.py:  __metaclass__ = abc.ABCMeta
tensorboard/program.py:  __metaclass__ = ABCMeta

Demo:

$ cat /tmp/meta.py; echo
import abc


class Foo(object):
  __metaclass__ = abc.ABCMeta

  @abc.abstractmethod
  def foo(self):
    pass


Foo()  # should fail!

$ python2 /tmp/meta.py; echo $?
Traceback (most recent call last):
  File "/tmp/meta.py", line 12, in <module>
    Foo()  # should fail!
TypeError: Can't instantiate abstract class Foo with abstract methods foo
1
$ python3 /tmp/meta.py; echo $?
0
@shashvatshahi1998
Copy link
Contributor

@wchargin @nfelt Can you provide me with the link where i can contribute as I am not able to find the doc

@shashvatshahi1998
Copy link
Contributor

Ok i got the files to contribute.

@shashvatshahi1998
Copy link
Contributor

shashvatshahi1998 commented Apr 20, 2019

@nfelt I have to repalce abstractmethod with @abc.abstractmethod in all functions in that class

@wchargin
Copy link
Contributor Author

I have to repalce abstractmethod with @abc.abstractmethod in all
functions in that class

No, the problem isn’t to do with the abstractmethod declarations. The
two forms @abc.abstractmethod and @abstractmethod are equivalent
(our style guide does require that we only import modules, not symbols,
but the functionality is the same in this case).

Rather, the problem is that the classes’ metaclasses are not being
declared properly. For instance, it should not be possible to
instantiate a tensorboard.plugins.base_plugin.TBPlugin object. If you
try to do this in Python 2, you properly get an error:

>>> import sys
>>> sys.version_info
sys.version_info(major=2, minor=7, micro=16, releaselevel='candidate', serial=1)
>>> from tensorboard.plugins import base_plugin
>>> p = base_plugin.TBPlugin()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Can't instantiate abstract class TBPlugin with abstract methods get_plugin_apps, is_active

But in Python 3 you do not get an error:

>>> import sys
>>> sys.version_info
sys.version_info(major=3, minor=6, micro=5, releaselevel='final', serial=0)
>>> from tensorboard.plugins import base_plugin
>>> p = base_plugin.TBPlugin()
>>> type(p)
<class 'tensorboard.plugins.base_plugin.TBPlugin'>

This is the problem. It should be an error in all Python versions.

@shashvatshahi1998
Copy link
Contributor

So basically we have to make those classes compatible with both python 2 and python 3. But I am not able to understand what's the bug in their declaration which makes them incompatible with python 2 they appear perfectly fine in documents.

@wchargin
Copy link
Contributor Author

But I am not able to understand what's the bug in their declaration
which makes them incompatible with python 2

The current forms that we use are compatible with Python 2 but not
Python 3.

Python 3 changed the metaclass syntax, and Python 2 is not
forward-compatible with the new Python 3 syntax.

The decorator @six.add_metaclass provides an interface that works in
both Python 2 and Python 3.

@shashvatshahi1998
Copy link
Contributor

shashvatshahi1998 commented Apr 20, 2019

class TensorBoardServer(object):
"""Class for customizing TensorBoard WSGI app serving."""
@six.add_metaclass
def init(self, wsgi_app, flags, metaclass=ABCMeta):
"""Create a flag-configured HTTP server for TensorBoard's WSGI app.
Args:
wsgi_app: The TensorBoard WSGI application to create a server for.
flags: argparse.Namespace instance of TensorBoard flags.
"""
raise NotImplementedError()
@six.add_metaclass
def serve_forever(self, metaclass=ABCMeta):
"""Blocking call to start serving the TensorBoard server."""
raise NotImplementedError()
@six.add_metaclass
def get_url(self, metaclass=ABCMeta):
"""Returns a URL at which this server should be reachable."""
raise NotImplementedError()

@wchargin I have modified TensorboardServer class of tensorboard/program.py. Can you please tell me that whether this is right way or not?

@wchargin
Copy link
Contributor Author

wchargin commented Apr 20, 2019

When posting comments, please use code formatting for code so that it
renders properly. Like this:

Here is my code:

```python
class TensorBoardServer(object):
  """Class for customizing TensorBoard WSGI app serving."""

  @six.add_metaclass
  def __init__(self, wsgi_app, flags, metaclass=ABCMeta):
    """Create a flag-configured HTTP server for TensorBoard's WSGI app.

    Args:
      wsgi_app: The TensorBoard WSGI application to create a server for.
      flags: argparse.Namespace instance of TensorBoard flags.
    """
    raise NotImplementedError()

  @six.add_metaclass
  def serve_forever(self, metaclass=ABCMeta):
    """Blocking call to start serving the TensorBoard server."""
    raise NotImplementedError()

  @six.add_metaclass
  def get_url(self, metaclass=ABCMeta):
    """Returns a URL at which this server should be reachable."""
    raise NotImplementedError()
```

Here is some more text.

This way, the indentation and line breaks are preserved (which is
important in all languages but actually significant in Python!), and
strings like “__init__” aren’t rendered as “init”.

Before submitting your comment, you can “Preview” it to make sure that
it appears correctly.

Now, to your question: Do you think that this is correct? What does it
do? How have you tested it? Remember the problem that we’re trying to
solve—can you write a test that verifies that the bug has been fixed?

@shashvatshahi1998
Copy link
Contributor

No I have not tested it. I just tried to change it according to the guide provided by you in previous comments.

@thealphacod3r
Copy link
Contributor

@shashvatshahi1998 is this done or anything left, if left can i work on this??

@shashvatshahi1998
Copy link
Contributor

@amanp592 I am working on further modification as told by wchargin

@thealphacod3r
Copy link
Contributor

@shashvatshahi1998 ohk cool

wchargin added a commit that referenced this issue May 17, 2019
Summary:
Fixes #2121.

Test Plan:

    $ git grep '__metaclass__' | wc -l
    0

wchargin-branch: six-add-metaclass
wchargin added a commit that referenced this issue May 17, 2019
Summary:
Fixes #2121.

Test Plan:

    $ git grep '__metaclass__' | wc -l
    0

wchargin-branch: six-add-metaclass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants