`--help` mentions that `--regex` is applied on top of topic list, but doing so gives "Must specify only one option out of topics, --regex or --all"

Raphtor commented 6 months ago

Description

Doing ros2 bag record --help says that -e is applied on top of topics list:

>> ros2 bag record --help
usage: ros2 bag record [-h] [-a] [-e REGEX] [-x EXCLUDE] [--include-unpublished-topics] [--include-hidden-topics] [-o OUTPUT]
 ...
  -e REGEX, --regex REGEX
         Record only topics containing provided regular expression. Overrides --all, applies on top of topics list.

Expected Behavior

The topics recorded include both the topic list AND all topics that match the regex.

Actual Behavior

Doing that gives an error.

ros2 bag record -e '\/namespace\/.*\/topic' /topic1 /topic2
[ERROR] [ros2bag]: Must specify only one option out of topics, --regex or --all

Flipping the arguments also gives an error:

To Reproduce

Run ros2 bag record -e '\/namespace\/.*\/topic' /topic1 /topic2

System (please complete the following information)

OS: Ubuntu 20.04
ROS 2 Distro: Humble
Install Method: APT
Version: ros-humble-ros2bag/jammy,now 0.15.8-1jammy.20231205.205704

Raphtor commented 6 months ago

A workaround is just to include the topic list in the regex, e.g ros2 bag record -e (\/namespace\/.*\/topic)|(\/topic1)|(\/topic2) But the documentation shouldn't give false hope.

Additionally, since subscribers are only created after messages get published, its possible to miss messages with from volatile QOS durability publishers if there are no subscribers. However, if they are specified normally in the topic list with --include-unpublished-topics, they are created at the beginning and capture all messages.

For example, consider the three cases below: Case 1: regex with no subscriber

Bag starts recording with -e to specify topics: ros2 bag record -e --include-unpublished-topics (\/namespace\/.*\/topic)|(\/topic1)|(\/topic2)
Node starts up.
Node publishes a single message with volatile durability to /topic.
Bag makes a subscriber to the message
Message is lost.

Case 2: listing topics manually

Bag starts recording, listing topics explicitly: ros2 bag record --include-unpublished-topics /topic1 /topic2
Node starts up.
Node publishes a single message with volatile durability to /topic.
Bag records message.

Case 3: regex with subscriber

Bag starts recording with -e to specify topics: ros2 bag record --include-unpublished-topics -e (\/namespace\/.*\/topic)|(\/topic1)|(\/topic2)
Node A and node B starts up. Node B starts listening to /topic
Node publishes a single message with volatile durability to /topic.
Bag records message.

MichaelOrlov commented 6 months ago

@Raphtor Thanks for reporting this issue. I addressed inconsistency on Rolling in the scope of the #1633. However, on Humble and Iron we have a different implementation in the TopicFilter class for the recorder. i.e., more restrictive. The simplest way to resolve this issue for Humble and Iron would be to adjust the help section in the recorder CLI to the actual behavior. The smarter way to address it would be to try to rewrite the TopicFilter class and unit tests for them in a similar way as we did recently for Rolling in the scope of the services recording feature. To eliminate restrictions "Must specify only one option out of topics, --regex or --all". However, we need to take into account that fix shall not introduce API/ABI breaking changes.

@Barry-Xu-2018 @fujitatomoya I would appreciate someone's help here with fixing this issue on Humble and Iron.

Barry-Xu-2018 commented 6 months ago

@MichaelOrlov

I just got back from vacation. I will investigate and fix this issue.

Barry-Xu-2018 commented 6 months ago

@MichaelOrlov

About this issue, this check is wrong. I will fix it. https://github.com/ros2/rosbag2/blob/e7f7f594651bdc5f4ec0ff541182a0028513a7a5/ros2bag/ros2bag/verb/record.py#L171-L172

Besides, I found the current implementation does not match the parameter description.

The implement is

if all is set, regex is ignored. That is, regex is overridden by --all. https://github.com/ros2/rosbag2/blob/e7f7f594651bdc5f4ec0ff541182a0028513a7a5/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp#L142-L148

But the description of argument -e is

 -e REGEX, --regex REGEX
         Record only topics containing provided regular expression. Overrides --all, applies on top of topics list.

