`--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" #1620

Raphtor · 2024-04-22T14:33:36Z

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

The text was updated successfully, but these errors were encountered:

Raphtor · 2024-04-22T14:56:46Z

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 · 2024-05-03T16:30:11Z

@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 · 2024-05-07T03:28:42Z

@MichaelOrlov

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

Barry-Xu-2018 · 2024-05-07T13:34:32Z

@MichaelOrlov

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

rosbag2/ros2bag/ros2bag/verb/record.py

Lines 171 to 172 in e7f7f59

    
           if (args.all and (args.topics or args.regex)) or (args.topics and args.regex): 
        
               return print_error('Must specify only one option out of topics, --regex or --all')

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.

rosbag2/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp

Lines 142 to 148 in e7f7f59

    
           if ( 
        
             !record_options_.all &&  // All takes precedence over regex 
        
             !record_options_.regex.empty() &&  // empty regex matches nothing, but should be ignored 
        
             !std::regex_search(topic_name, include_regex)) 
        
           { 
        
             return false; 
        
           }

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 · 2024-05-07T16:33:47Z

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 · 2024-05-07T16:43:50Z

@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 · 2024-05-07T16:53:59Z

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 · 2024-05-07T18:55:13Z

@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 · 2024-05-07T20:03:48Z

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 · 2024-05-07T20:17:44Z

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 · 2024-05-08T01:25:32Z

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 · 2024-05-08T01:47:32Z

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

Barry-Xu-2018 · 2024-05-08T06:47:40Z

@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 · 2024-05-08T21:18:13Z

i tried with source build ros2/ros2@4d07f58

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 [humble] Resolve recording option problem #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 --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" #1620 (comment))
(same behavior goes to services.)

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

MichaelOrlov · 2024-05-08T21:54:44Z

@fujitatomoya @Barry-Xu-2018 As regards

i tried with source build ros2/ros2@4d07f58
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 Sweep cleanup in rosbag2 recorder CLI args verification code #1633 targeting Rolling.

MichaelOrlov · 2024-05-08T22:00:42Z

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 · 2024-05-08T22:17:24Z

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

Ah 😅 i missed that, thanks!

MichaelOrlov · 2024-05-08T22:41:38Z

@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 · 2024-05-09T01:41:41Z

@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

rosbag2/ros2bag/ros2bag/verb/record.py

Lines 211 to 246 in 0b6a8da

    
           if not self._check_necessary_argument(args): 
        
               return print_error('Need to specify one option out of --all, --all-topics, ' 
        
                                  '--all-services, --services, --topics, --topic-types and --regex') 
        
           # Only one option out of --all, --all-services --services or --regex can be used 
        
           if (args.all and args.all_services) or \ 
        
              ((args.all or args.all_services) and args.regex) or \ 
        
              ((args.all or args.all_services or args.regex) and args.services): 
        
               return print_error('Must specify only one option out of --all, --all-services, ' 
        
                                  '--services or --regex') 
        
           # Only one option out of --all, --all-topics, --topics, --topic-types or --regex can 
        
           # be used 
        
           if (args.all and args.all_topics) or \ 
        
              ((args.all or args.all_topics) and args.regex) or \ 
        
              ((args.all or args.all_topics or args.regex) and args.topics) or \ 
        
              ((args.all or args.all_topics or args.regex or args.topics) and args.topic_types): 
        
               return print_error('Must specify only one option out of --all, --all-topics, ' 
        
                                  '--topics, --topic-types or --regex') 
        
           if (args.exclude_regex and 
        
              not (args.regex or args.all or args.all_topics or args.all_services)): 
        
               return print_error('--exclude-regex argument requires either --all, ' 
        
                                  '--all-topics, --all-services or --regex') 
        
           if args.exclude_topics and not (args.regex or args.all or args.all_topics): 
        
               return print_error('--exclude-topics argument requires either --all, --all-topics ' 
        
                                  'or --regex') 
        
           if args.exclude_topic_types and not (args.regex or args.all or args.all_topics): 
        
               return print_error('--exclude-topic-types argument requires either --all, ' 
        
                                  '--all-topics or --regex') 
        
           if args.exclude_services and not (args.regex or args.all or args.all_services): 
        
               return print_error('--exclude-services argument requires either --all, --all-services ' 
        
                                  'or --regex')

So you get the below output. (Sweep cleanup in rosbag2 recorder CLI args verification code #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

rosbag2/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp

Lines 122 to 124 in 0b6a8da

    
           bool TopicFilter::take_topic( 
        
             const std::string & topic_name, const std::vector<std::string> & topic_types) 
        
           {

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 · 2024-05-14T21:32:00Z

Closing as resolved in the [Humble] Resolve recording option problem (backport #1649) #1651

Raphtor added the bug Something isn't working label Apr 22, 2024

MichaelOrlov added the help wanted Extra attention is needed label Apr 25, 2024

MichaelOrlov mentioned this issue May 3, 2024

Sweep cleanup in rosbag2 recorder CLI args verification code #1633

Open

Barry-Xu-2018 mentioned this issue May 8, 2024

[humble] Resolve recording option problem #1644

Closed

MichaelOrlov linked a pull request May 10, 2024 that will close this issue

[humble] Resolve recording option problem #1644

Closed

Barry-Xu-2018 mentioned this issue May 10, 2024

[Iron] Resolve recording option problem #1649

Merged

mergify bot mentioned this issue May 10, 2024

[Humble] Resolve recording option problem (backport #1649) #1651

Merged

MichaelOrlov closed this as completed May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`--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" #1620

