truncate arrays, bytes, and strings by default, add option to show in full or use custom threshold #31

dirk-thomas · 2017-06-28T17:43:20Z

This will allow to print every image message with: ros2 topic echo /image

clalancette

Seems to work in my testing.

dirk-thomas · 2017-06-28T18:06:01Z

My rational for using truncation by default is the following:

For large messages I want "non-broken" behavior by default, if long messages are printed in full by default that basically prevents all messages to be outputted.
For small message I want to see all array values, and all character in bytes and strings.
ROS 1 has options to truncate arrays / strings (actually to show nothing but the length). Since these options have been added later in order to not previous behavior they don't achieve the goal of the first bullet. Therefore I don't think we should mimic ROS 1 behavior here.

Additional improvements could be:

Show the number of truncated values / characters or the full length for information purposes.
Allow to pass a custom threshold for the truncation.

mikaelarguedas · 2017-06-28T18:30:48Z

I'm not sure if the default behavior should be "not show the entire message". While making sense for large messages it seems odd that if I ask to print the content of a message I get only part of it.
I like the --no-arr option in ROS1 allowing to print messages without printing the arrays. It doesnt allow you to know that the content of your image changes in that case
rostopic hz is another useful way of knowing of often you are receiveing messages.
An alternative could be to print a warning before the message if the size exceed an arbitrary threshold. This way users would be warned that displaying it would screw up their performance but not making an arbitrary decision on the size of the message they can display.

Maybe if you can clarify what is the use case we try to solve with this approach we can discuss alternatives and decide which one is the best suited default behaviour

mikaelarguedas · 2017-06-28T18:38:25Z

Well I guess I should have sent this message when I wrote it after the offline discussion 😖 and not 40 min late... thanks for providing a rationale.

I see your reasoning, I agree that defaulting to something having bad performances is not ideal. I think that if this is to be merged we should at least allow users to chose the threshold.

I'd like to have more input from @ros2/team before merging this because that's a change from ROS1 behavior (and general expected behavior) that should be discussed and considered carefully before imposing it to users. Then I agree we can iterate on it later so it could be rolled out as is in beta2 and modified later.

dirk-thomas · 2017-06-28T19:15:58Z

we should at least allow users to chose the threshold.

I will add an additional option for this. I guess one threshold for all three types is sufficient?

I'd like to have more input from ros2/team before merging this because that's a change from ROS1 behavior (and general expected behavior) that should be discussed and considered carefully before imposing it to users.

Before discussing what we should do I would like to have a consensus on the goals. Do we agree about the two goals described in the first two bullets (non-broken for "large" msgs by default, see all values for "small" msgs by default)? If each of you could answer that question first that would be helpful.

mikaelarguedas · 2017-06-28T19:35:36Z

I will add an additional option for this. I guess one threshold for all three types is sufficient?

Yes I think that's sufficient

non-broken for "large" msgs, see all values for "small" msgs

Yeah I think that's a good goal to have if we agree that someone calling ros2 topic echo should not expect a full message if it's a long one.

Looking at the patch, this truncature also applies to fixed length arrays and not only the dynamic ones right ?

dirk-thomas · 2017-06-28T19:38:55Z

Looking at the patch, this truncature also applies to fixed length arrays and not only the dynamic ones right?

Yes, good point. I will rename the constant to TRUNCATE_LENGTH.

clalancette · 2017-06-28T19:47:01Z

My rational for using truncation by default is the following:

For large messages I want "non-broken" behavior by default, if long messages are printed in full by default that basically prevents all messages to be outputted.

Yeah, I definitely agree with this. Otherwise the user has no idea what is wrong with their invocation of echo.

For small message I want to see all array values, and all character in bytes and strings.

Seems sane to me.

ROS 1 has options to truncate arrays / strings (actually to show nothing but the length). Since these options have been added later in order to not previous behavior they don't achieve the goal of the first bullet. Therefore I don't think we should mimic ROS 1 behavior here.

I can see that option being useful here as well, but I'm happy to defer that until later.

… full or use custom threshold

wjwwood · 2017-06-28T20:20:34Z

I'm ok with having automatic truncating (or automatic summarizing, however you want to call it). +1 for the proposed changes.

Though I'm dubious about the rationale that large messages are broken by default in this case. Whether or not it can print all the messages depends on the speed of your computer and the size of message. Not whether or not it is truncated. Presumably a large enough message (or high enough frequency messages) would result in not every message being printed even with truncating on.

I like the proposed changes because it's a better user experience, i.e. the summary is more useful than the raw data in most cases, imo.

dirk-thomas · 2017-06-28T20:21:12Z

I updated the patch. It has an option to pass a custom threshold (which can also be zero).

Ready for re-review.

dirk-thomas · 2017-06-28T20:22:18Z

I could remove --full-length and use any negative value for --truncate-length to select that behavior?

wjwwood

