Sort info output by topic name

[Sanoronas]

Closes #1797

• 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

[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

[fujitatomoya]

as you suggested, this option would be nice to configure via CLI so that user can choose the sorting method as they like. and test would be ideal

[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.

[MichaelOrlov]

For the sake of consistency and more clarity, I would add the args.sort parameter to the print_output_topic_name_only(m) and print a warning inside those function if args.sort != name.

With such approach, it will be clear from the caller level that print_output_topic_name_only(..) also do sorting inside.
Currently, it is not clear and need to go inside the function implementation and check if it is really doing any sort.

[MichaelOrlov]

Please use switch-case instead of if-else with returns.
In case of default: "unknown value" in switch case, please throw std::runtime_error("switch is not exhaustive");

[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]

Sorry, it seems I made a mistake originally putting str in quotes.

Suggested change

	auto find_result = sorting_method_map.find("str");
	auto find_result = sorting_method_map.find(str);

This is proof of why we need unit tests 😉

[MichaelOrlov]

Please use switch-case instead of if-else with returns. As I've mentioned before.
In case of default: "unknown value" in switch case, please throw std::runtime_error("switch is not exhaustive");

[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.