ZMQStream.on_recv/on_send should pass stream to callbacks

[minrk]

Currently, the on_recv/on_send system only passes the message to the callback, but they should really pass both the stream and the message. This is the way the czmq reactor behaves, and it makes it easier to use the same callback with multiple streams.

The only tricky thing here is how to add this in a sensible way that does not break current apps that only expect the one argument.

For now, this behavior can be emulated with closures and intermediary functions:

def handle_recv(stream, msg):
    dostuff()

for s in streams:
    s.on_recv(lambda msg: handle_recv(s,msg))

But this is suboptimal.

[ellisonbg]

What about implementing new callbacks with different names? Something like on_recv_wstream/on_send_wstream. The ZMQStream object could look to see which pair of callbacks is registered and call those only. That allows existing apps to keep using the on_recv/on_send ones, and migrate when they are ready.

[minrk]

A new method is probably the best approach, though a clearly temporary name like 'on_recv_wstream' would not be ideal.

[minrk]

Another thing that's different about the czmq reactor (and one of the reasons it passes stream) is that it does not call recv itself, but rather passes stream, event_mask, and application code calls recv as appropriate. If we did it this way, the obvious name would be on_pollin.

[ellisonbg]

That makes the callback a little lower level though and forces the
user to make sure that recv is called the correct number of times.
But maybe a lower level API makes sense? Just to clarify, there isn't
any problem with passing the received messages to the callback though
right? I guess I have a slight inclination to continue to pass the
messages to the callback.

On Fri, Jan 20, 2012 at 11:29 AM, Min RK
[email protected]
wrote:

Another thing that's different about the czmq reactor (and one of the reasons it passes stream) is that it does not call recv itself, but rather passes stream, event_mask, and application code calls recv as appropriate.  If we did it this way, the obvious name would be on_pollin.

---

Reply to this email directly or view it on GitHub:
#166 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
[email protected] and [email protected]

[minrk]

Correct. I have encountered one or two cases where things would have been a bit simpler if I could call recv myself, rather than having the message, but I don't think that's an actual problem. This would make zmqstream more similar to tornado's own IOStream class (i.e. just use the add_handler level of code, rather than the higher level on_recv, etc.)

[ellisonbg]

Yes, it would make it more like the IOStream class if we required the
user to call recv. Which route do you think is better?

On Fri, Jan 20, 2012 at 11:43 AM, Min RK
[email protected]
wrote:

Correct.  I have encountered one or two cases where things would have been a bit simpler if I could call recv myself, rather than having the message, but I don't think that's an actual problem.  This would make zmqstream more similar to tornado's own IOStream class (i.e. just use the add_handler level of code, rather than the higher level on_recv, etc.)

---

Reply to this email directly or view it on GitHub:
#166 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
[email protected] and [email protected]

[minrk]

What I really want is on_recv to pass two arguments instead of one, with no other changes, and the most graceful path ahead for that. The rest is merely presenting further alternatives that could be considered, but are not actually what I am aiming for. It's possible that simply on_recv2 is the better new name, since it pass two arguments and is 'new'. on_send2 makes slightly less sense, as it already gets two arguments, and will get three.

Users who want a low-level mode can simply use Sockets with IOLoop.add_handler, skipping ZMQStream entirely, which will work just fine.

[ellisonbg]

OK, I like the idea of having the new methods get the stream and the
messages (ZMQStream would call recv for the user). As far as the
names for the new methods, I actually like on_recv2/on_send2, not
because they take 2 arguments but because they are the 2nd version of
those methods. I agree that the lower level API is already provided
by IOLoop.add_handler.

On Fri, Jan 20, 2012 at 2:59 PM, Min RK
[email protected]
wrote:

What I really want is on_recv to pass two arguments instead of one, with no other changes, and the most graceful path ahead for that.  The rest is merely presenting further alternatives that could be considered, but are not actually what I am aiming for.  It's possible that simply on_recv2 is the better new name, since it pass two arguments and is 'new'.  on_send2 makes slightly less sense, as it already gets two arguments, and will get three.

Users who want a low-level mode can simply use Sockets with IOLoop.add_handler, skipping ZMQStream entirely, which will work just fine.

---

Reply to this email directly or view it on GitHub:
#166 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
[email protected] and [email protected]

[guidog]

Hi!

The plain 2 at the end doesn't have any meaning, as shown in the arguments so far.
I would really like to see something that gives a hint where the difference to the normal (yes, normal)
registration methods is.

There are methods with longer names and they don't hurt, like recv_multipart etc.

Cheers
Guido

[minrk]

Hi!

The plain 2 at the end doesn't have any meaning, as shown in the arguments so far.
I would really like to see something that gives a hint where the difference to the normal (yes, normal)
registration methods is.

The old (no, not normal) on_recv methods gave no information, so the new descriptive name is only informative if you already understand on_recv. The reason why I dislike adding a qualifier to the name is that the new methods are meant to replace the old ones, as a year and a half of writing apps with them has proven to me that this is how they should have been written in the first place. The only reason they are separate is so that I don't break existing apps while users upgrade. That is less likely to take hold if the new name suggests that it is a bonus version, rather than the replacement that it is.

That said, maybe the best approach is to let it be, and use a more descriptive name, leaving the original bad decision in forever.

There are methods with longer names and they don't hurt, like recv_multipart etc.

