Exclude topics #105

danieldube · 2019-04-30T09:26:53Z

Changed the PlayOptions class to hold a topic filter function. This function tells the player, whether a topic should be included in the playback or not.

The python API now uses the topic filter to provide an exclude topic feature.

Karsten1987

Thanks for the patch. Looks good so far, but I have a few remarks for code reusability.
I request changes for the linters and would ideally like to see an extension of the current tests for play which tests the new functionality.

rosbag2_transport/include/rosbag2_transport/play_options.hpp

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

rosbag2_transport/include/rosbag2_transport/play_options.hpp

Karsten1987 · 2019-04-30T19:04:05Z

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

+    if (exclude_topics_list.empty() == false) {
+      auto topic_filter_function = [exclude_topics_list](const std::string& topic)
+      {
+        auto entry = exclude_topics_list.find(topic);


Could this block just be replaced by
return (exclude_topics_list.find(topic) != exclude_topics_list.end());

I would argue, that the current implementation is more readable and therefore more expressive than putting everything into one line. However, if you like to change this, I'll do it.

I thought to believe that it's easier for the compiler to inline and optimize this as there is no if and thus no branch prediction and stuff might happen. But it's solely premature optimization thinking :)

Karsten1987 · 2019-04-30T19:12:02Z

ros2bag/ros2bag/verb/play.py

@@ -43,4 +46,5 @@ def main(self, *, args):  # noqa: D102
            uri=bag_file,
            storage_id=args.storage,
            node_prefix=NODE_NAME_PREFIX,
-            read_ahead_queue_size=args.read_ahead_queue_size)
+            read_ahead_queue_size=args.read_ahead_queue_size,
+            exclude_topics=args.exclude_topics)


The check for unique topic names can be done in python. I guess that's a bit easier and avoids the need of a std::set.
Something like exclude_topics = list(set(args.exclude_topics))

For me a set would be the natural choice of container. I guess at this point it doesn't make a difference, but the idea is to have a O(log n) complexity for excluding topic names.

Using a set on python side, wouldn't guarantee, that the API isn't misused from Python side in the future. Using a sorted vector and binary search for the topic lookup bloats the code. @Karsten1987, are you sure you want to have a std::vector instead of a std::set?

I am not completely certain that I follow correctly. The idea to filter out duplicates in Python is really just a convenience thing because it's a one-liner in Python. That allows us further to pass in a vector with unique topic names in C++.
I am not so much concerned about a misuse from Python side as the current c++ file relies on receiving PyObjects.

Karsten1987 · 2019-04-30T19:13:40Z

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

