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

Simplify and extend usage documentation #131

Merged
merged 7 commits into from
Oct 16, 2022
Merged

Simplify and extend usage documentation #131

merged 7 commits into from
Oct 16, 2022

Conversation

empicano
Copy link
Collaborator

@empicano empicano commented Oct 7, 2022

Hi Frederik!

The Advanced usage section may be a bit overwhelming at first sight. I was thinking about dissecting it into smaller sections/examples that each explains only a single concept e.g. Reconnecting (see lines 41-56).

This makes it more in line with the TLS and Proxying sections and would make it easier to add/remove/edit the examples in the future. I'd also like to add a section about topic filters, closing #105.

Let me know what you think 😊

-- Felix

@frederikaalund
Copy link
Member

Hi Felix. :)

Thanks again for taking your time to improve asyncio-mqtt. It's really appreciated. 👍

Great idea to make the readme more approachable/comprehensible. In general, we lack on the documentation/examples side. I welcome all improvements in that area.

As for your specific suggestion, I think it makes great sense to split the readme up into smaller sections. Makes it easier to digest a given topic. Maybe this grows into an actual documentation (e.g., "Read the Docs" style) in the future.

So by all means, go ahead. :)

~Frederik

@frederikaalund
Copy link
Member

Your contributions so far are on point and in line with my own thinking/direction. If you're up for it, you're very welcome to join the asyncio-mqtt team. :) There are no strings attached, you just contribute/maintain asyncio-mqtt whenever you feel like it (or not at all if you grow tired of it).

You'll get direct read/write rights to the repo, so you can change the code directly without PRs. Of course, you're welcome to keep the PR workflow and I'll gladly review/comment on the PRs. For minor/non-breaking changes, it's just easier to edit the repo directly.

~Frederik

@empicano
Copy link
Collaborator Author

empicano commented Oct 8, 2022

As for your specific suggestion, I think it makes great sense to split the readme up into smaller sections. Makes it easier to digest a given topic.

Great! I'll write something down in the next few days :)

Your contributions so far are on point and in line with my own thinking/direction. If you're up for it, you're very welcome to join the asyncio-mqtt team. :) There are no strings attached, you just contribute/maintain asyncio-mqtt whenever you feel like it (or not at all if you grow tired of it).

Sure, I'm up for it, thanks for the trust! I'll keep with the PR workflow for most things I think, I like when someone looks over it, then I can learn from the comments :)

@frederikaalund
Copy link
Member

Great! I'll write something down in the next few days :)

Sounds good! I'll be ready to review the PR.

Sure, I'm up for it, thanks for the trust! I'll keep with the PR workflow for most things I think, I like when someone looks over it, then I can learn from the comments :)

Exellent! I'll add you right away. Welcome aboard. :)

@frederikaalund
Copy link
Member

Looking good so far. 👍 I especially like the new ### Configuring the client section. It neatly showcases the available functionality, Likewise, it's nice with several, specialized examples instead of a single, "advanced" example.

Could merge as is but let me know when you want to do so. :)

@empicano
Copy link
Collaborator Author

empicano commented Oct 9, 2022

Great that you like it! 🎉


I stumbled on two things while writing this down. (1.) In #105 you said that "we use filtered_messages to deinterleave [...] messages". I still don't really understand the benefit of the example in the README over:

async with Client("test.mosquitto.org") as client:
    async with client.unfiltered_messages() as messages:
        await client.subscribe("measurements/#")
        async for message in messages:
            if message.topic == "measurements/humidity":
                # do something humidity specific
            if message.topic == "measurements/temperature":
                # do something temperature specific

Is it that with filtered_messages messages of different topics can be processed concurrently? Or that the different filters can be changed afterward more easily? Or that the filters can include special characters like measurements/+/something (though that could be easily implemented differently)?

The current way seems overly complicated to me at the moment, so I feel like I'm not seeing something 😄


(2.) is that I don't understand how to share an MQTT connection between functions. In #82 (which was sadly handled very childishly by the issue author) you said that sharing the client between different files is an anti-pattern.

I searched around quite a bit, but I didn't find anything for "sharing context managers", "sharing database connection via context managers", or similar. Maybe I'm searching in the wrong direction. Could you give me some pointers on how I can find more information on this? Or is the idea to open a new connection for each message?