lgtm, though I was originally assuming that this would summarize the array's (a la ros/ros_comm#724), but the truncating is ok for now I guess.

wjwwood · 2017-06-28T20:22:41Z

ros2topic/ros2topic/verb/echo.py

@@ -41,6 +54,15 @@ def add_arguments(self, parser, cli_name):
            '--csv', action='store_true',
            help='Output all recursive fields separated by commas (e.g. for '
                 'plotting)')
+        parser.add_argument(
+            '--full-length', action='store_true',


I think it would be good to have a short flag for this, maybe -f or -a. @ros2/team what does everyone else think?

Yeah, I also think it would be good to have short options.

Done: 0777ef2

wjwwood · 2017-06-28T20:23:54Z

ros2topic/ros2topic/verb/echo.py

+                 "length > '--truncate-length', by default they are truncated "
+                 "after '--truncate-length' elements with '...''")
+        parser.add_argument(
+            '--truncate-length', type=unsigned_int, default=DEFAULT_TRUNCATE_LENGTH,


Same here, maybe -l.

Done: 0777ef2

wjwwood · 2017-06-28T20:29:14Z

Personally I like the explicit --full-length argument. It is more visible/obvious when someone reads it (imagine it's in a tutorial). With the implicit full length on -1 you have to assume or read the docs to understand it. You could also do both, but I don't see a reason for it.

clalancette · 2017-06-28T20:30:09Z

I could remove --full-length and use any negative value for --truncate-length to select that behavior?

I'd be OK with that, though if you did that, I would suggest changing the name of the flag to "max-print-length" or something like that.

dirk-thomas · 2017-06-28T21:14:40Z

I will go ahead soon and merge this as-is if there are no further comments.

mikaelarguedas · 2017-06-28T21:16:05Z

that sounds good to me we can iterate on a summarize option or an alternative after the release

dirk-thomas · 2017-06-28T23:39:05Z

Updated test accordingly in 1156c0d.

* parameter values get evaluated by yaml rules Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Use new variable type syntax Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * type_ -> last_subtype Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

dirk-thomas added the in review Waiting for review (Kanban column) label Jun 28, 2017

dirk-thomas self-assigned this Jun 28, 2017

clalancette approved these changes Jun 28, 2017

View reviewed changes

truncate arrays, bytes, and strings by default, add option to show in…

33fb094

… full or use custom threshold

dirk-thomas force-pushed the truncate_dynamic_length_by_default branch from 7f4d7bd to 33fb094 Compare June 28, 2017 20:19

dirk-thomas changed the title ~~truncate dynamic length by default, add option to show in full~~ truncate arrays, bytes, and strings by default, add option to show in full or use custom threshold Jun 28, 2017

wjwwood approved these changes Jun 28, 2017

View reviewed changes

add short options

0777ef2

dirk-thomas merged commit 5429d29 into master Jun 28, 2017

dirk-thomas deleted the truncate_dynamic_length_by_default branch June 28, 2017 22:18

dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

truncate arrays, bytes, and strings by default, add option to show in full or use custom threshold #31

truncate arrays, bytes, and strings by default, add option to show in full or use custom threshold #31

dirk-thomas commented Jun 28, 2017

clalancette left a comment

dirk-thomas commented Jun 28, 2017

mikaelarguedas commented Jun 28, 2017

mikaelarguedas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017 •

edited

Loading

mikaelarguedas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

clalancette commented Jun 28, 2017

wjwwood commented Jun 28, 2017 •

edited

Loading

dirk-thomas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

wjwwood left a comment

wjwwood Jun 28, 2017

clalancette Jun 28, 2017

dirk-thomas Jun 28, 2017

wjwwood Jun 28, 2017

dirk-thomas Jun 28, 2017

wjwwood commented Jun 28, 2017

clalancette commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

mikaelarguedas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

truncate arrays, bytes, and strings by default, add option to show in full or use custom threshold #31

truncate arrays, bytes, and strings by default, add option to show in full or use custom threshold #31

Conversation

dirk-thomas commented Jun 28, 2017

clalancette left a comment

Choose a reason for hiding this comment

dirk-thomas commented Jun 28, 2017

mikaelarguedas commented Jun 28, 2017

mikaelarguedas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017 • edited Loading

mikaelarguedas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

clalancette commented Jun 28, 2017

wjwwood commented Jun 28, 2017 • edited Loading

dirk-thomas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Jun 28, 2017

Choose a reason for hiding this comment

clalancette Jun 28, 2017

Choose a reason for hiding this comment

dirk-thomas Jun 28, 2017

Choose a reason for hiding this comment

wjwwood Jun 28, 2017

Choose a reason for hiding this comment

dirk-thomas Jun 28, 2017

Choose a reason for hiding this comment

wjwwood commented Jun 28, 2017

clalancette commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

mikaelarguedas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017

dirk-thomas commented Jun 28, 2017 •

edited

Loading

wjwwood commented Jun 28, 2017 •

edited

Loading