rospy: make multi threaded publishing default in indigo (#169)

[tfoote]

We kept the legacy behavior in #169 fix as default for stability.

For indigo we should make the new behavior default.

[dirk-thomas]

I am absolutely not sure about the default behavior in Indigo. We should discuss this before going ahead (@wjwwood fyi).

In Hydro an optional keyword argument can be used to specify the queue size:

• N being the number of messages to queue
• 0 for unlimited
• None being the default value making the call synchonous / non-threaded / non-queued

I don't think that it is a good idea to make the new default value 0. That would lead to unlimited memory usage by default when a connection (e.g. via WLAN) drops out. That is also not what we do in C++ - in roscpp the user must specify the queue size explicitly.

I think we should not:

• require an argument to be passes since that would break every existing Python node
• default to 0 since that is a pretty bad default value

Imo the options are only:

• keep it as-is so the user must actively pass the keyword argument if asynchronous behavior is desired
• choose a default value N being something reasonable (e.g. between 1 and 100).

[mikeferguson]

@dirk-thomas Does roscpp have a default N then? Ideally rospy should mirror roscpp here (as it's also what people expect). I concur that we should not require an argument to be passed, and 0 also seems pretty bad.

I do not think we should keep it as-is, because quite frankly, this deficiency in rospy has been causing trouble for years (most people have simply considered the higher level software to be flaky, but as soon as this deficiency is explained to them, things tend to be realized to have been rospy locking up trying to publish to a lost subscriber).

[dirk-thomas]

@mikeferguson No, roscpp does not have a default N. The roscpp API requires the user to pass in an explicit queue size: e.g. https://github.com/ros/ros_comm/blob/hydro-devel/clients/roscpp/include/ros/node_handle.h#L236 https://github.com/ros/ros_comm/blob/hydro-devel/clients/roscpp/include/ros/advertise_options.h#L58 I think that is the "right" way to do it but for rospy we can't enforce that now since the API did not require it until now.

So the question remains: do we change the default value for it (and to what value) or do we keep it as-is.

[wjwwood]

We could leave the current default, but print a warning notifying people that they should explicitly pass the argument. So existing code will continue to work as it currently does, but will get a message like this:

rospy.Publisher() called without explicitly specifying the queue size.
This behavior is deprecated and you should explicitly use a queue_size keyword argument.

See: http://wiki.ros.org/rospy/Overview/Publishers%20and%20Subscribers#queue_size

This option will be removed in J-Turtle.

Called from /path/to/source/module.py:1234

Or something like that. And then in J-Turtle we can require that the queue_size is passed to the constructor, or we can make a generator function which takes the arguments in a more convenient manner, something like this as a shim:

def create_publisher(name, data_class, queue_size, **kwargs):
    return rospy.Publisher(name, data_class, queue_size=queue_size, **kwargs)

[dirk-thomas]

While I see the advantage of having the user specify the queue size I am not convinced that printing a warning for every existing Python publisher is a good idea and especially requiring everybody for J-Turtle to change their code. Realistically it has worked for most of the users until now since the common use case is local communication.

We could also emphasize this in the documentation and tutorials and mention it in the migration guide for indigo but not change any code/API at all. Perhaps getting some more user feedback might be a good idea before making a decision.

[wjwwood]

If you believe that there is not good default value and/or that changing the current default is not a good idea, then I think there should be a warning. We can have a mechanism for suppressing the warning for people running code which they do not control, and I am ok with never removing the behavior, even in J-Turtle or later. I do not believe putting it in the migration guide and the tutorials will be sufficient to get people to change existing code. It really comes down to two questions:

• Is it important for people to change existing code to specify a queue_size explicitly?
• Is it sufficient to update the tutorials and documentation or do we need more visible notices?

I would say it is important to get people to change the existing code to prevent users from running into hard to debug problems and that the only people who will notice the documentation changes will be people who run into the problem and new users, so I think a more visible warning is needed.

[wjwwood]

I agree that we should get broader feedback on this before making a decision.

[dirk-thomas]

Could you please describe a bit more how you imagine to implement "a mechanism for suppressing the warning for people running code which they do not control"?

[wjwwood]

There are plenty of ways to suppress it globally, like an environment variable, global configuration, or via a python global setting in a pythonrc file. If we use the python warnings module then the user can setup a warning filter, or we can setup a warning filter based on some user configuration.

I don't really see that as being as important as deciding what behavior we want.

[wjwwood]

I did not mean to say selectively disable the warning, just everywhere or no where. But that way they can disable the warnings once they have fixed their code, but not the code they don't control the release of.

[jonbinney]

I like the idea of having a warning in indigo if queue_size isn't supplied, and requiring the argument in indigo. Since this is a relatively small fix, it might even be possible to provide a simple script that finds/replaces these in a repo and works most of the time.

Even if that isn't done, it's probably worth changing all rospy tutorials to include queue_size in example code. New users tend to copy/paste those examples, so they should reflect good practices: http://wiki.ros.org/rospy_tutorials/Tutorials/WritingPublisherSubscriber

[jonbinney]

Oops, mean to say that I like the idea of a warning in indigo, and requiring the argument in j-turtle.

[jonbinney]

Here's a script that recursively goes through a directory and fixes python files to use queue_size when creating publishers: https://gist.github.com/jonbinney/9299095

It takes the root directory as an argument, and the queue_size as an optional arg (defaults to 10). I've tested it on python2.7, and it seems to do the right thing (I tried it on the ros_comm_repo itself).

[dirk-thomas]

The PR #372 implements the proposed warning. Please comment on that PR if you have an opinion on this (either you think it is a good or a bad idea).

[dirk-thomas]

I asked for broader feedback on ros-users: http://lists.ros.org/lurker/message/20140306.015824.7333cfaf.en.html Waiting for feedback...

[dirk-thomas]

Based in the discussion the warning was added.