There are two ways to fix it. One is to modify parameter description based on current implementation. The other is to modify the implementation based on parameter description. I would like to hear your opinion.

fujitatomoya commented 6 months ago

About this issue, this check is wrong. I will fix it.

👍

There are two ways to fix it. One is to modify parameter description based on current implementation. The other is to modify the implementation based on parameter description. I would like to hear your opinion.

IMO that would be useful when regex (exclude-regex) overrides the all or topic lists.

MichaelOrlov commented 6 months ago

@fujitatomoya As regards

IMO that would be useful when regex (exclude-regex) overrides the all or topic lists.

Could you please clarify a use case when it would be useful when --regex overrides --all or --topics?

IMO exclude-regex yes make sense but regular --regex not.

@Barry-Xu-2018 I want to be consistent with the Rolling as much as possible in regards to the behavior.

fujitatomoya commented 6 months ago

Could you please clarify a use case when it would be useful when --regex overrides --all or --topics?

i thought the following cases,

--all can allow user to record all topics with itself. (if they want all, this can be just specified.)
--topics can allow user to record specified topics with it.

and the,

--regex to narrow down the topics specified from --all (in this case, same with only --regex?) and --topics with expression parameters.
--exclude-regex to exclude the topics specified from --all (in this case, same with only --exclude-regex?) and --topics with expression parameters.

MichaelOrlov commented 6 months ago

@fujitatomoya I would disagree with

--regex to narrow down the topics specified from --all (in this case, same with only --regex?) and --topics with expression parameters.

Regrex is a regular expression that defines a pattern of what to select by itself, and it really contradicts with --all. if one messed up and uses them together, -all shall override --regex because --all must really mean select all, then exclusions from selections could be applied. Otherwise, --all does not really mean select all, and I would consider it as a bug.

fujitatomoya commented 6 months ago

So --all prevails to anything else except exclusion, i am good to go with that.

what about the --topics then..., it is clear that user specifies the topic lists that they want to record, so neither --regex nor --exclude-regex can be applied? Or expressions apply on top of topics list as help message prints.

MichaelOrlov commented 6 months ago

Ideally --topics shall be possible to combine with the --regex as we did on Rolling. Why not? For instance, one may want to record topic1, topic2, and all other topics with the name transform_ in their name. However, I don't know what the current implementation is on Iron and Humble on top of my head. Of course, --exclude-regex can be applied on top of them.

Barry-Xu-2018 commented 6 months ago

I want to be consistent with the Rolling as much as possible in regards to the behavior.

Yes. I will confirm the behavior of the Rolling version and then modify the code to match its behavior.

Barry-Xu-2018 commented 6 months ago

For Rolling, '--all' and '--all-topics' overrides '--regex'. So I will change code to follow this behavior.

Barry-Xu-2018 commented 6 months ago

@Raphtor

Expected Behavior The topics recorded include both the topic list AND all topics that match the regex.

This is not correct. Please note that applies on top of topics list is in the description of -e. Topic should be in topic list and then check if it matches the regex. I confirm that current implementation in the humble version is as this. BTW, Iron version has the same issue. From the Jazzy version, the expected behavior you described is implemented.

fujitatomoya commented 6 months ago

i tried with source build https://github.com/ros2/ros2/commit/4d07f58591920794676261a14c2ba9d0a7bf2746

Ideally --topics shall be possible to combine with the --regex as we did on Rolling.

@MichaelOrlov

this is not implemented in rolling? they are exclusive options.

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag record -e '\/topic*' --topics /topic1 /topic2
[ERROR] [ros2bag]: Must specify only one option out of --all, --all-topics, --topics, --topic-types or --regex
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag record -e '\/topic*' /topic1 /topic2
Warning! Positional "topics" argument deprecated. Please use optional "--topics" argument instead.
[ERROR] [ros2bag]: Must specify only one option out of --all, --all-topics, --topics, --topic-types or --regex

Please note that applies on top of topics list is in the description of -e. Topic should be in topic list and then check if it matches the regex.

@Barry-Xu-2018

it seems that --regex does not apply on top of the --topics or [topics] either... according to above result?

