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

open_memory_channel(): return a named tuple #1771

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

belm0
Copy link
Member

@belm0 belm0 commented Oct 26, 2020

partially addresses #719

trio/_channel.py Outdated
Comment on lines 18 to 19
send_channel: "MemorySendChannel"
receive_channel: "MemoryReceiveChannel"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for these qualified names rather than just send and receive to avoid the awkwardness of send() and receive() also being the primary methods:

channel_pair = open_memory_channel()
await channel_pair.send.send()

Comment on lines +1127 to +1132
Assigning the send and receive channels to separate variables usually
produces the most readable code. However, in situations where the pair
is preserved-- such as a collection of memory channels-- prefer named tuple
access (``pair.send_channel``, ``pair.receive_channel``) over indexed access
(``pair[0]``, ``pair[1]``).

Copy link
Member Author

@belm0 belm0 Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please note this point

It's why I didn't go crazy converting all the docs and tests to named tuple access-- tuple destructuring is more readable for local code dealing with a single memory channel.

However the named tuple still wins when you're dealing with many channels and keep the pairs intact.

@codecov
Copy link

codecov bot commented Oct 26, 2020

Codecov Report

Merging #1771 (4f78539) into master (bda5c11) will decrease coverage by 0.07%.
The diff coverage is 67.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
- Coverage   99.18%   99.12%   -0.07%     
==========================================
  Files         115      115              
  Lines       17594    17625      +31     
  Branches     3142     3146       +4     
==========================================
+ Hits        17451    17470      +19     
- Misses         99      111      +12     
  Partials       44       44              
Files Coverage Δ
trio/_tests/test_channel.py 100.00% <100.00%> (ø)
trio/_tests/test_highlevel_serve_listeners.py 100.00% <100.00%> (ø)
trio/_channel.py 94.33% <62.50%> (-5.67%) ⬇️

@oremanj
Copy link
Member

oremanj commented Oct 26, 2020

I like this. @njsmith thoughts?

I often find myself wanting a single object that includes both ends of the channel, like the old trio.Queue. With this PR I can now do that more readily, but it's cumbersome typing send_channel.send() and receive_channel.receive(). You can define additional methods when using typing.NamedTuple, unlike the collections version. How would you feel about adding send() and receive() methods as aliases for send_channel.send() and receive_channel.receive(), plus or minus _nowait?

One step further would make the returned object an AsyncResource whose aclose() closes both channels. I doubt that's necessary in practice though.

@belm0 belm0 force-pushed the memory_channel_named_tuple branch 2 times, most recently from d4a8db0 to 4e7a466 Compare October 26, 2020 03:41
@belm0
Copy link
Member Author

belm0 commented Oct 26, 2020

How would you feel about adding send() and receive() methods as aliases for send_channel.send() and receive_channel.receive(), plus or minus _nowait?

It seems confusing for API users as it's adding another layer (where a pure named tuple really isn't). Now there are multiple ways to do the same thing (channel.send() and channel.send_channel.send()), and we can no longer say simply that open_memory_channel() returns a (named) tuple, but now have to point to a class reference with the methods.

I'd rather accept the memory channel interface as somewhat low level: it does expose the halves, though in many uses that isn't needed. As far as wrapping that in a simpler API having less features, defer to the application code or a utility library.

@belm0 belm0 requested a review from njsmith October 26, 2020 03:52
@belm0
Copy link
Member Author

belm0 commented Oct 26, 2020

As a straw man, here are some of the queue-like methods:

    async def send(self, value):
        await self.send_channel.send(value)

    def send_nowait(self, value):
        self.send_channel.send_nowait(value)

    async def receive(self):
        return await self.receive_channel.receive()

    def receive_nowait(self):
        return self.receive_channel.receive_nowait()

    async def aclose(self):
        await self.send_channel.aclose()
        await self.receive_channel.aclose()

What's notably missing is receive iteration. But adding that as the default iterator (as Queue once did) would conflict with tuple's iterator. And going down this road, I would just like the class to be named TrioQueue and have the constructor open the memory channel to complete the encapsulation and have no reference to channels.

I'm looking at this for our app and/or trio_util, but it still seems fine as something on top of the memory channel API.

@njsmith
Copy link
Member

njsmith commented Oct 26, 2020

