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

PyZMQ API improvements #172

Merged
merged 21 commits into from Feb 19, 2012
Merged

PyZMQ API improvements #172

merged 21 commits into from Feb 19, 2012

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 4, 2012

Some new APIs and deprecations of old ones that made no sense.

Deprecations

  • Stop using POLLERR, and deprecate on_err in ZMQStream, which never did anything because POLLER can never occur on zmq sockets.

New Features

  • default socket options can be set as attributes on Contexts. This lets you do ctx.linger = 0 to set the initial LINGER of all subsequently created sockets, and the same with all sockopts.
  • ZMQStream.on_recv_stream/on_send_send callbacks, which get the stream as the first argument, in addition to those that were previously passed. Makes it easier to use a single function as the callback for multiple sockets.

Changes

  • zmq.Message renamed to zmq.Frame, to better match high-level binding naming conventions. zmq.Message name remains for backwards compatibility.

Cleanup

  • Removed some #! lines from module files that are not scripts

Message kept as backwards-compatible alias
Setting `ctx.linger = 5` results in sockets subsequently created by ctx having `LINGER=5`.
removes on_err / errback from ZMQStream, which can never be triggered

POLLERR is exclusively for *non-zmq* FDs used with zmq_poll,
and can never occur with a zmq socket according to `man zmq_poll`.
These methods add the stream to the arguments received by callbacks.
@guidog
Copy link
Contributor

guidog commented Feb 4, 2012

Howdy!

Is it possible to rename the the on_* callback registration functions to on_[recv|send]_stream?
(or something like that)
The plain on_*2 doesn't say much.
Why is the old behaviour deprecated?

Cheers
Guido

@minrk
Copy link
Member Author

minrk commented Feb 4, 2012

I guess there's really no need to deprecate the old way, as they are different methods. See #166 for discussion of the new names. I'm not particularly happy with any name we've found yet (including on_recv_stream), so further recommendations are welcome.

@minrk
Copy link
Member Author

minrk commented Feb 5, 2012

old on_recv/on_send no longer marked as deprecated.

@guidog
Copy link
Contributor

guidog commented Feb 6, 2012

Hi!

Cool, thanks.
The naming discussion... sigh

Cheers
Guido

@ellisonbg
Copy link
Contributor

Comments:

  • I agree that we should not deprecate on_recv/on_send.
  • In terms of naming the new functions, I am 50/50 on the on_recv2 versus on_recv_stream.
  • Not sure how I feel about the Message->Frame renaming. Are the other language bindings going in this direction strongly enough that it is worth it?

@ellisonbg
Copy link
Contributor

I like being able to set socket options in the context.

On the Frame/Message renaming. I think of Framing as being a lower-level construct (down towards the wire protocol) than a Message (which is the application level construct). I think the _best_way of distinguishing messages and multipart messages is the following terminology:

  • Message - the single message version
  • Message Group - A group a Messages that are send/recv'd together.

But we should probably follow what other language bindings and the core libs are doing.

@minrk
Copy link
Member Author

minrk commented Feb 6, 2012

On the Frame/Message renaming. I think of Framing as being a lower-level construct (down towards the wire protocol) than a Message (which is the application level construct). I think the _best_way of distinguishing messages and multipart messages is the following terminology:

  • Message - the single message version
  • Message Group - A group a Messages that are send/recv'd together.

This is simply inaccurate - Message Group would suggest that it is multiple messages somehow associated, rather than a single message with multiple segments. A Message is a single zeromq communication, which may have one or more parts. That's why it's confusing that pyzmq's Message class does not reflect a message, but a part of a message. It is already the convention of other bindings and the Guide that these individual message parts are referred to as Frames.

@ellisonbg
Copy link
Contributor

I don't think it is entirely inaccurate - but I am definitely not proposing we adopt the Message/Message Group terminology and I definitely don't feel strongly about this - let's pretend I didn't bring that up...

If other bindings and the Guide are referring to them as Frames, we should definitely follow that. I do think we should leave Message there like you have done for backwards compat though.

@minrk
Copy link
Member Author

minrk commented Feb 6, 2012

The concern about Frame you brought up regarding low-level terminology is valid, and has been brought up elsewhere. I started a discussion on zeromq-dev about this sort of naming convention in bindings, but it wandered off track without much useful information. The only point that does seem well established is that ZeroMQ has multipart messages (i.e. Message refers to the collection), and it doesn't make sense to call the parts of a multipart-message 'messages' themselves. Various names have been proposed by folks who don't like Frame (MessagePart, MessageFragment, etc.), but the only name actually in use anywhere is Frame (aka ZFrame, depending).

@ellisonbg
Copy link
Contributor

I was following your thread until it drifted off topic. Let's just go with Frame (keeping Message for backwards compat.).

@minrk
Copy link
Member Author

