feat(RaiBaseNode): add qos policy matching #353
Conversation
maciejmajek
force-pushed
the
feat/qos-matching
branch
2 times, most recently
from
a946222 to
b10954f
Compare
|
@coderabbitai full review |
WalkthroughThe changes involve enhancements to the ROS 2 node implementation in Changes
Sequence DiagramsequenceDiagram
participant Node as RaiBaseNode
participant Cache as QoS Profile Cache
participant Publishers as Topic Publishers
Node->>Publishers: Request Topic Endpoint Info
Publishers-->>Node: Return Publisher QoS Profiles
Node->>Node: adapt_requests_to_offers()
Node->>Cache: Store Adapted QoS Profile
Node->>Publishers: Subscribe with Adapted QoS
Publishers-->>Node: Publish Messages
Node->>Node: Retrieve Messages
The sequence diagram illustrates the new QoS profile adaptation process, showing how the Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/rai/rai/node.py (2)
292-340: Expand QoS matching logic if needed.
Currently,adapt_requests_to_offersaccounts for reliability and durability but does not address potential conflicts withHistoryPolicyorLivelinessPolicy. This may be acceptable for your immediate use case. However, if comprehensive QoS matching is needed in the future, consider extending the logic to handle those fields.
348-353: Validate QoS usage for dynamic publisher changes.
Caching QoS profiles is a solid optimization. However, if the set of publishers changes (e.g., a new reliable publisher appears later), the node may continue to use an outdated QoS profile. Periodic refresh or a cache invalidation strategy can help maintain optimal QoS handling.tests/messages/test_transport.py (2)
27-53: Confirm resource usage in tests.
This publisher repeatedly generates large random images. While it's acceptable for a short-running test, you might want to restrict image resolution or frequency in larger test suites to keep resource usage manageable.
55-69: Broad coverage, consider QoS mismatch tests.
This test checks basic message reception. Consider adding scenarios with deliberate QoS mismatches (e.g., a best-effort publisher and a reliable subscriber) to validate the newly introduced QoS adaptation logic more thoroughly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/rai/rai/node.py(3 hunks)tests/messages/test_transport.py(1 hunks)
🔇 Additional comments (4)
src/rai/rai/node.py (4)
45-45: Good import.
Importing TopicEndpointInfo is a necessary step for retrieving QoS information from publishers.
272-272: Consider concurrency and cache refresh logic.
You might want to ensure that this cache stays consistent if new publishers appear with different QoS settings. Consider re-checking or clearing stale entries whenever the node discovers new endpoints so that new publishers' QoS settings are applied.
354-357: Efficient reuse of cached QoS.
Reusing a cached QoS profile here is beneficial for performance. Be mindful about whether further concurrency handling is needed if multiple threads invoke get_raw_message_from_topic simultaneously.
362-362: Correct QoS passing to wait_for_message.
Passing the computed QoS profile to the subscription-based helper is consistent with your new QoS matching mechanism.
|
QoS matching massively improved performance (latency, standard deviation) |
|
|
|
Default (no matching) subscriber qos self.qos_profile = QoSProfile(
history=HistoryPolicy.KEEP_LAST,
depth=1,
reliability=ReliabilityPolicy.BEST_EFFORT,
durability=DurabilityPolicy.VOLATILE,
liveliness=LivelinessPolicy.AUTOMATIC,
)Manipulation image publisher Performance gains can be attributed to changing subscriber's QoS from Best Effort to Reliable.
Agriculture image publisher No improvement is due to no change in qos settings.
|
|
As shown before, performance gains are not guaranteed. Due to the nature of ROS 2 and it's subscriber-publisher design dynamic subscriptions are bound to be slower than their static counterpart due to discovery and subscription event overhead. |
| @@ -287,18 +289,77 @@ def discovery(self): | |||
| get_action_names_and_types(self), | |||
| ) | |||
|
|
|||
| def adapt_requests_to_offers( | |||
There was a problem hiding this comment.
maciejmajek
force-pushed
the
feat/qos-matching
branch
from
e4fc176 to
e276f93
Compare
maciejmajek
force-pushed
the
feat/qos-matching
branch
from
75d1030 to
70c497d
Compare
maciejmajek
force-pushed
the
feat/qos-matching
branch
2 times, most recently
from
45625f9 to
1971b45
Compare
maciejmajek
force-pushed
the
feat/qos-matching
branch
from
6495e18 to
a0779e5
Compare
|
As discussed with @boczekbartek, we are merging this PR without the xfail change. Xfail will be added later. |
This reverts commit 4ae430d.
Purpose
QoS matching is essential for proper data transfer in ROS 2. Incompatible QoS settings may result in miscommunication between publishers and subscribers.
Proposed Changes
Simple QoS matching (copying) mechanism, based on
Node.get_publishers_info_by_topicIssues
#336
Testing
Summary by CodeRabbit
New Features
Tests