I'd like to add a section about this to the README as well.

@frederikaalund
Copy link
Member

Great questions. Let me try to answer them. :)

Indeed, it's a bit strange that we have both filtered_messages vs unfiltered_messages. It's because paho-mqtt has both. Personally, I would just have unfiltered_messages (and simply call it messages) and then do the filtering manually (like you do with ifs in your example code).
asyncio-mqtt is just a wrapper around paho-mqtt. In that spirit, I decided to not change the interface too much (and keep the filtered functionality). Maybe that wasn't a wise decision. 😅
As far as I know, paho-mqtt basically reimplements the topic wildcards (+ and *) on the client for filtered_messages. So it does provide some convenience over simple ifs. Enough convenience to warrant the existence of filtered? Personally, I don't really think so but the paho-mqtt developers apparently thought so. :)


Yeah, #82 did have some merit because we don't have examples to show how to do it proper. The conversation just got a bit off track.
To share a client between multiple functions, just pass the Client instance to the function! That's all there is to it. I usually have the async with Client() in the "main" function and then pass said Client to all other functions that want to subscribe/publish.

However, there are two problems with this approach:

  1. Some frameworks take control over the "main" function, which makes it difficult to figure out where to create/enter the Client.
  2. What if the connection breaks? There is no reconnect functionality.

Frameworks such as FastAPI solve (1) via their dependency system. We can give an example for FastAPI just to show how.
Problem (2) is more difficult. It requires more fundamental change. Either we make Client itself "auto-reconnect" internally, or, we create a wrapper around Client (say, ReconnectingClient) that does so. I neither case, it requires some dev work (PRs are welcome :D)


I hope that answers your questions. Those were good questions that showcases some of the rough edges in asyncio-mqtt. Keep them coming. 👍

@empicano
Copy link
Collaborator Author

(1.) I get that, staying close to paho.mqtt makes the asyncio-mqtt much leaner. Personally, I don't think that's worth having to deal with AsyncExitStack for multiple filters. paho.mqtt does indeed reimplement the topic wildcards in paho/mqtt/matcher.py. The logic is pretty simple. If you're ok with it, I'll take a stab at this in a while, for something like:

async with Client("test.mosquitto.org") as client:
    async with client.messages() as messages:
        await client.subscribe("measurements/#")
        async for message in messages:
            if matches(payload.topic, "measurements/+/something"):
                print(message.payload)

(2.) That makes sense! I'm actually using asyncio-mqtt inside a web framework, that's why I am struggling. Using FastAPI's dependency injection to share context is pretty neat, for other frameworks it sadly doesn't work that easily.

I read in e.g. #117 that you're leaning against using connect/disconnect directly. I think it could be a nice solution for other web frameworks, usually, there's some sort of startup/shutdown procedure. It doesn't seem too unusual to have both either, see encode/databases.

Automating reconnection would be pretty cool, I already considered that as one of the next things to do 👍

@empicano
Copy link
Collaborator Author

I now also added an example of how to start a listener inside a web framework without blocking the rest of the code. I'm happy about all feedback! 😊

@empicano empicano marked this pull request as ready for review October 13, 2022 20:39
@frederikaalund
Copy link
Member

(1.) [...] The logic is pretty simple. If you're ok with it, I'll take a stab at this in a while, for something like:

async with Client("test.mosquitto.org") as client:
    async with client.messages() as messages:
        await client.subscribe("measurements/#")
        async for message in messages:
            if matches(payload.topic, "measurements/+/something"):
                print(message.payload)

That's much cleaner and more approachable than the current API. Well done. 👍 Feels more natural to split up the semantics (iterating messages and filtering messages) into separate operations. I also see how we can add this in a backwards-compatible way. I welcome a PR with that change. :)

You could also choose to do a Message.matches like so:

 if message.matches("measurements/+/something"):

Of course, that'll mean that we have to create a asyncio_mqtt.MqttMessage class that wraps mqtt.MQTTMessage. I just put the idea here as an alternative. You decide which one that you want to proceed with (matches, Message.matches, or something new). 👍

(2.) That makes sense! I'm actually using asyncio-mqtt inside a web framework, that's why I am struggling. Using FastAPI's dependency injection to share context is pretty neat, for other frameworks it sadly doesn't work that easily.

