Add "--sort" CLI option to the "ros2 bag info" command

[Sanoronas]

This PR adds the "--sort" CLI option to the "ros2 bag info" command. With "--sort" CLI option user will be able to sort topics and services by name, topic type or number of recorded messages.

• generate sorted indices for topics from multiple different options (topic name, type, count or serialization format)
• defaults to sorting by topic name
• use sorted indices rather than using strict increasing indices for printing out the topics
• potential use of different sorting options through CLI by passing sort_method argument to format_topics_with_type function

• Closes Sort topics by name when executing ros2 bag info #1797

[fujitatomoya]

it would be nice if format_service_with_type has the same sorting interface aligned with topics.

this PR is failing to build with the following error and DCO is missing. could you address them?

07:59:08 /tmp/ws/src/rosbag2/rosbag2_py/src/rosbag2_py/format_bag_metadata.cpp:144:8: error: ‘iota’ is not a member of ‘std’
07:59:08   144 |   std::iota(sorted_idx.begin(), sorted_idx.end(), 0);
07:59:08       |        ^~~~
07:59:08 gmake[2]: *** [CMakeFiles/_storage.dir/build.make:90: CMakeFiles/_storage.dir/src/rosbag2_py/format_bag_metadata.cpp.o] Error 1
07:59:08 gmake[1]: *** [CMakeFiles/Makefile2:284: CMakeFiles/_storage.dir/all] Error 2
07:59:08 gmake: *** [Makefile:146: all] Error 2

[MichaelOrlov]

@Sanoronas Thank you for your contribution.
I agree with @fujitatomoya that sorting by the serialization format is unlikely to be useful. Because we are not supporting different serialization formats during recording by design.
I can only envision a possibility to have a different serilization formats if someone will convert the already recorded bag or record with modifyied rosbag2 recorder or using some different tool for recording.
Therefore I think it makes sense to have options for sorting:

1. By topic or service name.
2. By topic or service type
3. By topic or service messages count

Could you please use C++ enum values for the sorting method?
Also, It will be appreciated if you would add the same sorting algorithm for the services in the format_service_with_type(..) function located in the same file.

[Sanoronas]

Thanks for the feedback @MichaelOrlov and @fujitatomoya

I added --sort option for CLI, handling sorting of service topics and moved the sorting options to enum.

[MichaelOrlov]

@Sanoronas, Thank you for addressing comments from the previous review and for adding a CLI option for the sorting method.

I've put some comments and requests for code changes. Also, in addition to that, could you please add sorting to the format_service_info(..) function from the format_service_info.cpp?

rosbag2/rosbag2_py/src/rosbag2_py/format_service_info.cpp

Lines 80 to 86 in cd7bd63

	print_service_info(service_info_list[0]);
	auto number_of_services = service_info_list.size();
	for (size_t j = 1; j < number_of_services; ++j) {
	info_stream << std::string(indentation_spaces, ' ');
	print_service_info(service_info_list[j]);
	}

The same way you did for services in another place.
For sorting by "count" you can use the sum of the si->request_count and si->response_count.
To add sorting for the format_service_info.cpp, the declaration of the enum class SortingMethod will need to be moved to a separate header file, which makes sense anyway.
Also, I would like to ask you to create the following info_sorting_method_from_string(str) helper function

SortingMethod info_sorting_method_from_string(std::string str)
{
  static std::unordered_map<std::string, SortingMethod> sorting_method_map = {
    {"name", SortingMethod::NAME},
    {"type", SortingMethod::TYPE},
    {"count", SortingMethod::COUNT}};
  std::transform(str.begin(), str.end(), str.begin(), ::tolower);
  auto find_result = sorting_method_map.find("str");
  if (find_result == sorting_method_map.end()) {
    throw std::runtime_error("Enum value match for \"" + str + "\" string is not found.");
  }
  return find_result->second;
}

Use it instead of rosbag2_py::stringToSortingMethod[sorting_method]; whenever conversion from string to enum is needed.
You can put a declaration of those info_sorting_method_from_string(str) function to the same header file with the enum class. However, the definition should be in the cpp file to not violate the C++ ODR rule. Will need to create one more InfoSortingMethod.cpp file for that.

And the last but not least request is to create a few unit tests to cover all these changes.
I know, creating integration unit tests from scratch sounds like a tedious and non-trivial task.
But we have got covered you 😉
We already have integration tests for the ros2 bag info. However, they are in a different package. You can find them in the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.cpp.
All the infrastructure and sample bag files for tests already exist. Perhaps it will not be so difficult to write a few new tests similar to the existing ones.

[Sanoronas]

@MichaelOrlov , thanks for your input!

I addressed some of your points in the recent commit (missing switch-case and tests).
However, I was unable to regenerate stub-files and cannot track down the reason for this right now. Any help on that regard would be helpful.

[MichaelOrlov]

@Sanoronas Have you tried instruction from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/README.md?

[Sanoronas]

@MichaelOrlov Yes, they were working fine until the recent changes

[MichaelOrlov]

@Sanoronas Thanks for addressing most of my previous comments.

I don't think that moving ServiceInfo with ServiceMetadata to the rosbag2_storage package is a good idea because it will cause a lot of confusion about why it is there if it is not used in any way in those package.
I belive we can do better.