@@ -124,6 +128,32 @@ rosbag2_transport_play(PyObject * Py_UNUSED(self), PyObject * args, PyObject * k
  play_options.node_prefix = std::string(node_prefix);
  play_options.read_ahead_queue_size = read_ahead_queue_size;

+  std::set<std::string> exclude_topics_list;


Given the command that the topic names could be filtered for uniqueness on the python side, I believe it makes sense to parse the topics as a vector and reuse the code for converting the PyObject to a vector from the record function. I believe this conversion can be excluded in a separate function.

Refactored the PyObject parser code into a generic parse_python_list() function.

… (Change Request ros2#3)

Jenkins had some build problems I couldn't reproduce on my local system. get_messages_primitives() could not be found.

danieldube · 2019-05-14T11:28:29Z

Currently the Jenkins can not build the new test case for the exclude_topic_filter. The error message is

/tmp/ws/src/rosbag2/rosbag2_transport/test/rosbag2_transport/test_play.cpp:111:61: error: ‘Primitives’ is not a member of ‘test_msgs::msg’
   auto primitive_message = std::make_shared<test_msgs::msg::Primitives>();

(http://build.ros2.org/job/Dpr__rosbag2__ubuntu_bionic_amd64/8/consoleText )

I can not reproduce it on my local system. The class test_msgs::msg::Primitives is well defined in the header file primitives__struct.hpp. The same object is used in the test case recorded_messages_are_played_for_all_topics defined in the same source file. The only thing I recognized is, that the line number 111 stated by Jenkins is wrong. Actually the rejected code is in line 112.

I'm out of ideas. @Karsten1987, do you have a clue where I should start looking for this problem?

Karsten1987 · 2019-05-14T16:26:19Z

The test_msgs in Dashing were recently renamed. There is no longer a test_msgs::msg::Primitives message. That type was renamed to BasicTypes.
We recently had to change all these test messages ourselves:
https://github.com/ros2/rosbag2/pull/111/files

In any case, thanks for iterating over this PR. I'll review your PR as soon as possible.

Karsten1987 · 2019-05-14T20:59:47Z

Looking at this PR again, I believe you actually have to rebase your branch on top of the latest master.

danieldube · 2019-05-15T09:29:08Z

Thanks, this was exactly what I was missing. The current rosbag2 master is not compiling using the latest ROS2 stable release anymore. Since I can not build the ROS2 master on my system, I'm trying to fix the build problems in blind mode. Let's see what Jenkins is saying. ;)

Karsten1987

I have two comments I would like to iterate over:

why not exposing a std::vector<std::string> in the options? I believe that's a bit more user-friendly than forcing to write a complete lambda/function.
I think it's easier to turn this list of excluded topics into a whitelist. This basically means that you only ask once for a list of all topics, cross-validate this with all available topics and run the code as always just like when a --topics is specified. This avoids to iterate over the blacklisted topics every time when calling publish on it.

Karsten1987 · 2019-05-17T00:55:13Z

ros2bag/ros2bag/verb/play.py

@@ -43,4 +46,5 @@ def main(self, *, args):  # noqa: D102
            uri=bag_file,
            storage_id=args.storage,
            node_prefix=NODE_NAME_PREFIX,
-            read_ahead_queue_size=args.read_ahead_queue_size)
+            read_ahead_queue_size=args.read_ahead_queue_size,
+            exclude_topics=args.exclude_topics)


I am not completely certain that I follow correctly. The idea to filter out duplicates in Python is really just a convenience thing because it's a one-liner in Python. That allows us further to pass in a vector with unique topic names in C++.
I am not so much concerned about a misuse from Python side as the current c++ file relies on receiving PyObjects.

Karsten1987 · 2019-05-17T00:56:54Z

