-
Notifications
You must be signed in to change notification settings - Fork 192
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
QoS profile files proposal #296
Conversation
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting a proposal!
I've left some thoughts and wording suggestions. In addition to the feedback below, I think we should consider other proposed solutions. For example, using a well-known set of ROS parameters was proposed. We should consider (and document in this design doc) the pros/cons of this approach versus a QoS specific file.
Also, what about providing individual QoS settings on the command-line (instead of bundling them in a file)? The idea is similar to what is possible with parameters; using parameters as a back-bone would give us the implementation for free. I haven't thought about it too much yet, but seems like it's worth considering.
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Geoffrey Biggs <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another round of review. After that, I think we can request review from a larger audience. It's probably worth making a post on Discourse to invite community feedback.
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
```xml | ||
<qos_profiles> | ||
<default> | ||
<publisher topic_name="/my_ns/nested_ns/asd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have a default profile that applies to everything, no matter what topic or service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine:
<publisher topic_name="/**">
I didn't think about something that works for different kind of entities.
@ivanpauno What's your plan for moving forward with this? Do you plan to open it up to wider comments as a proposal, or are you going to take it as a design document and begin prototyping an implementation? If the latter, then I think you need to make it more concrete first. There are still a lot of coulds and shoulds in it. |
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Geoffrey Biggs <[email protected]>
That's a great question.
I agree. I will reorganize the document before starting a prototype. |
I think refining those coulds and shoulds into musts and shalls while you produce the prototype is a fine approach. I don't think there's a need for a perfect document before starting to implement something to confirm decisions. |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/configuring-qos-profiles-from-an-external-file-proposal/15991/1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this feature is useful and productive, we would use Named QoS Profile
especially.
|
||
### Author's opinion | ||
|
||
Using yaml sounds preferred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just a comment) So do I, I don't see we necessarily use XML at the moment.
Furthermore, the option to externally configure QoS settings should be disabled by default (ie. node authors must opt-in). | ||
The rationale for an opt-in policy is to force node authors to have to think about the implications of allowing various QoS changes to the node entities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking opposite. It would be hard for node authors to make decision since this is dependent on what application requires. (i think that i am gonna find difficulty to make decision so maybe i am gonna just enables it.) even with the same node
, topics
and services
, we'd like to adjust the QoS based on the use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for node author opt-in. Otherwise you can end up in out of spec situations with strange behavior that is maybe not obvious when debugging
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@wjwwood can you share your thoughts on the matter? |
As discussed in the middleware WG meeting, are you aware of the QoS policy definitions in rosbag2?
this feature allows a user to specify the QoS settings for any recorded topic. |
I wasn't aware of that particular feature. I think that the community will be benefited if we have a single and standard mechanism to allow configuring QoS profiles. The |
I'm copying notes from the middleware working group meeting, thanks everybody for the feedback! Notes:
@wjwwood About using parameters, I would like to see a concrete proposal that can satisfy the following requirements:
|
@ivanpauno replying to #296 (comment):
I am not sure that I understand what you mean by that. Are you saying that there are nodes that do not need any parameterization? If yes I agree with that but what is the penalty if all nodes get parameterizied with the QoS profiles?
I would first like that we agree that QoS settings in a node are no different than any other parameter that the node might have. Namely as an example, a camera node that sets the frequency at which an image is published out of the node, is almost the same thing as setting the reliability at which this node sends out those same images. Or consider Deadline - that is a totally node specific configuration that will be different for a camera node publishing messages at 25Hz and lidar node publishing messages at 10Hz. Now if I am able to convince you that, then I would like to say that there are definitely use cases for parameterizing a "group" of things in the nodes. For instance, if you have 4 cameras on your robot car and you are processing the data from these 4 cameras. The downstream nodes will use the same configuration for queue size, debayerization, rectification, image size, ... With this logic, if "parameters API isn't great for representing a "group" of things", then that is really a flaw that should be fixed in the implementation of ROS 2 parameters.
We implemented the read only parameters: https://github.com/ros2/rclcpp/pull/495/commits.
What I mean is that we already have 3 different ways to configure ROS 2 applications which you list in the design document:
Now you are adding the fourth one and this complicates things in that the other 3 ways will not go away and it is also impossible to prohibit users to not use the above 3 ways to also configure QoS settings via them. Look what user now needs to do in their application to get through the configuration phase:
I think that this is insanely complicated and exactly error prone because there is too many knobs to turn. |
What I mean is that all nodes need some parameterization of QoS policies. Particularly:
The proposed issue is opt-in and by default it is not enabled, so there shouldn't be any surprises except the node's author consciously activated the feature. About the difference between parameters and QoS settings: It's true that qos settings can be considered as any other parameter. The main issue with using parameters is that you cannot make them look ergonomic, or at least, I haven't come up with something that is ergonomic. More on that later ...
I was thinking the same while I was writing my original comment, but I don't think that solving that problem will be enough.
Yeap! About the current ways of configuring QoS settings:
I don't think it will be confusing to have a mechanism specifically targeted to solve the QoS settings parameterization problems + this two "general" mechanisms to parameterize other things, but it might be arguable.
I think this doesn't solve the problem. If we make this mechanism more powerful, following the ideas of ros2/rmw_fastrtps#335, I don't think that will look ergonomic. This mechanism cannot be applied to non-DDS rmw implementations, which I think it's not ideal. IMO having DDS profile files as the only mechanism to solve the problem ergonomically is not an option, but I understand others might disagree.
If we never use arguments and parameters to configure QoS, that won't be in the list of things you need to check. I think that I need a more compelling argument about why I think that to manually parameterize QoS settings (i.e. not having a standard mechanism for it) don't work and why using parameters (even with the automatic creation of a known parameter subset) don't look ergonomic. I will try to write a full post about that. |
Ok I will try to answer why I think that we need an standard "built-in" mechanism to parameterize QoS settings and why parameters doesn't look ergonomic. The need of a built-in mechanismSuppose a node needs to parameterize the reliability/history kind/history depth of a publisher or subscription (which are required to be parameterized quite often): So node A has only one publisher and one subscription, and the node uses the following parameters:
node B has also only one publisher and one subscription, and that nodes uses the following parameters:
I think those kind of differences will create a usability problem. Ergonomics of a solution based on parametersI will suppose that the parameters API is more powerful than now:
Based on that, I imagine that the best we can come up is something like this: /**:
ros__parameters:
qos_profiles: # group for all the qos settings
# Using '/' will create problems with ros URLs to address parameters, thus a replacement is needed.
# Using nested groups will create inconsistencies (and it would be a bit confusing).
# We need a character that is not accepted in topic names but accepted in parameter names
# I think both '/' and '#' aren't accepted in parameter or topic names currently (but we could fix that for parameters).
'my#fully#qualified#topic#name#here':
publisher: &my_profile
reliability: reliable
history_depth: 100
history: keep_last
/my_ns/nested_ns/my_node:
ros__parameters:
qos_profiles:
'my#fully#qualified#topic#name#here':
publisher:
<<: *my_profile,
reliability: best_effort # yeah, yaml anchors allow this :) Maybe, it doesn't look that but after all ...
I think I will be happy with a solution based on parameters with all that solved ... (I have to put a bit more of thought on the parameters alternative, but it seems to be solvable). |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-agenda-2020-09-17/16464/1 |
### QoS profiles IDs (optional) | ||
|
||
In the case that a node wants to create two `publishers` on the same topic with different QoS, the above format wouldn't allow to uniquely identify the `publisher`. The same applies to other kinds of entitys. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the profile ID provides the matching between entities in the code and in the QoS configuration. I'm wondering if maybe there will be other configurations for entities in future and if it would make sense to have a more generic entity ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the profile ID provides the matching between entities in the code and in the QoS configuration. I'm wondering if maybe there will be other configurations for entities in future and if it would make sense to have a more generic entity ID.
The concept of ID would apply to subscription/publishers/etc, I'm not sure if I understand the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the profile ID provides the matching between entities in the code and in the QoS configuration. I'm wondering if maybe there will be other configurations for entities in future and if it would make sense to have a more generic entity ID.
The concept of ID would apply to subscription/publishers/etc, I'm not sure if I understand the question.
I more thought of other non-QoS topics. Maybe there will be things in future where you have to configure something per entity and a differentiation is needed.
One example that just comes in my mind would be priorities for callbacks of subscribers. Imagine there is an executor that supports this and can also be configured similar to your QoS settings. Then you could also have two different subscribers on the same topic that shall be handled differently and must be uniquely identified (Maybe a crazy example).
My point is, if you have to introduce an ID to solve this uniqueness issue for QoS, would it maybe make sense to introduce this more generally? Then it could be reused if other use cases come up where this is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see your point now.
I think it could be available for other things, there's nothing restricting that.
I wouldn't make that "id" introspectable in the graph though.
Furthermore, the option to externally configure QoS settings should be disabled by default (ie. node authors must opt-in). | ||
The rationale for an opt-in policy is to force node authors to have to think about the implications of allowing various QoS changes to the node entities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for node author opt-in. Otherwise you can end up in out of spec situations with strange behavior that is maybe not obvious when debugging
Closing in favor of #300. |
Fixes #280.
I have written down the ideas I have in a document, and I have included the points that were raised in previous discussions.