tilezen/mapbox-vector-tile

python3 support?

yohanboniface opened this issue · 16 comments

yohanboniface commented

Just made a quick try, doesn't seem to work:

  • relative imports
  • protobuf 2.6 instead of 3.0alpha3 (which API seems to have changed a bit)

README says it's compatible. Do you have a hidden branch with the fixes or it's me missing something? :)

rmarianski commented

I just tried python 3 and it doesn't work for me either, so you're not missing anything :). We run with python 2.7 in all our environments. @hkrishna?

I updated the readme to only say that python 2.6 and 2.7 are supported for now.

Is the move to the protobuf 3 alpha required to support python 3? Some searching suggests that the metaclass syntax is problematic in older versions, but maybe it's possible to hack it to make it work? I would feel a little nervous moving to an alpha version of a new major release.

yohanboniface commented

Is the move to the protobuf 3 alpha required to support python 3?

Sounds like: protocolbuffers/protobuf#166 (comment)

Some searching suggests that the metaclass syntax is problematic in older versions, but maybe
it's possible to hack it to make it work? I would feel a little nervous moving to an alpha version of a
new major release.

I understand that, but in the same time it's an alpha3, so not the first round, and if you have unittests covering the use cases of MVT this can be quite safe to handle the upgrade. And BTW, that may be an alpha version here too, or just a branch that can be used, and then tested by people using python 3. :)

rmarianski commented

Yea, a separate python 3 supported branch that tracks the protobuf alpha in the interim sounds fine to me.

hkrishna commented

@rmarianski yes, we run python 2.7 in all our environments (as of few weeks ago)

@yohanboniface a python3 branch sounds good.. we should probably add a line about the branch to the README as well

yohanboniface commented

(as of few weeks ago)

Does that means that this package has worked in python 3 at some point in history?

I can give a hand in porting the python part with pleasure, but honestly the protobuf API is Chinese for me.

rmarianski commented

Does that means that this package has worked in python 3 at some point in history?

My guess is that it didn't work with python3 and the note in the readme was aspirational, but I could be completely wrong about that :) I myself haven't tried python3 before now. @hkrishna?

I can give a hand in porting the python part with pleasure, but honestly the protobuf API is Chinese for me.

That sounds good. We probably won't get to move this forward on our end much, but I'd be happy to try and provide any support to help.

Instead of bumping to protobuf 3 alpha, maybe what we could try instead is to just edit the generated generated files to work in either python 2 or 3 [1]. This might be end up being a shorter step in the interim. Btw, I haven't tried this, even with a simple example, so not sure if that's all it would take.

[1] http://stackoverflow.com/questions/25036487/protocol-buffers-in-python-3-notimplementederror

yohanboniface commented

Instead of bumping to protobuf 3 alpha, maybe what we could try instead is to just edit the generated generated files to work in either python 2 or 3 [1].

The "solution" linked seems like a python 3 only, metaclass as argument will fail at parsing I think.

I can make a POC in this direction, but will definitely profit some help when on the protobuf side.

rmarianski commented

The "solution" linked seems like a python 3 only, metaclass as argument will fail at parsing I think.

Yea, you're right. I was thinking of doing something like checking sys.version_info.major and having the classes that used metaclasses be defined differently based on the version. Not pretty, but it should work, assuming that's all that it takes.

yohanboniface commented

Not sure it will even parse, actually :/

yohanboniface commented