rosbag2_transport/src/rosbag2_transport/player.cpp

 {
  ReplayableMessage message;
  while (message_queue_.try_dequeue(message) && rclcpp::ok()) {
+    if (options.exclude_topic_filter(message.message->topic_name)) {


is that check actually still needed if in prepare_publishers there are no publishers available for excluded topics?

Karsten1987 · 2019-05-17T01:03:48Z

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

@@ -22,6 +23,23 @@
 #include "rosbag2_transport/storage_options.hpp"
 #include "rmw/rmw.h"

+
+template<class InserterIterator>


Karsten1987 · 2019-05-17T01:08:02Z

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

+    if (exclude_topics_list.empty() == false) {
+      auto topic_filter_function = [exclude_topics_list](const std::string& topic)
+      {
+        auto entry = exclude_topics_list.find(topic);


I thought to believe that it's easier for the compiler to inline and optimize this as there is no if and thus no branch prediction and stuff might happen. But it's solely premature optimization thinking :)

Karsten1987 · 2019-05-17T01:08:18Z

rosbag2_transport/test/rosbag2_transport/test_play.cpp

@@ -104,3 +104,56 @@ TEST_F(RosBag2PlayTestFixture, recorded_messages_are_played_for_all_topics)
    Each(Pointee(Field(&test_msgs::msg::Arrays::float32_values,
    ElementsAre(40.0f, 2.0f, 0.0f)))));
 }
+


Suggested change

Karsten1987 · 2019-05-17T01:13:39Z

rosbag2_transport/include/rosbag2_transport/play_options.hpp

  size_t read_ahead_queue_size;
  std::string node_prefix = "";
+  TopicFilter exclude_topic_filter = [](const std::string &) {return false;};


When I am thinking about this, I believe it's a bit overkill to provide a lambda/function for this. Why can't a simple std::vector<std::string> not suffice? Especially when using this as an user facing option. I believe a list of vectors is easier and we can internally apply a generic lambda.

Karsten1987 · 2019-05-21T00:09:15Z

@danieldube Is there any update from your side on this PR?

danieldube · 2019-05-21T06:29:49Z

Nope. Didn't have time yet to get back to this.

Karsten1987 · 2019-08-06T18:57:00Z

@danieldube just a friendly ping for an update

danieldube · 2019-08-07T07:37:23Z

I will not be able to do the necessary changes right now. I will reopen the request, when there is some progress. @Karsten1987, thanks for the review.

danieldube added 2 commits April 29, 2019 18:04

Implemented exclude_topic feature.

fccafd0

Changed parameter parser to exclude-topics.

c7a885a

tfoote added the in review label Apr 30, 2019

Karsten1987 requested changes Apr 30, 2019

View reviewed changes

Fixed all linter errors. (Change Request ros2#1)

ec055cb

danieldube force-pushed the exclude_topics branch from 65ed36b to ec055cb Compare May 13, 2019 16:21

danieldube added 5 commits May 13, 2019 18:22

Fixed include order. (Change Request ros2#2)

255102d

Renamed PlayOptions.topic_filter to PlayOptions.exclude_topic_filter.…

9202737

… (Change Request ros2#3)

Refactored the Python list parser. (Change Request 6)

65ed49e

Implemented a test for players exclude_topic feature.

9bb8d9d

Simplified the exclude_topic_filter test.

f521e92

Jenkins had some build problems I couldn't reproduce on my local system. get_messages_primitives() could not be found.

danieldube added 2 commits May 15, 2019 11:08

Merge branch 'master' into exclude_topics

f7297a0

Changed the test case exclude_topic_filter to work with ROS master.

9880bf2

Additional change to fix the build for the current master.

ce5cad4

Karsten1987 reviewed May 17, 2019

View reviewed changes

danieldube closed this Aug 7, 2019

mjeronimo mentioned this pull request Nov 25, 2022

Expose Recorder::pause/resume to Python #558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude topics #105

Exclude topics #105

danieldube commented Apr 30, 2019

Karsten1987 left a comment

Karsten1987 Apr 30, 2019

danieldube May 13, 2019

Karsten1987 May 17, 2019

Karsten1987 Apr 30, 2019

danieldube May 13, 2019 •

edited

Loading

Karsten1987 May 17, 2019

Karsten1987 Apr 30, 2019

danieldube May 13, 2019

danieldube commented May 14, 2019 •

edited

Loading

Karsten1987 commented May 14, 2019

Karsten1987 commented May 14, 2019

danieldube commented May 15, 2019

Karsten1987 left a comment

Karsten1987 May 17, 2019

Karsten1987 May 17, 2019

Karsten1987 May 17, 2019

Karsten1987 May 17, 2019

Karsten1987 May 17, 2019

Karsten1987 May 17, 2019

Karsten1987 commented May 21, 2019

danieldube commented May 21, 2019

Karsten1987 commented Aug 6, 2019

danieldube commented Aug 7, 2019

Exclude topics #105

Exclude topics #105

Conversation

danieldube commented Apr 30, 2019

Karsten1987 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

danieldube May 13, 2019 • 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

danieldube commented May 14, 2019 • edited Loading

Karsten1987 commented May 14, 2019

Karsten1987 commented May 14, 2019

danieldube commented May 15, 2019

Karsten1987 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

Karsten1987 commented May 21, 2019

danieldube commented May 21, 2019

Karsten1987 commented Aug 6, 2019

danieldube commented Aug 7, 2019

danieldube May 13, 2019 •

edited

Loading

danieldube commented May 14, 2019 •

edited

Loading