i think the expected behavior from the discussion is,

--all / --all-topics prevails --regex. (this is addressed with https://github.com/ros2/rosbag2/pull/1644)
--exclude-regex can be applied on top of --all / --all-topics / --topics / [topics].
--regex can be combined with --topics / [topics]. (not applied on top of it, see https://github.com/ros2/rosbag2/issues/1620#issuecomment-2099229474)
(same behavior goes to services.)

what do you think? after all, we need to modify rolling to comply this behavior?

MichaelOrlov commented 6 months ago

@fujitatomoya @Barry-Xu-2018 As regards

i tried with source build https://github.com/ros2/ros2/commit/4d07f58591920794676261a14c2ba9d0a7bf2746 Ideally --topics shall be possible to combine with the --regex as we did on Rolling. @MichaelOrlov this is not implemented in rolling? they are exclusive options.

Sorry for the confusion; it's too much on my plate in recent days. When I said Ideally --topics shall be possible to combine with the --regex as we did on Rolling.

I actually meant as I made in the https://github.com/ros2/rosbag2/pull/1633 targeting Rolling.

MichaelOrlov commented 6 months ago

As regards the

what do you think? after all, we need to modify rolling to comply this behavior?

The rolling modification already done in the #1633. However, it is still in review.

fujitatomoya commented 6 months ago

The rolling modification already done in the https://github.com/ros2/rosbag2/pull/1633. However, it is still in review.

Ah 😅 i missed that, thanks!

MichaelOrlov commented 6 months ago

@Raphtor As regards

Please note that applies on top of the topics list is in the description of -e. Topic should be in topic list and then check if it matches the regex. I confirm that current implementation in the humble version is as this. BTW, Iron version has the same issue. From the Jazzy version, the expected behavior you described is implemented.

In particular, "Topic should be in topic list and then check if it matches the regex"

That is not very user-friendly, and we found it tedious to find a correct regex, which should also include topics from the topic list. For instance, as mentioned above, one may want to record topic-1, topic2a, and all other topics with the name transform_ in their name via --regex. In this case, writing regex, which will select topics from the topic list and also topics with transform_ in their name, will be a non-trivial and challenging task. Also, it defeats the option of combining the topic list with regex if regex must select topics from the topic list.

We've changed this behavior in the #1480 and #1585. And starting from Jazzy --regex will be applied if the topic is not found in the topic list. i.e. regex applying in addition to the topic list.

Barry-Xu-2018 commented 6 months ago

@fujitatomoya

it seems that --regex does not apply on top of the --topics or [topics] either... according to above result?

Yes, this is current behavior. As MichaelOrlov said, it will be changed by #1633 which is under review.
In the code, there are two places related to user input parameters.

Check user input parameters. The code is located at https://github.com/ros2/rosbag2/blob/0b6a8dacef1a7de0117a48502c987209830f5e96/ros2bag/ros2bag/verb/record.py#L211-L246 So you get the below output. (https://github.com/ros2/rosbag2/pull/1633 revised these codes)

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag record -e '\/topic*' --topics /topic1 /topic2
[ERROR] [ros2bag]: Must specify only one option out of --all, --all-topics, --topics, --topic-types or --regex
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag record -e '\/topic*' /topic1 /topic2
Warning! Positional "topics" argument deprecated. Please use optional "--topics" argument instead.
[ERROR] [ros2bag]: Must specify only one option out of --all, --all-topics, --topics, --topic-types or --regex

Decide whether to record the topic/service based on user parameters. The codes are located at this function https://github.com/ros2/rosbag2/blob/0b6a8dacef1a7de0117a48502c987209830f5e96/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp#L122-L124

For Humble and Iron, it should support that --regex and --topics / [topics] are set at the same time. This bug is fixed by #1644. Current implementation is not user-friendly while user input --regex and topic list at the same time. --regex have to be applied on filtered topic by topic list.

From Jazzy and rolling, TopicFilter::take_topic() has been updated to remove user-unfriendly behavior. But check user input parameter has the issue you mentioned. It will be fixed at #1633.

MichaelOrlov commented 5 months ago

Closing as resolved in the #1651

ros2 / rosbag2