I have some recent (though not direct) experience with FastAPI and it looks like a great framework. Can't say for other frameworks but I fear for the current state of affairs.


I read in e.g. #117 that you're leaning against using connect/disconnect directly. I think it could be a nice solution for other web frameworks, usually, there's some sort of startup/shutdown procedure. It doesn't seem too unusual to have both either, see encode/databases.

I'm probably just being stubborn/pedantic about it. 😅

In a perfect world, every (web)framework uses context managers and doesn't even expose raw "on enter"/"on exit" callbacks. There are no footguns since you can only use the client this way.
Starlette (that FastAPI builds on) actually supports context managers (https://www.starlette.io/events/) but FastAPI doesn't expose that API yet (see tiangolo/fastapi#2944)

In the current world, few-to-none (web)frameworks use context managers and instead expose raw "on enter"/"on exit" callbacks. So we have footguns since our APIs have to expose separate "on enter"/"on exit" functions and we can't restrict their use.
Example of misuse: Call disconnect in one task while another task still relies on the connection. Or, call connect repeatedly in a background task for a crude-but-well-intentioned workaround to auto-reconnect logic.
To support all possible combinations of connect/disconnect, we have to add extra runtime checks to all functions to avoid misuse. This is what we currently (try) to do and it's to blame for a lot of the internal complexity in asyncio_mqtt.

In any case, I like to think that I can be pragmatic so in the spirit of pragmatism, let's design an API for the current world (and not the perfect world). That puts the strain on us (the asyncio-mqtt developers) to ensure that we have the proper runtime checks for connect/disconnect.

Automating reconnection would be pretty cool, I already considered that as one of the next things to do 👍

Awesome! I look forward to the discussions/PRs. 😄

@frederikaalund
Copy link
Member

I'll try to get back on track with this PR now. 😅

I now also added an example of how to start a listener inside a web framework without blocking the rest of the code. I'm happy about all feedback! 😊

Looks really good! Well done. 👍

In practice, I know that starlette depends on anyio so if it was me, I'd probably use something like:

@contextlib.asynccontextmanager
async def lifespan(app):
    async with anyio.create_task_group() as tg:
        tg.start_soon(listen)  # This is your `listen` method from the "Side by side with async web frameworks" example
        yield

routes = [
    ...
]

app = Starlette(routes=routes, lifespan=lifespan)

To be clear: That's not a "request for change". It's just an alternative example. :) You decide. 👍


As for this PR as a whole it LGTM. It's a great improvement to our documentation and it'll be a welcome resource for new and existing users alike. :)

So if you're happy with it as well, I'll merge right away. Just give your final confirmation.

@empicano
Copy link
Collaborator Author

Wow, thank you for the thoughtful review, I learned a lot!


You could also choose to do a Message.matches [...]

This does look even cleaner, great idea.


Starlette (that FastAPI builds on) actually supports context managers

I didn't know that (or that it's possible like that). Consider my point moot then. I admit I was mostly pushing this for my use case with starlette. The FastAPI PR that you linked looks straightforward, so when that is merged we have an elegant solution for most popular async frameworks, contrary to what I originally thought.

I don't want to storm in and add lots of functionality that later only leads to headaches, either, you're right about wanting to keep it lean. Thanks for the "example of misuse" as well. I'm onboard now with the rejection of connect/disconnect 😄


Last changes:

  • I included the starlette example in the "Sharing a connection" section (using asyncio, I think it's nicer to use the stdlib in the examples, even though I'm going to use anyio for my use case now) and rewrote the "Listening without blocking" one to be without starlette
  • I added the discussion about exposing connect/disconnect, hopefully there are going to be less questions on it now, I saw in other issues that you had to defend this quite often already 😉

If you're ok will all that, I think it's ready to merge! 😊

@frederikaalund frederikaalund merged commit f1ae72f into sbtinstruments:master Oct 16, 2022
@frederikaalund
Copy link
Member

Great! Thank you for your contribution to asyncio-mqtt. 😄👍

I like the section on connect/disconnect vs context managers. Hopefully that'll clear up our (the asyncio-mqtt developers) stance on this topic. If our users really want to, they can always call __aenter__ and __aexit__ directly (which is indeed one of the workarounds in the FastAPI issue thread). :)

I also just released v0.13.0 by the way.

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

Successfully merging this pull request may close these issues.

None yet

2 participants