Another vague plan for memory channels is that I feel like they should return a pair of bidirectional channels, so you can send messages both ways. It would dramatically reduce the bookkeeping you need in that case now to juggle 4 different handles, and if you're doing unidirectional communication then it doesn't affect you at all – you can just ignore the extra methods.

Also, I feel like it will just be easier to read code where the channels are named based on the endpoint, rather than based on directionality.

But, if we make memory channels symmetric like that, then it conflicts with the .send_channel/.receive_channel names proposed in this pr.

Another option to throw into the mix: StapledChannel(*open_memory_channel(...)). How does that compare to the namedtuple, for your use cases?

@belm0
Copy link
Member Author

belm0 commented Oct 26, 2020

So what is StapledChannel? Is it just a named tuple, or does it have the send/receive convenience methods (#1771 (comment)), receive async iter, and aclose?

@njsmith
Copy link
Member

njsmith commented Oct 26, 2020

It would be the equivalent of trio.StapledStream, but for channels. So yeah, it would have all the channel methods. And when applied to a pair of memory channels, it'd basically be a "loopback" channel, where everything you send in comes right back out again.

@belm0
Copy link
Member Author

belm0 commented Oct 27, 2020

It would be the equivalent of trio.StapledStream, but for channels. So yeah, it would have all the channel methods.

Would it be OK for the instance __aiter__ to iterate receive_channel items, as Queue did?

And when applied to a pair of memory channels, it'd basically be a "loopback" channel, where everything you send in comes right back out again.

What does the construction look like? StapledChannel(open_memory_channel(...), open_memory_channel(...))

what are the additional attributes and methods in that case?

@belm0
Copy link
Member Author

belm0 commented Oct 27, 2020

But, if we make memory channels symmetric like that, then it conflicts with the .send_channel/.receive_channel names proposed in this pr.

Doesn't StapledStream use .send_stream/.receive_stream?

@belm0
Copy link
Member Author

belm0 commented Oct 30, 2020

Also, I feel like it will just be easier to read code where the channels are named based on the endpoint, rather than based on directionality.

But, if we make memory channels symmetric like that, then it conflicts with the .send_channel/.receive_channel names proposed in this pr.

open_memory_channel() is returning a Tuple[MemorySendChannel, MemoryReceiveChannel]. So it's tenuous to argue that send_channel and receive_channel aren't appropriate names for the corresponding items.

In the bidirectional channels case, isn't there going to be an object wrapping two memory channels (or one side of the two channels)? That would be a potential place to introduce alternate naming.

@njsmith
Copy link
Member

njsmith commented Oct 30, 2020

Would it be OK for the instance __aiter__ to iterate receive_channel items, as Queue did?

It would be a Channel, and that's part of the Channel interface, so yes.

What does the construction look like? StapledChannel(open_memory_channel(...), open_memory_channel(...))

StapledChannel(send_channel, receive_channel) in general. So for the specific case of making a loopback memory channel, it would be StapledChannel(*open_memory_channel(...)).

open_memory_channel() is returning a Tuple[MemorySendChannel, MemoryReceiveChannel]. So it's tenuous to argue that send_channel and receive_channel aren't appropriate names for the corresponding items.

Yeah, that's how it works currently, but I'm talking about a potential future where it returns Tuple[MemoryChannel, MemoryChannel].

@belm0
Copy link
Member Author

belm0 commented Nov 1, 2020

attempted StapledMemoryChannel in #1784

in my case I'm needing the _nowait variants, which aren't currently represented by any interface

@pquentin
Copy link
Member

pquentin commented Nov 2, 2020

(Merged master into this branch to get the new Travis check, all checks pass now)

@TeamSpen210
Copy link
Contributor

Hmm that's an issue. The named tuple needs to be generic, but that only works in 3.11+, unless you import typing_extensions at runtime. Manually defining a tuple subclass works, but it's a bunch of code.

@A5rocks
Copy link
Contributor

A5rocks commented Sep 8, 2023

Hmm that's an issue. The named tuple needs to be generic, but that only works in 3.11+, unless you import typing_extensions at runtime. Manually defining a tuple subclass works, but it's a bunch of code.

Can we do the trick we did elsewhere where we only do [...] bit in typing.TYPE_CHECKING? It's awful for runtime-friendliness... but we haven't cared about that much so far.

@TeamSpen210
Copy link
Contributor

In this case it doesn't really work, because users need to be able to subscript the class for their own type hints.

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.

7 participants