minrk commented Feb 6, 2012

Okay, sounds good. I certainly don't want to break any existing APIs with these changes, only add new ones (even for those meant as replacements). I'm using the IPython test suite to confirm backwards compatibility, as it's the heaviest user of the zero-copy and ZMQStream interfaces I know of.

On the Context default options, there are two questions:

  1. Should they be attribute-only (ctx.hwm = 0) or method-only (ctx.setsockopt(zmq.HWM, 0))? I don't think there's a need to have both, unlike sockets. Another alternative is to just expose the ctx.sockopts dict as the only interface, so it would be ctx.sockopts[zmq.HWM] = 0.

  2. What should happen on a failed setsockopt? Some socket options do not apply to all sockets.

    • raise the error, destroying the socket?
    • silently ignore the error, assuming it's a sockopt that doesn't apply to the given socket type.

    I've handled SUBSCRIBE as the most common one of these, but not any others.

@ellisonbg
Copy link
Contributor

On (1) I don't have a strong feeling. Seems like the ctx.sockops dict provides a small amount of encapsulation that is nice, but the API is not quite as simple as the attribute-only. The attribute and method approaches more closely mirror the API of Socket itself, which is most consistent. In the end, I would probably pick either the attribute or method API - which ever of the two is most commonly used by Socket users. For me, I would probably use the attribute style.

On (2), I am tempted to silently ignore the error, as there are some options that don't apply globally. While I am a bit hesitant to silently ignore errors, in this case, I think it is needed for this API to be useful.

@minrk
Copy link
Member Author

minrk commented Feb 9, 2012

on_{recv|send}_stream added, now as simple wrappers of on_{recv|send}, rather than as replacements.

@ellisonbg
Copy link
Contributor

This part looks good, what did you decide on the Context socket options API?

@minrk
Copy link
Member Author

minrk commented Feb 9, 2012

Another proposed change, with an eye towards cross-language consistency:

  • rename *_unicode to *_string (keeping _unicode for compatibility). Unicode is not only Python-specific as a name for native strings, but specific to Python 2, and no longer makes sense in Python 3.

@ellisonbg
Copy link
Contributor

I think this is a good idea as well.

@minrk
Copy link
Member Author

minrk commented Feb 9, 2012

I went with your suggestion on the Context - attributes only, errors when assigning to new sockets silently ignored (assumes things like SUBSCRIBE on non-SUB).

`_unicode` kept as alias
@ellisonbg
Copy link
Contributor

OK great, I think this PR is getting close to being merge ready - once
you implement the *_strong methods, I would say go for it.

On Thu, Feb 9, 2012 at 12:40 PM, Min RK
reply@reply.github.com
wrote:

I went with your suggestion on the Context - attributes only, errors when assigning to new sockets silently ignored (assumes things like SUBSCRIBE on non-SUB).


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

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member Author

minrk commented Feb 9, 2012

Sure - I'll do a pass on the docs before I merge.

Frame.more is bool, as opposed to flags, which was a bitmask

Frame.flags solves a problem that doesn't exist, because there is only one flag at this point.
which used copy=False by default.

It doesn't make sense to use different defaults than Socket.send,
and non-copying sends can cause problems on shutdown without
manually terminating the context in Python 3.

ref: zeromq#173
@minrk
Copy link
Member Author

minrk commented Feb 18, 2012

Further small changes:

  • Frame.flags is now Frame.more, and is a bool rather than a bitfield. There is only one flag right now, and it doesn't make a lot of sense to support flags that don't exist yet (despite having existed in the past in 3.0 and 4.0).
  • default ZMQStream.send() to copy=True instead of copy=False. It doesn't make sense for its defaults to be different, and can actually cause problems in some edge cases (see libzmq assertion at exit under Python 3 #173).

One last thing I am considering before finishing this up: Full removal of the prefix arg on send_multipart. This was added to support ZMQ_LABEL functionality, which has since been removed. Right now, all it does is prepend a second the prefix frames to the message. Perhaps it is more sensible to wait until the feature is reintroduced before restoring the argument, because it is not guaranteed that the prefix arg will even make sense when it comes back. I obviously wouldn't remove it if anyone is using it, but I seriously doubt that this is the case.

@ellisonbg
Copy link
Contributor

I think using copy=True as the default for streams makes sense as does removing the prefix arg for now.

@minrk
Copy link
Member Author

minrk commented Feb 19, 2012

Okay, changes made, and docs updated.

minrk added a commit that referenced this pull request Feb 19, 2012
PyZMQ API improvements

* on_recv_stream
* scrub mistaken use of POLLERR
* scrub unused prefix
* ctx.sockopts
* get_includes() fixes
* Message -> Frame
* _unicode -> _string

closes #178
@minrk minrk merged commit cadd6d9 into zeromq:master Feb 19, 2012
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

3 participants