139: Split up S2Connection into a sync and async interface as well as decoupling the underlying medium e.g. websockets, mqtt and d-bus interfaces

[lfse-slafleur]

No description provided.

[lfse-slafleur]

@philipptrenz I can't put you as an official reviewer but I would be very interested in your thoughts and comments. Finally had time to finish this PR and it should allow you as well as anyone else to use just the S2Connection on top of any 'medium', use it fully sync or fully async as well as allow for extension towards a CEM handler.

@jorritn @wcoenraads I don't have time to add the CEM handler similarly to the ResourceManager. PR is big enough already so perhaps we should do that separately.

[philipptrenz]

Hi @lfse-slafleur, great thanks. Will have a detailed look, however will probably be only after next week.

[wcoenraads]

Overall this looks like an excellent change! I like the abstraction for the communication medium. I didn't do a close line-by-line read of the code, but I think I covered everything pretty well.

A few overall comments in addition to the ones in the code:

• I'm still not a huge fan of the "provide us your handlers and we'll manage them" style of API; I prefer it when the developer writes their own main loop. But that's more of a taste issue.
• In almost all cases, the developer wants to integrate external signals (i.e. from the hardware) that cause new S2 messages to be sent. Can we show how to do this in the examples? I imagine this differs a bit between the sync and async case.
• I'm missing a disconnect method on the S2 medium classes. Depending on the used medium, disconnecting properly can involve some actions (e.g. sending a close frame for Websockets). I assume disconnecting is now done by just dropping the connection object?
• The library currently assumes that the data going over the line is JSON, and passes all received strings/bytes to S2Parser to parse it as JSON. I could also imagine pulling that into the medium, so you would get e.g. a WebsocketsJSONMedium and if you for some reason want to use YAML you could implement a WebsocketsYAMLMedium. This might be nice for future flexibility (but also quite hypothetical).

Thanks for your work on this large change!

[lfse-slafleur]

Overall this looks like an excellent change! I like the abstraction for the communication medium. I didn't do a close line-by-line read of the code, but I think I covered everything pretty well.

A few overall comments in addition to the ones in the code:

* I'm still not a huge fan of the "provide us your handlers and we'll manage them" style of API; I prefer it when the developer writes their own main loop. But that's more of a taste issue.

You are referring to the .run() functions that block? With the way ReceptationStatuses and messages come in an arbitrary order, something needs to continuously run the connection. I know some libraries do this implicitely when certain functions are called on the connection, but I personally I don't like that implicit progress handling as you then link the lifecycles of those code flows. Do you have an alternative in mind?

* In almost all cases, the developer wants to integrate external signals (i.e. from the hardware) that cause new S2 messages to be sent. Can we show how to do this in the examples? I imagine this differs a bit between the sync and async case.

You mean sending a message arbritarely that is not part of a handler? I can add that! With both sync and async it is actually very similar. The send functions are respectively async and thread safe so if the S2 connection is run by its own background task or thread, other tasks or threads can just send a message on the

* I'm missing a disconnect method on the S2 medium classes. Depending on the used medium, disconnecting properly can involve some actions (e.g. sending a close frame for Websockets). I assume disconnecting is now done by just dropping the connection object?

Yeah I am assuming the medium is handled completely outside of S2. So no connects, disconnects or reconnects. This actually fits the preference you mentioned earlier where a user of the library has to construct its own main loop but at some point gives control to the connection through .run().

* The library currently assumes that the data going over the line is JSON, and passes all received strings/bytes to `S2Parser` to parse it as JSON. I could also imagine pulling that into the medium, so you would get e.g. a `WebsocketsJSONMedium` and if you for some reason want to use YAML you could implement a `WebsocketsYAMLMedium`. This might be nice for future flexibility (but also quite hypothetical).

Yeah I doubted if I wanted to pull that in already but considering its a huge change already, I opted that we pull it out later if we ever need it.

Thanks for your work on this large change!

You are welcome! Hopefully this gives us the async and sync control we want. Fixing the comments now.

[wcoenraads]

You are referring to the .run() functions that block? With the way ReceptationStatuses and messages come in an arbitrary order, something needs to continuously run the connection. I know some libraries do this implicitely when certain functions are called on the connection, but I personally I don't like that implicit progress handling as you then link the lifecycles of those code flows. Do you have an alternative in mind?

Oh, maybe this is just my inexperience with Python asyncio showing. I'm used to async systems where you can just spawn a background task and that'll drive the connection without having to wait on that specific task. If that's not the case here, then I understand the design a lot more.

You mean sending a message arbritarely that is not part of a handler? I can add that! With both sync and async it is actually very similar. The send functions are respectively async and thread safe so if the S2 connection is run by its own background task or thread, other tasks or threads can just send a message on the

Perfect! This is a common issue I hear from people using the library, so I think adding an official example would go a long way towards fixing that.

