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

Disable output buffering by default #595

Closed
jacobperron opened this issue Feb 2, 2021 · 5 comments · Fixed by #659
Closed

Disable output buffering by default #595

jacobperron opened this issue Feb 2, 2021 · 5 comments · Fixed by #659
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jacobperron
Copy link
Member

jacobperron commented Feb 2, 2021

Feature request

Feature description

The current default of Python buffering output can lead to undesirable behavior when working with ros2cli tools. For example,

ros2 echo /chatter | grep "Hello" 

won't output messages right away unless I disable buffering with PYTHONUNBUFFERED=1.

I've created this ticket to discuss if we should default to disabling output buffering. Though we've had this discussion related to other features/tools in ROS 2, I couldn't find much about the CLI tools (just this: #588).

Implementation considerations

Cons:

Disabling output buffering can lead to performance issues. But, I'd argue that it would be acceptable presuming these tools are primarily used for debugging, and they are not very performant as-is.

Pros:

Users won't get confused when they rely on output of ros2cli tools not immediately being fed into another process.
E.g. tests won't have to explicitly set PYTHONUNBUFFERED.

@jacobperron jacobperron added the enhancement New feature or request label Feb 2, 2021
@fujitatomoya
Copy link
Collaborator

if we should default to disabling output buffering.

+1 for disabling output buffer default. and we can guide user how to enable buffer it if user wants. i think that could be more friendly for user.

@clalancette
Copy link
Contributor

I'm OK with changing the default behavior of the CLI to be unbuffered by default. I just want to make sure that:

  1. All of the CLI tools are consistent, and
  2. That there is a way to go back to buffered mode if the user wants

@clalancette clalancette added the help wanted Extra attention is needed label Feb 18, 2021
@ivanpauno ivanpauno self-assigned this Aug 9, 2021
@ivanpauno
Copy link
Member

We could use stderr for logging which is always unbuffered, though that might be confusing.
I think for commands that return "immediately", using buffered output is fine (e.g. ros2 topic list), but for things like echo it makes sense to have unbuffered ouput.

We can also use the flush kwarg of print() in those cases: https://docs.python.org/3/library/functions.html#print.
I think I'm going to use that approach.

@clalancette
Copy link
Contributor

We can also use the flush kwarg of print() in those cases: https://docs.python.org/3/library/functions.html#print.
I think I'm going to use that approach.

I'm kind of against doing it this way. I think that for any new code, it is going to be really easy to forget to do it (and for the reviewer to notice that it is missing).

What if we instead set PYTHONUNBUFFERED=1 at startup by default, and then let the user disable it using the same mechanism if they want?

@ivanpauno
Copy link
Member

What if we instead set PYTHONUNBUFFERED=1 at startup by default, and then let the user disable it using the same mechanism if they want?

I don't think you can do that exactly, IIUC PYTHOUNBUFFERED is an environment variable that python checks only at startup.
There're ways to programatically make stdout unbuffered in python, but all the ones that I found are a bit hacky.

I will take a look if I can find an easy way to do it, or apply one of the hacky ways to do it.
I will probably enable line buffering instead of unbuffered output, which is reasonable for this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants