Filter topic by type #1577

ahcorde · 2024-02-28T15:43:57Z

It will be great to be able to filter topic by type.

Example usage:

ros2 bag record --topic_types sensor_msgs/msg/Image sensor_msgs/msg/CameraInfo

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

MichaelOrlov

@ahcorde Thanks for this PR.

Among other small findings and neatpicks I have a concern about the current logic of how the new parameter --topic-types is handled.
I would expect that two fundamental and conceptual conditions shall be true.

At least one options out of --all, --all-topics, --all-services, --services, --topics, --topic-types or --regex must be used.
Only one option out of --all, --all-topics, --topics, --topic-types or --regex can be used.

The --all means All topics and services regardless of the other inclusive filters among exclusive filters.
i.e. --topic-types shall not override --all. However, currently according to the logic in the topic_filters.cpp and newly created test --topic-types overrides --all parameter.
The same for --all-topics. It means all topics and not services regardless of the other inclusive filters among exclusive filters.

If agree with the proposed logic please change the logic in the topic_filters.cpp and the unit test, if not let's discuss.
Also, need to add new unit tests to the test_topic_filter.cpp to cover these two conceptual conditions. Currently, the changed logic is not covered by the unit tests.

Also, I found that missed part related to the parsing RecordOptions from nodes parameters. We recently added a new feature aka composable recorder/player.
Please add updates for the new field RecordOptions::topics_types to the get_record_options_from_node_params(rclcpp::Node & node)

rosbag2/rosbag2_transport/src/rosbag2_transport/config_options_from_node_params.cpp