Cheers
Guido

---

Reply to this email directly or view it on GitHub:
#166 (comment)

[guidog]

Hi from frozen Germany!

Hmmm... given the time pyzmq is in use and the number of users I've seen (irc mostly) I'd say:
You'll have a bad time changing the API.

A new (better) way using new methods will be the only way I guess.
It's the same as with the core libs.
In general: If an API is in use, forget changing it.

Anyway, the new way is of course better and does make some things much easier.
Will port python-mdp soon. :)

Cheers
Guido

[ellisonbg]

@guidog: how are you using PyZMQ in python-mdp?

[guidog]

@ellisonbg They way it should be used :)

I'm using the ZMQStream in the broker. Made the whole thing a lot easier.
Also using PeriodicCallback for the heartbeat etc.

I wanted to have a different set of examples using the more advanced features of pyzmq.
The examples in The Guide are mostly translations of the C version.

Hope this answers your question.

[minrk]

Hmmm... given the time pyzmq is in use and the number of users I've seen (irc mostly) I'd say:
You'll have a bad time changing the API.
A new (better) way using new methods will be the only way I guess.
It's the same as with the core libs. In general: If an API is in use, forget changing it.

Yes, I have no intention of removing the existing (bad) API in the foreseeable future for this reason. But I don't want the new API to appear as if it's sugar wrapped around the old API. Docs, examples, etc. should never mention the old API, and it should never occur to new users to use it. That's why I don't like the idea of writing docs with Stream.on_recv_stream all over the place, because it would mean that the primary API is a redundant name, referring to itself as a special case of something never mentioned elsewhere.

An alternative would be to continue to use the same on_recv name, but switch to the new behavior with a from zmq.__future__ import on_recv_stream style import.

[guidog]

Hi!

Please try to see this from the point of a user.
You can't make the old API vanish, neither from the code nor the minds of the people.

See, on IRC there are still questions on where the device programs are.
They disappeared long ago, but the memory is still alive.

The only way to get a migration going is to provide the alternative and explain what the benefits are.
Then hope for the best.

You can't -- and shouldn't -- force it. This builds opposition and scares away people.

If you write the reason for having the new methods into the docs, then I assume every
reasonable programmer will see that the methods are different.
And that they are not bloat, but came out of usage of the library.
What could be a better reason to use the new API?

Cheers
Guido

[minrk]

Fair point about about the explanation, I will certainly do that. Though the old methods should still not be mentioned in any place other than that explanation of why the new names are not what they should be. I don't want to pretend that these new methods mean there are two ways of doing things. These are the exact same methods with a single extra argument that should have always been there in the first place, and the old methods only exist for backwards compatibility (which is important).

[minrk]

I think I'm just going to have to give up on my desire to "fix" the API. Since it really can't be done in a backward-compatible way, I'm not sure it's useful.

What should have always been the case is that on_recv callbacks are passed the stream itself. Since this wasn't done, there is simply no path forward that fixes this mistake that doesn't break APIs. Adding a second on_recv_stream method has little to no value if it isn't going to be a replacement for the bad choice in the past. A method called on_recv_anything will always be viewed as a special case of on_recv, regardless of the truth of the matter, so honestly calling the fixed API anything but on_recv simply misses the point. on_recv2 was meant as a compromise, suggesting that 'this is what on_recv should be', but still doesn't seem good enough.

Users who want on_recv to behave as it always should have can simply get used to the lambda+closure trick:

stream.on_recv(lambda msg: callback(stream, msg))

[ellisonbg]

I agree that we should have passed the stream from the start, but give
yourself some credit. The ZMQStream API was created very early on,
before anyone else had done anything even close to this. It is very
hard to guess in advance how things will be used. That the API has
remained as stable as it has is amazing.

I am a little more willing to add the new functions under some
reasonable name. Some people will find them useful and they do serve
a purpose that the lambda/closure trick does not: their presence would
encourage people to think about how they can write their code to pass
streams around like this. The lambda/closure trick is also a bit
subtle and many people would resort to other ways of passing the
stream around, such as storing it in a global location, which is
definitely not good.

Go ahead and make the final decision on this one, but I am +1 on
adding them under a new name.

On Tue, Feb 7, 2012 at 2:59 PM, Min RK
[email protected]
wrote:

I think I'm just going to have to give up on my desire to "fix" the API.  Since it really can't be done in a backward-compatible way, I'm not sure it's useful.

What should have always been the case is that on_recv callbacks are passed the stream itself.  Since this wasn't done, there is simply no path forward that fixes this mistake that doesn't break APIs.  Adding a second on_recv_stream method has little to no value if it isn't going to be a replacement for the bad choice in the past. A method called on_recv_anything will always be viewed as a special case of on_recv, regardless of the truth of the matter, so honestly calling the fixed API anything but on_recv simply misses the point.  on_recv2 was meant as a compromise, suggesting that 'this is what on_recv should be', but still doesn't seem good enough.

Users who want on_recv to behave as it always should have can simply get used to the lambda+closure trick:

   stream.on_recv(lambda msg: callback(stream, msg))

---

Reply to this email directly or view it on GitHub:
#166 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
[email protected] and [email protected]

[guidog]

Hi!

ellisonbg is right (imho), both +1 (the credits and the new function).

Cheers
Guido