`--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" #1620

Raphtor commented Apr 22, 2024

Raphtor commented Apr 22, 2024 •

edited

MichaelOrlov commented May 3, 2024

Barry-Xu-2018 commented May 7, 2024

Barry-Xu-2018 commented May 7, 2024 •

edited

fujitatomoya commented May 7, 2024

MichaelOrlov commented May 7, 2024

fujitatomoya commented May 7, 2024

MichaelOrlov commented May 7, 2024

fujitatomoya commented May 7, 2024

MichaelOrlov commented May 7, 2024

Barry-Xu-2018 commented May 8, 2024

Barry-Xu-2018 commented May 8, 2024

Barry-Xu-2018 commented May 8, 2024

fujitatomoya commented May 8, 2024

MichaelOrlov commented May 8, 2024

MichaelOrlov commented May 8, 2024

fujitatomoya commented May 8, 2024

MichaelOrlov commented May 8, 2024

Barry-Xu-2018 commented May 9, 2024

MichaelOrlov commented May 14, 2024

--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" #1620

--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" #1620

Comments

Raphtor commented Apr 22, 2024

Description

Expected Behavior

Actual Behavior

To Reproduce

System (please complete the following information)

Raphtor commented Apr 22, 2024 • edited

MichaelOrlov commented May 3, 2024

Barry-Xu-2018 commented May 7, 2024

Barry-Xu-2018 commented May 7, 2024 • edited

fujitatomoya commented May 7, 2024

MichaelOrlov commented May 7, 2024

fujitatomoya commented May 7, 2024

MichaelOrlov commented May 7, 2024

fujitatomoya commented May 7, 2024

MichaelOrlov commented May 7, 2024

Barry-Xu-2018 commented May 8, 2024

Barry-Xu-2018 commented May 8, 2024

Barry-Xu-2018 commented May 8, 2024

fujitatomoya commented May 8, 2024

MichaelOrlov commented May 8, 2024

MichaelOrlov commented May 8, 2024

fujitatomoya commented May 8, 2024

MichaelOrlov commented May 8, 2024

Barry-Xu-2018 commented May 9, 2024

MichaelOrlov commented May 14, 2024

`--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" #1620

`--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" #1620

Raphtor commented Apr 22, 2024 •

edited

Barry-Xu-2018 commented May 7, 2024 •

edited