QoS autodetection #613

gonzodepedro · 2021-03-23T19:44:06Z

First iteration. Connects to #594

Signed-off-by: Gonzalo de Pedro gonzalo@depedro.com.ar

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro · 2021-03-23T19:46:38Z

Things to do:

Autodetect when nodes come and go
Consider using 'QoS Profile compatibility" API

ivanpauno

LGTM after making user provided qos settings have higher precedence than the autodetected ones.
I've also left some minor comments

The TODOs can be left for a follow-up, but here some comments:

Autodetect when nodes come and go

I would rather do what was commented here:

Maybe we don't have to detect when a new publisher joins exactly, rather we can detect when there's a new publisher with an incompatible QoS. There is a callback API that can let us know when a new incompatible publisher is detected (see this example).

I actually think that we're installing an "incompatible qos" event handler by default, so maybe nothing is needed here (?).
You can try to force an incompatibility and see if you get a warning.

Consider using 'QoS Profile compatibility" API

I'm not sure if that will help much here.

ivanpauno · 2021-03-24T20:26:45Z

ros2topic/ros2topic/verb/echo.py

@@ -82,6 +84,59 @@ def add_arguments(self, parser, cli_name):
        parser.add_argument(
            '--raw', action='store_true', help='Echo the raw binary representation')

+    def chooseQoS(self, node, args):


nit: snake_case

Addresed in 8efa6ed

ivanpauno · 2021-03-24T20:29:10Z

ros2topic/ros2topic/verb/echo.py

+        except NotImplementedError as e:
+            return str(e)


is this needed?

get_publishers_info_by_topic should be implemented everywhere

If it's not implemented, it seems like a bug and we should just let the exception propagate.

Addresed in 8efa6ed

ivanpauno · 2021-03-24T20:30:04Z

ros2topic/ros2topic/verb/echo.py

+
+        try:
+            for info in node.get_publishers_info_by_topic(args.topic_name):
+                node_count += 1


nit: this is the publishers count, not the node count

Addresed in 8efa6ed

ivanpauno · 2021-03-24T21:04:20Z

ros2topic/ros2topic/verb/echo.py

        with NodeStrategy(args) as node:
+
+            qos_profile = self.chooseQoS(node, args)


ideally, if the user provided an argument that should be preferred over the autodetection logic.
I mean one of these:

ros2cli/ros2topic/ros2topic/api/__init__.py

Lines 189 to 219 in 56db2ba

parser.add_argument(

'--qos-profile',

choices=rclpy.qos.QoSPresetProfiles.short_keys(),

default=default_preset,

help='Quality of service preset profile to {} with (default: {})'

.format(verb, default_preset))

default_profile = rclpy.qos.QoSPresetProfiles.get_from_short_key(

default_preset)

parser.add_argument(

'--qos-depth', metavar='N', type=int, default=-1,

help='Queue size setting to {} with '

'(overrides depth value of --qos-profile option)'

.format(verb))

parser.add_argument(

'--qos-history',

choices=rclpy.qos.QoSHistoryPolicy.short_keys(),

help='History of samples setting to {} with '

'(overrides history value of --qos-profile option, default: {})'

.format(verb, default_profile.history.short_key))

parser.add_argument(

'--qos-reliability',

choices=rclpy.qos.QoSReliabilityPolicy.short_keys(),

help='Quality of service reliability setting to {} with '

'(overrides reliability value of --qos-profile option, default: {})'

.format(verb, default_profile.reliability.short_key))

parser.add_argument(

'--qos-durability',

choices=rclpy.qos.QoSDurabilityPolicy.short_keys(),

help='Quality of service durability setting to {} with '

'(overrides durability value of --qos-profile option, default: {})'

.format(verb, default_profile.durability.short_key))

@jacobperron what do you think?

to handle that I would delete the defaults from the argument and make them optional, so you can check if they were set or not

I agree. The CLI options should have priority over the auto-detection logic.

I made some changes to give CLI options priority over auto-detection logic

Addresed in 8efa6ed

jacobperron · 2021-03-24T21:36:04Z

ros2topic/ros2topic/verb/echo.py

+        except NotImplementedError as e:
+            return str(e)


If it's not implemented, it seems like a bug and we should just let the exception propagate.

ros2topic/ros2topic/verb/echo.py

jacobperron · 2021-03-24T21:39:20Z

ros2topic/ros2topic/verb/echo.py

        with NodeStrategy(args) as node:
+
+            qos_profile = self.chooseQoS(node, args)


I agree. The CLI options should have priority over the auto-detection logic.

jacobperron · 2021-03-24T21:42:52Z

I agree with @ivanpauno about the following two points:

Autodetect when nodes come and go