Yeah I am assuming the medium is handled completely outside of S2. So no connects, disconnects or reconnects. This actually fits the preference you mentioned earlier where a user of the library has to construct its own main loop but at some point gives control to the connection through .run().

So if the user wants to disconnect the connection properly, how would they do that? If the user implements their own medium it's not too difficult, but the provided mediums don't provide any functionality for it.

Yeah I doubted if I wanted to pull that in already but considering its a huge change already, I opted that we pull it out later if we ever need it.

Makes sense to me!

[wcoenraads]

I just spotted one more issue: in async_/medium/websocket.py, one of the imports from websockets is not gated behind the ws try/catch:

import logging
import ssl
from typing import AsyncGenerator, Optional, Dict, Any
from typing_extensions import override
from websockets import Data # <--- this one

from s2python.connection.async_.medium.s2_medium import (
    MediumClosedConnectionError,
    MediumCouldNotConnectError,
    S2AsyncMediumConnection,
    UnparsedMediumData,
)

try:
    import websockets
    from websockets.asyncio.client import (
        ClientConnection as WSConnection,
        connect as ws_connect,
    )
except ImportError as exc:
    raise ImportError(
        "The 'websockets' package is required. Run 'pip install s2-python[ws]' to use this feature."
    ) from exc

[lfse-slafleur]

You are referring to the .run() functions that block? With the way ReceptationStatuses and messages come in an arbitrary order, something needs to continuously run the connection. I know some libraries do this implicitely when certain functions are called on the connection, but I personally I don't like that implicit progress handling as you then link the lifecycles of those code flows. Do you have an alternative in mind?

Oh, maybe this is just my inexperience with Python asyncio showing. I'm used to async systems where you can just spawn a background task and that'll drive the connection without having to wait on that specific task. If that's not the case here, then I understand the design a lot more.

You can spawn a background task, but eventually you need to await it to return the exception or result. If you do not, then you will never receive the actual exception if it crashed. It just stays quiet and does nothing. For example using the commit I just wrote: fb96895

If you look at the async part at the SendPowerMeasurementPeriodically task, I start it with asyncio.create_task and then await if after .cancel(). You only get the exception at the 'await' unfortunately if something goes wrong. That is why on S2AsyncConnection.run() you force a user to wrap it in whatever way they want. As a asyncio.create_task that they then manage themselves or some other way. This forces them to decide how they want to be notified in case of exceptions.

You mean sending a message arbritarely that is not part of a handler? I can add that! With both sync and async it is actually very similar. The send functions are respectively async and thread safe so if the S2 connection is run by its own background task or thread, other tasks or threads can just send a message on the

Perfect! This is a common issue I hear from people using the library, so I think adding an official example would go a long way towards fixing that.

Good idea to immediately do something about it. I included a way to retrieve the PowerMeasurement periodically in the examples. Is this what you have in mind? If not, curious to hear about other use cases we can include: fb96895

Yeah I am assuming the medium is handled completely outside of S2. So no connects, disconnects or reconnects. This actually fits the preference you mentioned earlier where a user of the library has to construct its own main loop but at some point gives control to the connection through .run().

So if the user wants to disconnect the connection properly, how would they do that? If the user implements their own medium it's not too difficult, but the provided mediums don't provide any functionality for it.

Gotcha, will fix! Then it wont be an interface function, but it will be a function on the provided websocket medium. Thanks :D

Yeah I doubted if I wanted to pull that in already but considering its a huge change already, I opted that we pull it out later if we ever need it.

Makes sense to me!

[lfse-slafleur]

I just spotted one more issue: in async_/medium/websocket.py, one of the imports from websockets is not gated behind the ws try/catch:

import logging
import ssl
from typing import AsyncGenerator, Optional, Dict, Any
from typing_extensions import override
from websockets import Data # <--- this one

from s2python.connection.async_.medium.s2_medium import (
    MediumClosedConnectionError,
    MediumCouldNotConnectError,
    S2AsyncMediumConnection,
    UnparsedMediumData,
)

try:
    import websockets
    from websockets.asyncio.client import (
        ClientConnection as WSConnection,
        connect as ws_connect,
    )
except ImportError as exc:
    raise ImportError(
        "The 'websockets' package is required. Run 'pip install s2-python[ws]' to use this feature."
    ) from exc

Fixed: 9aff094

[lfse-slafleur]

@wcoenraads I believe I got everything either answered or fixed. Thanks! Could you have another look if you approve the changes?

[wcoenraads]

Nice, thanks Sebastiaan! These changes look good and address my questions. I think this is ready to merge!

[lfse-slafleur]

@wcoenraads Much appreciated! Thanks for the support!

Just released this PR under v0.9.0

@philipptrenz Still very interested in your feedback and if this approach helps you guys out as well regarding d-bus. In case you have anything that you would like to see changed, definitely let us know and we can tackle it in a new ticket.