Allow creating multiple channels

[devashravan]

Description

These changes will allow users to configure multiple channels in a tracing session.

Trace(
    session_name='single-channel-session',
    events_ust=['ros2:rcl_init', 'ros2:rcl_node_init'],
)

This will create one channel with the UST events ros2:rcl_init and ros2:rcl_node_init enabled. Whereas if a list of lists is provided for events_ust, example:

Trace(
    session_name='multi-channel-session',
    events_ust=[['ros2:rcl_init', 'ros2:rcl_node_init'], ['ros2:rcl_init']],
)

2 channels are be created with UST events ros2:rcl_init,ros2:rcl_node_init enabled in one channels and ros2:rcl_init enabled in another channel.

Fixes #199

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

This PR adds this functionality only to python launch files and not to xml files, yaml files, ros2trace cli, substitutions

[devashravan]

@christophebedard Can you please review this?

[christophebedard]

The type annotations for events_ust and events_kernel need to be updated; the typing-related test failures you see are probably related to this. However, it kind of highlights an issue I hadn't thought of, and I'm not quite sure how to solve it cleanly. I apologize for the long comment, but I think some context/explanation might be helpful.

---

Currently, events_ust is Optional[Iterable[SomeSubstitutionsType]]. It's kind of tricky, because SomeSubstitutionsType can be a single string/substitution, or it can be an iterable of strings/substitutions: https://github.com/ros2/launch/blob/1abf55ef6d3874ab5a4656e9ac0ea5d057c3c541/launch/launch/some_substitutions_type.py#L25-L30. If it's just a single string/substitution, then that's just 1 value, i.e., one event name. If it's an iterable, then it's still just 1 value: strings and substitutions (after being resolved) in the iterable will get concatenated into a single string. This is how substitutable inputs work.

Then, Iterable[SomeSubstitutionsType] means we have a list of values (where, again, each "value" can be a single string or an iterable of strings/substitutions that gets resolved and concatenated into a single value). normalize_to_list_of_substitutions() converts each SomeSubstitutionsType in that Iterable into a list of substitutions.

Since we want to support providing a list of this, we'd need something like List[Iterable[SomeSubstitutionsType]] (or Iterable[Iterable[SomeSubstitutionsType]]). The issue is that we don't know if it's a list of lists of individual values (single substitutions) or a list of lists that each represents a single value. Note that the output after normalization is definitely List[List[List[Substitution]]], though, like you did:

List[  # list of lists of events
    List[  # list of events
        List[Substitution]  # single event (potentially made up of multiple substitutions that get resolved and concatenated into a single value, i.e., a single event name)
    ]
]

To illustrate this, these two lists (before this PR) are functionally equivalent after normalization and substitution resolution:

events_ust_a = [
    'ros2:*',
    'ros2:rcl_publish',
    'ros2:rclcpp_publish',
]
events_ust_b = [
    'ros2:*',  # or ['ros2:*']
    ['ros2:rcl_publish'],
    ['ros2:', 'rclcpp_publish'],
]

With this PR, we'd like to do this:

events_ust_c = [
    [  # one list of events for a channel
        'ros2:*',
        ['ros2:rcl_publish'],
        ['ros2:', 'rclcpp_publish'],
    ],
    [  # another list of events for another channel
        'ros2:rmw_take',
        'ros2:rcl_take',
    ],
]

The problem is: How can you know if events_ust_c is a list of lists of events and not a list of lists of substitutions like events_ust_b? ['ros2:*', ['ros2:', 'rcl_publish']] is a single list of event names, but all(not isinstance(x, Iterable) or isinstance(x, (str, Generator)) for x in ['ros2:*', ['ros2:', 'rcl_publish']]) is False, meaning that the proposed code thinks it's actually a list of lists of events, when it's actually a single list of events.

An option might be to use sets to have separate lists of events, then you could do this:

if events_ust and not isinstance(events_ust, set):
    # Single list
     events_ust = [events_ust]
else:
    # Multiple lists
    events_ust = list(events_ust)

However, there's a fundamental flaw with this. We currently accept an Iterable for events_ust, so a single "list" of events might actually be a single set of events, since a set is an Iterable. I'm not quite sure how to solve this in a clean and backwards-compatible way, but a 100% safe option would be to have a separate input for multiple (channel-specific) lists of events, e.g., events_ust_channels. If events_ust_channels is used (not None and not empty), then events_ust is ignored. This way, there is no doubt about whether the input is a list of lists of events or a single list of events. I don't like having a separate option, but since this is a more "advanced" feature, it might be acceptable.

What do you think? Did I miss anything?

[devashravan]

How about using dictionaries? events_ust (and events_kernel) can be of the type Union[Iterable[SomeSubstitutionType], dict[str, Iterable[SomeSubstitutionType]]].

I am thinking to implement something like this:

if not isinstance(events_ust, dict):
    # single list is provided
    events_ust = {'default_cannel_name', events_ust}

for single channel:

Trace(
    session_name='single-channel-session',
    events_ust=['ros2:rcl_init', 'ros2:rcl_node_init'],
)

for multiple channels:

Trace(
    session_name='multi-channel-session',
    events_ust={'channel1': ['ros2:rcl_init', 'ros2:rcl_node_init'], 'channel2': ['ros2:rcl_init']},
)

Am I missing something??

[christophebedard]

Thanks for bringing this to my attention again during the ROS-ffice hours!

I like this idea, because it also lets you set the channel name, so someone could even use it for a single list of events just to set the channel name! I think there are two things missing:

First thing: Like I mentioned during the meeting, we should support this through the XML/YAML frontends too. Right now, I think we just expect a space-separated list of events, e.g., events-ust="ros2:* *". Users can also provide substitutions, e.g., events-ust="$(var events-ust-1) $(var events-ust-2)". However, I remember realizing last year that we don't expect users to provide multiple (space-separated) events in a single substitution; we just expect one channel name per substitution.

Considering this, we could easily figure out if the user provided a string dictionary by simply detecting that events-ust starts with a { and then try to parse the string as a dictionary. Each dictionary item would point to a single string that's a space-separated list of events/substitutions: events-ust="{'channel1': 'event1 event2 event3', 'channel2': 'event4 $(var other-event)']}". You'd need to first convert the whole events-ust string into an actual dictionary, and then you could parse the space-separated lists of events using _parse_cmdline() just like we do right now. You could create a separate function in a separate file to do all of this parsing and add a test for it.

It would be good to support substitutions for channel names (events-ust="{'$(var channel-name)': 'event1 event2', 'channel2': 'event3 $(var other-event)']}") if we can have substitutions as a dictionary key (dict[SomeSubstitutionType, Iterable[SomeSubstitutionType]]), but we can do that later.

Second thing: Since sub-buffer sizes are set per-channel, if we allow users to create multiple channels, we should allow users to also set sub-buffer sizes for each channel. We could use a similar syntax with Python dictionaries or string dictionaries for XML/YAML frontends, so this should be doable. This can be done in a later PR!

[christophebedard]

Since this is technically public API, we should avoid potentially breaking downstream users, so I would still allow users to provide a simple list/set of events: Union[List[str], Set[str], dict[str, List[str]]]. And then we can keep the default value as DEFAULT_EVENTS_ROS/[] and not need the empty string dictionary key.

Also, I would keep the current channel_name_ust/channel_name_kernel names but document that the channel names may be provided through ros_events/kernel_events.