Just produce a warning if a new publisher joins with incompatible QoS (don't update the subscriber QoS). Maybe we don't have to implement the warning as Ivan points out.

Consider using 'QoS Profile compatibility" API

I think the API we could reuse would be this proposed RMW API: ros2/rmw#304

Since that API doesn't exist (yet), we could just add a comment referencing the proposal.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro · 2021-04-14T14:33:05Z

I'm looking at the failing tests

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

ivanpauno

LGTM!

I left some minor comments that can be ignored.
We should wait until a galactic branch is created before merging though.

ivanpauno · 2021-04-15T14:28:31Z

ros2topic/ros2topic/verb/echo.py

+        for info in node.get_publishers_info_by_topic(args.topic_name):
+            publishers_count += 1
+            if (info.qos_profile.reliability ==
+                QoSReliabilityPolicy.RELIABLE):
+                    reliability_reliable_endpoints_count += 1
+            if (info.qos_profile.durability ==
+                QoSDurabilityPolicy.TRANSIENT_LOCAL):
+                    durability_transient_local_endpoints_count += 1


Minor style comment, feel free to ignore:

this is a bit more readable (though it iters twice over the list):

Suggested change

for info in node.get_publishers_info_by_topic(args.topic_name):

publishers_count += 1

if (info.qos_profile.reliability ==

QoSReliabilityPolicy.RELIABLE):

reliability_reliable_endpoints_count += 1

if (info.qos_profile.durability ==

QoSDurabilityPolicy.TRANSIENT_LOCAL):

durability_transient_local_endpoints_count += 1

pubs_info = node.get_publishers_info_by_topic(args.topic_name)

publishers_count = len(pubs_info)

reliability_reliable_endpoints_count = sum(

info.qos_profile.reliability == QoSReliabilityPolicy.RELIABLE for info in pubs_info)

durability_transient_local_endpoints_count = sum(

info.qos_profile.durability == QoSDurabilityPolicy.TRANSIENT_LOCAL for info in pubs_info)

Thanks for pointing this out. I made some changes based on your comments, but not exactly the one you suggested.

Addressed in: afb4873

ivanpauno · 2021-04-15T14:30:57Z

ros2topic/ros2topic/verb/echo.py

+                    'Some, but not all, publisher are offering '
+                    'QoSReliabilityPolicy.RELIABLE. Falling back to '
+                    'QoSReliabilityPolicy.BEST_EFFORT as it will connect '
+                    'to all publishers', end='\n\n'


is there any benefit of using end?
do we need to print this message?
should we use stderr?

Suggested change

'to all publishers', end='\n\n'

'to all publishers\n\n'

Addressed in: afb4873

ivanpauno · 2021-04-15T14:31:09Z

ros2topic/ros2topic/verb/echo.py

+                    'Some, but not all, publisher are offering '
+                    'QoSDurabilityPolicy.TRANSIENT_LOCAL. Falling back to '
+                    'QoSDurabilityPolicy.VOLATILE as it will connect '
+                    'to all publishers', end='\n\n'


same comments here

Addressed in: afb4873

ivanpauno · 2021-04-15T15:21:37Z

ros2topic/ros2topic/api/__init__.py

@@ -182,20 +186,20 @@ def qos_profile_from_short_keys(


 def add_qos_arguments_to_argument_parser(
-    parser: argparse.ArgumentParser, is_publisher: bool = True, default_preset: str = 'sensor_data'
+    parser: argparse.ArgumentParser, is_publisher: bool = True,
+    default_preset: str = default_preset_str


this argument is unused

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron

Mostly minor comments; I have some concerns about reusing the argparse logic between the pub and echo verbs. The implementation of choose_qos LGTM.

ros2topic/ros2topic/verb/echo.py

jacobperron · 2021-04-22T21:59:31Z

ros2topic/ros2topic/verb/echo.py

+            return qos_profile_from_short_keys(args.qos_profile,
+                                               reliability=args.qos_reliability,
+                                               durability=args.qos_durability,
+                                               depth=args.qos_depth,
+                                               history=args.qos_history)


Style nit: prefer hanging indents

Suggested change

return qos_profile_from_short_keys(args.qos_profile,

reliability=args.qos_reliability,

durability=args.qos_durability,

depth=args.qos_depth,

history=args.qos_history)

return qos_profile_from_short_keys(

args.qos_profile,

reliability=args.qos_reliability,

durability=args.qos_durability,

depth=args.qos_depth,

history=args.qos_history)

Addressed on 160fce7

The style didn't change here.

ros2topic/ros2topic/verb/echo.py

ros2topic/ros2topic/api/__init__.py

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron · 2021-04-30T22:14:08Z

ros2topic/ros2topic/verb/echo.py

+            args.qos_durability is not None or
+            args.qos_depth is not None or
+            args.qos_history is not None
+            ):


Suggested change

):

):

jacobperron

There a linter error that my suggestion above should fix. LGTM with green CI.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron · 2021-05-04T20:22:05Z

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2021-06-13T19:39:11Z

ros2topic/ros2topic/api/__init__.py

    return profile
-
-
-def add_qos_arguments_to_argument_parser(


I've found an instance of this function being used in the wild. In the future, we should be mindful of removing API from this file since it appears as part of the public API (unless we otherwise state that users should't rely on it).

Added autodetection

8efa6ed

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron self-requested a review March 24, 2021 20:16

ivanpauno reviewed Mar 24, 2021

View reviewed changes