Lines 205 to 215 in a2283ab

    
           RecordOptions get_record_options_from_node_params(rclcpp::Node & node) 
        
           { 
        
             RecordOptions record_options{}; 
        
             record_options.all_topics = node.declare_parameter<bool>("record.all_topics", false); 
        
             record_options.all_services = node.declare_parameter<bool>("record.all_services", false); 
        
             record_options.is_discovery_disabled = 
        
               node.declare_parameter<bool>("record.is_discovery_disabled", false); 
        
             record_options.topics = node.declare_parameter<std::vector<std::string>>( 
        
               "record.topics", std::vector<std::string>());

Also please update the corresponding tests.

Will need to add the new parameter topic_types to the

rosbag2/rosbag2_transport/test/resources/recorder_node_params.yaml

Lines 9 to 11 in a2283ab

    
           is_discovery_disabled: true 
        
           topics: ["topic", "other_topic"] 
        
           services: ["service", "other_service"]

Will need to add a check for a new parameter topic_types in the

rosbag2/rosbag2_transport/test/rosbag2_transport/test_composable_recorder.cpp

Lines 213 to 215 in a2283ab

    
           std::vector<std::string> topics {"/topic", "/other_topic"}; 
        
           EXPECT_EQ(record_options.topics, topics); 
        
           std::vector<std::string> services {"/service/_service_event", "/other_service/_service_event"};

ros2bag/ros2bag/verb/record.py

rosbag2_transport/CMakeLists.txt

rosbag2_transport/test/rosbag2_transport/test_record_topic_types.cpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

fujitatomoya

@MichaelOrlov @ahcorde

just one clarification about this behavior in rosbag2_transport.

rosbag2_transport/src/rosbag2_transport/topic_filter.cpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

fujitatomoya

lgtm with green CI

ahcorde · 2024-03-11T08:53:12Z

Linux
Linux-aarch64
Windows

ahcorde · 2024-03-11T17:33:11Z

@MichaelOrlov all good from your side ?

MichaelOrlov · 2024-03-12T03:42:09Z

@ahcorde I am sorry, I didn't have time to get back to it. Please give me one more day or so.

MichaelOrlov

@ahcorde Thanks for addressing comments from the previous review round.
It's almost all good. However, I found a couple of mismatches in the newly added test.

rosbag2_transport/test/rosbag2_transport/test_record_topic_types.cpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

MichaelOrlov · 2024-03-14T06:56:04Z

@ahcorde The ros2bag tests fail with an error message:

==================================== ERRORS ====================================
2024-03-13T10:19:00.8916311Z ____________________________ ERROR collecting test _____________________________
2024-03-13T10:19:00.8917767Z /usr/local/lib/python3.10/dist-packages/pluggy/_hooks.py:501: in __call__
2024-03-13T10:19:00.8920690Z     return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
2024-03-13T10:19:00.8922556Z /usr/local/lib/python3.10/dist-packages/pluggy/_manager.py:119: in _hookexec
2024-03-13T10:19:00.8923632Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-03-13T10:19:00.8924871Z /usr/local/lib/python3.10/dist-packages/_pytest/python.py:225: in pytest_collect_file
2024-03-13T10:19:00.8926155Z     module: Module = ihook.pytest_pycollect_makemodule(
2024-03-13T10:19:00.8927395Z /usr/local/lib/python3.10/dist-packages/_pytest/config/compat.py:79: in fixed_hook
2024-03-13T10:19:00.8928511Z     return hook(**kw)
2024-03-13T10:19:00.8939779Z /usr/local/lib/python3.10/dist-packages/pluggy/_hooks.py:501: in __call__
2024-03-13T10:19:00.8940939Z     return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
2024-03-13T10:19:00.8942176Z /usr/local/lib/python3.10/dist-packages/pluggy/_manager.py:119: in _hookexec
2024-03-13T10:19:00.8943254Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-03-13T10:19:00.8944406Z ../../../../build/launch_testing/launch_testing/pytest/hooks.py:188: in pytest_pycollect_makemodule
2024-03-13T10:19:00.8945018Z     entrypoint = find_launch_test_entrypoint(path)
2024-03-13T10:19:00.8945613Z ../../../../build/launch_testing/launch_testing/pytest/hooks.py:178: in find_launch_test_entrypoint
2024-03-13T10:19:00.8946177Z     module = import_path(path, root=None)
2024-03-13T10:19:00.8946847Z E   TypeError: import_path() missing 1 required keyword-only argument: 'consider_namespace_packages'
2024-03-13T10:19:00.8947631Z --- generated xml file: /__w/rosbag2/rosbag2/ros_ws/build/ros2bag/pytest.xml ---
2024-03-13T10:19:00.8948177Z =========================== short test summary info ============================
2024-03-13T10:19:00.8949067Z ERROR test - TypeError: import_path() missing 1 required keyword-only argument: 'consider_namespace_packages'
2024-03-13T10:19:00.8949855Z !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
2024-03-13T10:19:00.8950330Z =============================== 1 error in 0.16s ===============================
2024-03-13T10:19:00.9269817Z Failed   <<< ros2bag [1.56s, exited with code 2]

MichaelOrlov · 2024-03-14T07:08:19Z

@ahcorde However, I can't reproduce this failure on my local setup.
Will try to restart the CI job.

MichaelOrlov

@ahcorde Approving this PR.
However, I have a concern to the way how written unit tests and logic in the topic_filter.cpp.
In general need to avoid using the transport layer and pub/sub if it could be avoided, especially for testing some specific classes like filters which could be easily tested and verified with regular unit tests. The logic in the topic_filter.cpp needs to be simplified and cleaned up.
I am going to address these issues in the follow-up PR.

ahcorde · 2024-03-14T09:13:01Z

Linux
Linux-aarch64
Windows

ros-discourse · 2024-03-26T00:32:51Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2024-03-21/36814/1

ahcorde added 2 commits February 28, 2024 16:11

Filter topic by type

8883316

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

some fixes

d79429f

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde self-assigned this Feb 28, 2024

ahcorde requested a review from a team as a code owner February 28, 2024 15:43

ahcorde requested review from emersonknapp and hidmic and removed request for a team February 28, 2024 15:43

Added test

991a16d

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from MichaelOrlov March 1, 2024 10:08

MichaelOrlov requested changes Mar 3, 2024

View reviewed changes

Added feedback

0dd02e5

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from MichaelOrlov March 5, 2024 13:56

fujitatomoya reviewed Mar 5, 2024

View reviewed changes

rosbag2_transport/src/rosbag2_transport/topic_filter.cpp Outdated Show resolved Hide resolved

rosbag2_transport/src/rosbag2_transport/topic_filter.cpp Outdated Show resolved Hide resolved

Changed function name

ec77639

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

fujitatomoya approved these changes Mar 8, 2024

View reviewed changes

ahcorde mentioned this pull request Mar 12, 2024

Added exclude-topic-types to record #1582

Merged

MichaelOrlov requested changes Mar 13, 2024

View reviewed changes

rosbag2_transport/test/rosbag2_transport/test_record_topic_types.cpp Outdated Show resolved Hide resolved

rosbag2_transport/test/rosbag2_transport/test_record_topic_types.cpp Outdated Show resolved Hide resolved

fixed test

25f82ba

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from MichaelOrlov March 13, 2024 09:37

MichaelOrlov reviewed Mar 14, 2024

View reviewed changes

MichaelOrlov approved these changes Mar 14, 2024

View reviewed changes

ahcorde merged commit b5e9926 into rolling Mar 14, 2024
13 of 14 checks passed

delete-merged-branch bot deleted the ahcorde/rolling/filter_by_topic_type branch March 14, 2024 12:51

MichaelOrlov mentioned this pull request Mar 14, 2024

Overhaul in the rosbag2_transport::TopicFilter class and relevant tests #1585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter topic by type #1577

Filter topic by type #1577

ahcorde commented Feb 28, 2024

MichaelOrlov left a comment

fujitatomoya left a comment

fujitatomoya left a comment

ahcorde commented Mar 11, 2024

ahcorde commented Mar 11, 2024

MichaelOrlov commented Mar 12, 2024

MichaelOrlov left a comment

MichaelOrlov commented Mar 14, 2024

MichaelOrlov commented Mar 14, 2024

MichaelOrlov left a comment

ahcorde commented Mar 14, 2024

ros-discourse commented Mar 26, 2024

	RecordOptions get_record_options_from_node_params(rclcpp::Node & node)
	{
	RecordOptions record_options{};
	record_options.all_topics = node.declare_parameter<bool>("record.all_topics", false);
	record_options.all_services = node.declare_parameter<bool>("record.all_services", false);

	record_options.is_discovery_disabled =
	node.declare_parameter<bool>("record.is_discovery_disabled", false);

	record_options.topics = node.declare_parameter<std::vector<std::string>>(
	"record.topics", std::vector<std::string>());

	is_discovery_disabled: true
	topics: ["topic", "other_topic"]
	services: ["service", "other_service"]

	std::vector<std::string> topics {"/topic", "/other_topic"};
	EXPECT_EQ(record_options.topics, topics);
	std::vector<std::string> services {"/service/_service_event", "/other_service/_service_event"};

Filter topic by type #1577

Filter topic by type #1577

Conversation

ahcorde commented Feb 28, 2024

MichaelOrlov left a comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

ahcorde commented Mar 11, 2024

ahcorde commented Mar 11, 2024

MichaelOrlov commented Mar 12, 2024

MichaelOrlov left a comment

Choose a reason for hiding this comment

MichaelOrlov commented Mar 14, 2024

MichaelOrlov commented Mar 14, 2024

MichaelOrlov left a comment

Choose a reason for hiding this comment

ahcorde commented Mar 14, 2024

ros-discourse commented Mar 26, 2024