tiagocoutinho/multivisor

Python 3 support

Closed this issue ยท 11 comments

Hello, I am happy to say that supervisor is finally supporting Python 3! Supervisor/supervisor#1060

Python 2.7 will be maintained only till end of 2019, so I think it's crucial to provide python 3 support. I can work on this issue.

@guy881,
Thanks for the news and for volunteering.
Support for python 3 would be greatly appreciated.
Feel free to work on it. I will be happy to review a PR

@tiagocoutinho
One of the blockers here is louie which still hasn't got Python 3 compatible version on PyPI, although codebase is ready since 2016. 11craft/louie#6

The other problem I am facing right now is associated with zerorpc Client functions: getAllProcessInfo and event_stream, when using supervisor which runs on Python 2.7 we get dictionaries with keys and values of type bytes, which raises KeyErrors on usual access of course. If you have any idea why this is happening, I will be grateful, I am not familiar with zerorpc.

image

// edit
I have managed to fix unusual behaviour with bytes being returned instead of strings, but it's not very pretty. @tiagocoutinho do you think that might be the issue with supervisor? I found some recent issues, where they mention about changing their inner data flow to use bytes.

One of the blockers here is louie which still hasn't got Python 3 compatible version on PyPI, although codebase is ready since 2016. 11craft/louie#6

I propose:

  1. wait and see if the louie team reacts to your request
  2. if there is no response in a week assume the project is stalled and replace louie with another dispatcher (blinker for example)

The other problem I am facing right now is associated with zerorpc Client functions: getAllProcessInfo and event_stream, when using supervisor which runs on Python 2.7 we get dictionaries with keys and values of type bytes, which raises KeyErrors on usual access of course. If you have any idea why this is happening, I will be grateful, I am not familiar with zerorpc.

Not sure I understand: on python 2.7, bytes and str are the same. I guess you are running into trouble with python 3, not python 2, right?

As far as I understand, zerorpc uses msgpack codec. I think the problem is, on python 3, you encode a str and when you decode it you get back bytes:

>>> import msgpack
>>> info = {"hello": "world"}  # a dict with key and value being str
>>> data = msgpack.dumps(info)
>>> msgpack.loads(data)  # this comes back as a dict with key and value being bytes!
{b'hello': b'world'}

Concerning zerorpc:
I had a closer look and it seems zerorpc configures msgpack in a way that it respects the data type you send it. I guess it is on the multivisor side to fix the problem.

I think now I understand what you mean:

  • supervisor runs on python 2.7; supervisor process info is encoded with fields being bytes
  • multivisor runs on python 3; it decodes process info and sees bytes

On python 2 this didn't matter but on python 3 it becomes an error.

Probably the proper patch is, on python 2, to decode str to unicode as soon as possible in the chain. This means intercepting in the multivisor rpc every call and translate it to unicode on python 2. What do you think @guy881?

@tiagocoutinho thanks a lot for investigating this issue! ๐Ÿ‘
Exactly, supervisor runs on 2.7, multivisor on python 3 and here it becomes an error.

Your proposed patch seems reasonable, but I'm having trouble implementing it.
I found server_after_exec hook in zerorpc, however it seems that when using stream decorator we cannot modify event there. From code:

    def hook_server_after_exec(self, request_event, reply_event):
        """Called when a method has been executed successfully.

        This hook is called right before the answer is sent back to the client.
        If the method streams its answer (i.e: it uses the zerorpc.stream
        decorator) then this hook will be called once the reply has been fully
        streamed (and right before the stream is "closed").

Which is a shame, because we cannot modify it in one place. ๐Ÿ˜ž Maybe it would be possible on the client hook, would that be okay?

Currently I created ServerMiddleware class which is converting dicts from str keys and values to unicode. But in _process_event there is also a similar thing done to streamed events.

I created a pull request, so you can see the current state of work, but if we can come up with something better then I will definitely opt for it. Have a look at rpc.py.

I think it'll be better if you just use your fork of the python2 library as they are not going to listen. It's not updated regularly, so there is no reason to wait for a new release and you won't be maintaining it as it is essentially legacy

Okay, I refactored the code to use blinker instead of louie. Of course It would be good to do some tests before merging this. Here's the PR: #40

Thanks @guy881. I will have a look

@tiagocoutinho Thanks for all your work on this. Any updates by chance?

Hi, sorry for the long period of silence.
So, yeah this issue is quite important since supervisor has add support for python 3.

Its been a long time since I had a look last time at #40 so am afraid I need to review from zero.

I owe an apology to @guy881 for the lack of help with this issue.

Finally, thanks to the hard work of @guy881 this is merged in develop.
Will make a release soon