jacobperron reviewed Mar 24, 2021

View reviewed changes

clalancette assigned gonzodepedro Apr 8, 2021

Changes based con review

5d70fbc

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested review from jacobperron and ivanpauno April 14, 2021 14:33

Fixed linter, removed comments

b414acf

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro marked this pull request as ready for review April 14, 2021 20:19

ivanpauno reviewed Apr 15, 2021

View reviewed changes

Gonzalo de Pedro added 3 commits April 22, 2021 13:04

Fixed bug, changes based on review

a63105a

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Fixed bug

751f584

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Changes based on review

afb4873

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from ivanpauno April 22, 2021 19:33

jacobperron reviewed Apr 22, 2021

View reviewed changes

Changes based on review

160fce7

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from jacobperron April 30, 2021 00:18

jacobperron reviewed Apr 30, 2021

View reviewed changes

jacobperron approved these changes Apr 30, 2021

View reviewed changes

Fixed style

36bcf07

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from jacobperron May 4, 2021 20:15

jacobperron approved these changes May 5, 2021

View reviewed changes

gonzodepedro merged commit b6f1b6b into ros2:master May 5, 2021

jacobperron mentioned this pull request May 11, 2021

Try to autodetect matching QoS in ros2 topic pub/echo #594

Closed

jacobperron reviewed Jun 13, 2021

View reviewed changes

clalancette mentioned this pull request Apr 29, 2022

ROS 2 Humble needed release notes ros2/ros2_documentation#2446

Closed

87 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QoS autodetection #613

QoS autodetection #613

gonzodepedro commented Mar 23, 2021

gonzodepedro commented Mar 23, 2021 •

edited

Loading

ivanpauno left a comment

ivanpauno Mar 24, 2021

gonzodepedro Apr 14, 2021

ivanpauno Mar 24, 2021

jacobperron Mar 24, 2021

gonzodepedro Apr 14, 2021

ivanpauno Mar 24, 2021

gonzodepedro Apr 14, 2021

ivanpauno Mar 24, 2021

ivanpauno Mar 24, 2021

jacobperron Mar 24, 2021

gonzodepedro Apr 14, 2021

jacobperron Mar 24, 2021

jacobperron Mar 24, 2021

jacobperron commented Mar 24, 2021

gonzodepedro commented Apr 14, 2021

ivanpauno left a comment

ivanpauno Apr 15, 2021

gonzodepedro Apr 22, 2021 •

edited

Loading

ivanpauno Apr 15, 2021

gonzodepedro Apr 22, 2021

ivanpauno Apr 15, 2021

gonzodepedro Apr 22, 2021

ivanpauno Apr 15, 2021

gonzodepedro Apr 22, 2021

jacobperron left a comment

jacobperron Apr 22, 2021

gonzodepedro Apr 30, 2021

jacobperron Apr 30, 2021

jacobperron Apr 30, 2021

jacobperron left a comment

jacobperron commented May 4, 2021 •

edited by gonzodepedro

Loading

jacobperron Jun 13, 2021

		with NodeStrategy(args) as node:

		qos_profile = self.chooseQoS(node, args)

	parser.add_argument(
	'--qos-profile',
	choices=rclpy.qos.QoSPresetProfiles.short_keys(),
	default=default_preset,
	help='Quality of service preset profile to {} with (default: {})'
	.format(verb, default_preset))
	default_profile = rclpy.qos.QoSPresetProfiles.get_from_short_key(
	default_preset)
	parser.add_argument(
	'--qos-depth', metavar='N', type=int, default=-1,
	help='Queue size setting to {} with '
	'(overrides depth value of --qos-profile option)'
	.format(verb))
	parser.add_argument(
	'--qos-history',
	choices=rclpy.qos.QoSHistoryPolicy.short_keys(),
	help='History of samples setting to {} with '
	'(overrides history value of --qos-profile option, default: {})'
	.format(verb, default_profile.history.short_key))
	parser.add_argument(
	'--qos-reliability',
	choices=rclpy.qos.QoSReliabilityPolicy.short_keys(),
	help='Quality of service reliability setting to {} with '
	'(overrides reliability value of --qos-profile option, default: {})'
	.format(verb, default_profile.reliability.short_key))
	parser.add_argument(
	'--qos-durability',
	choices=rclpy.qos.QoSDurabilityPolicy.short_keys(),
	help='Quality of service durability setting to {} with '
	'(overrides durability value of --qos-profile option, default: {})'
	.format(verb, default_profile.durability.short_key))

QoS autodetection #613

QoS autodetection #613

Conversation

gonzodepedro commented Mar 23, 2021

gonzodepedro commented Mar 23, 2021 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron commented Mar 24, 2021

gonzodepedro commented Apr 14, 2021

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzodepedro Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

jacobperron commented May 4, 2021 • edited by gonzodepedro Loading

Choose a reason for hiding this comment

gonzodepedro commented Mar 23, 2021 •

edited

Loading

gonzodepedro Apr 22, 2021 •

edited

Loading

jacobperron commented May 4, 2021 •

edited by gonzodepedro

Loading