So, if I do a quick and dirty python 3 compat on MVT side (see #18), I hit this:

Traceback (most recent call last):
  File "/home/ybon/.virtualenvs/utilery3/lib/python3.4/site-packages/protobuf-3.0.0a3-py3.4.egg/google/protobuf/internal/python_message.py", line 1008, in MergeFromString
  File "/home/ybon/.virtualenvs/utilery3/lib/python3.4/site-packages/protobuf-3.0.0a3-py3.4.egg/google/protobuf/internal/python_message.py", line 1030, in InternalParse
  File "/home/ybon/.virtualenvs/utilery3/lib/python3.4/site-packages/protobuf-3.0.0a3-py3.4.egg/google/protobuf/internal/decoder.py", line 189, in ReadTag
TypeError: unsupported operand type(s) for &: 'str' and 'int'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "utilery/serve.py", line 3, in <module>
    from utilery.core import app
  File "/home/ybon/Code/py/utilery/utilery/core.py", line 56, in <module>
    import utilery.views  # noqa
  File "/home/ybon/Code/py/utilery/utilery/views.py", line 12, in <module>
    import mapbox_vector_tile
  File "/home/ybon/Code/py/mapbox-vector-tile/mapbox_vector_tile/__init__.py", line 1, in <module>
    from . import encoder
  File "/home/ybon/Code/py/mapbox-vector-tile/mapbox_vector_tile/encoder.py", line 2, in <module>
    from .Mapbox import vector_tile_pb2
  File "/home/ybon/Code/py/mapbox-vector-tile/mapbox_vector_tile/Mapbox/vector_tile_pb2.py", line 139, in <module>
    options=_descriptor._ParseOptions(descriptor_pb2.FieldOptions(), '\020\001')),
  File "/home/ybon/.virtualenvs/utilery3/lib/python3.4/site-packages/protobuf-3.0.0a3-py3.4.egg/google/protobuf/descriptor.py", line 820, in _ParseOptions
  File "/home/ybon/.virtualenvs/utilery3/lib/python3.4/site-packages/protobuf-3.0.0a3-py3.4.egg/google/protobuf/message.py", line 185, in ParseFromString
  File "/home/ybon/.virtualenvs/utilery3/lib/python3.4/site-packages/protobuf-3.0.0a3-py3.4.egg/google/protobuf/internal/python_message.py", line 1014, in MergeFromString
google.protobuf.message.DecodeError: Truncated message.

so I'm not sure protobuf is really ready for python 3, actually :/

rmarianski commented

I think you'll have to regenerate the pbf wrappers when changing the protobuf version, and then account for whatever the api changes are.

I tried a simple example with conditionally defining the classes, and you're right, it didn't parse. I think this strategy might still work by conditionally importing the classes from separate files. A proof of concept seemed to work for me, but I didn't get a chance to try it on the actual pbf wrappers. ie something like this:

if sys.version_info.major == 2:
    from metaclasses2 import Foo
elif sys.version_info.major == 3:
    from metaclasses3 import Foo

Where the metaclasses2 file has the python 2 syntax for defining metaclasses, and similarly for python 3.

yohanboniface commented

This trick may worth a try too:

# Define the Enum class using metaclass syntax compatible with both Python 2
# and Python 3.
Enum = EnumMetaclass(str('Enum'), (), {
    '__doc__': 'The public API Enum class.',
    })

I think you'll have to regenerate the pbf wrappers when changing the protobuf version.

Do you remember from the top of your head how would I do that?

rmarianski commented

I think you'll have to regenerate the pbf wrappers when changing the protobuf version.

Do you remember from the top of your head how would I do that?

https://developers.google.com/protocol-buffers/docs/pythontutorial#compiling-your-protocol-buffers

I suspect that you might need to use the version of protoc that matches the protobuf library, but I'm not sure about that.

yohanboniface commented

Made a quick shot, seems that the .proto needs to be updated to new proto3 syntax:

edoardo:~/C/p/m/m/Mapbox (python3 ⚡=) protoc vector_tile.proto --python_out=.
vector_tile.proto: Extension ranges are not allowed in proto3.
vector_tile.proto: Explicit default values are not allowed in proto3.
vector_tile.proto: Required fields are not allowed in proto3.
vector_tile.proto: Explicit default values are not allowed in proto3.
vector_tile.proto: Required fields are not allowed in proto3.
vector_tile.proto: Explicit default values are not allowed in proto3.
vector_tile.proto: Extension ranges are not allowed in proto3.
vector_tile.proto: Extension ranges are not allowed in proto3.
vector_tile.proto: Lite runtime is not supported in proto3.

Extension seems to be now replace by Any, but at the moment I don't get my head around it.

yohanboniface commented

Actually, it's possible to keep proto2 syntax while using protoc 3.0.0: https://github.com/mapzen/mapbox-vector-tile/pull/18/files#diff-aa5a952047d7e07608d4371eccace6eeR3

rmarianski commented

Merged in the relevant pull request: #18