First of all, there is some confusion about naming after the implementation of services support in Rosbag2. It seems I overlooked it on the review back then.
We already have a definition for the rosbag2_service_info_t in the rosbag2_cpp package, where we distinguish the service's requests and responses.
At the same time, we had a similar structure, ServiceInfo, in the rosbag2_py package defined in the format_bag_metada.cpp. However, in the ServiceInfo, we consolidate the service's requests and responses into the one event_message_count field. In the Service Introspection feature, we name it as ServiceEvents.

To clean up the mess, we need to do the following:

1. Rename ServiceInfo to the ServiceEventInfo to avoid further misunderstanding and confusion.
2. Move declaration of the ServiceMetada and ServiceEventIno in the separate header file ServiceEventInfo.hpp and put it inside rosbag2_py/src, since we are not going to use it somewhere outside of the rosbag2_py package. This way, you will be able to refer to it from the format_bag_metadata.cpp and from the newly created info_sorting_method.hpp{cpp} files.

[MichaelOrlov]

@Sanoronas As regards:

However, I was unable to regenerate stub-files and cannot track down the reason for this right now. Any help on that regard would be helpful.

I've tried to checkout your latest version and follow up the instruction from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/README.md
and I've got error message when trying to accomplish step

Make sure rosbag2_py can be imported
python3 -c 'import rosbag2_py'

The error message is

 python3 -c 'import rosbag2_py'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/morlov/ros2_rolling_new/install/rosbag2_py/lib/python3.10/site-packages/rosbag2_py/__init__.py", line 35, in <module>
    from rosbag2_py._storage import (
ImportError: /home/morlov/ros2_rolling_new/install/rosbag2_py/lib/python3.10/site-packages/rosbag2_py/_storage.cpython-310-x86_64-linux-gnu.so: undefined symbol: _ZN10rosbag2_py19generate_sorted_idxERKSt6vectorISt10shared_ptrIN15rosbag2_storage18ServiceInformationEESaIS4_EENS_17InfoSortingMethodE

I think this error message will go away as soon as you will move ServiceInformation type decalration back to the rosbag2_py package as I advised you above. If not, please let me know; I will try to reiterate and help you with pyi stubs regeneration.
Please note that pyi regeneration is usually the last step when all other changes in the rosbag2 packages have already been done.

[Sanoronas]

@MichaelOrlov
Moving ServiceEventInformation inside rosbag2_py didn't help with the pybind-issue. I think it has to do with linking of the source code somehow as I was able to change the import error by adding a resource to _storage element in CMakeLists.
However, I was unable to fully resolve the issue. For now it is working when the implementation of generate_sorted_idx is done directly in format_service_info.cpp. All other locations somehow are fine with the function being included, but not at this position.
If you have an idea what to change please let me know. Having all sorting functions in one file is probably better in the long-run.

Your other comments should also be resolved now.

Regarding tests I used the existing resources for now. For more detailed tests we would probably need to create another resource with differing count, types and names to properly distinguish the available options.

[MichaelOrlov]

@Sanoronas I've managed to fix linker issues and was able to regenerate pyi stubs.
I also made small cleanups by moving things around and applying small nitpicks.
Please see my three latest commits on your branch.

However, I didn't have a chance to review unit tests yet.

[MichaelOrlov]

@ros-pull-request-builder retest this please

[MichaelOrlov]

@Sanoronas Ok. I reviewed the tests.
Overall looks good. However, it will be more useful to use bag_with_topics_and_service_events.mcap{db3} files as test files, since they have many topics and service events. While the cdr_test.mcap{db3} has only two topics.

Could you please rewrite tests to use bag_with_topics_and_service_events.mcap{db3} files to have better coverage?

[Sanoronas]

@MichaelOrlov Thanks for your fixes!

I chose cdr_test.mcap{db3} over bag_with_topics_and_service_events.mcap{db3} as it has unordered topic names and differing count and types for the contained topics.
Testing ordering on bag_with_topics_and_service_events.mcap{db3} is probably not as useful as ordering by name, count and type will always be the same result.

[MichaelOrlov]

Pulls: #1804
Gist: https://gist.githubusercontent.com/MichaelOrlov/8e4617bc24aa9c31e2f2ca79a536429f/raw/c5e2c9724054c21164179d235978ac8bed2b7403/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14712

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[MichaelOrlov]

https://github.com/Mergifyio backport jazzy

[mergify]

backport jazzy

✅ Backports have been created

• #1838 Add "--sort" CLI option to the "ros2 bag info" command (backport #1804) has been created for branch jazzy but encountered conflicts

[MichaelOrlov]

It looks like we are changing API here, which is publicly exposed. Likely we can't backport this PR.
@clalancette Could you please give a second opinion on that?

[clalancette]

I'm not totally familiar with what is going on here, but I can offer a couple of thoughts:

1. It looks to me that the API break is only in adding an additional parameter to print_output, print_output_topic_name_only, and print_output_verbose. Given that this is Python, can we give that final parameter a default value, so that anything depending on the "old" API will continue to work?
2. The intent of the "no breaks in API" rule is so that third-party code that depends on these APIs continue to work across updates to the core. If these are considered entirely internal APIs (i.e. there are no external users), then it is probably acceptable to break API here.

[MichaelOrlov]

@clalancette Thank you for the information I will take it into consideration in the backporting PR #1838.