Use enum values for offered_qos_profiles in code and string names in serialized metadata

[roncapat]

Fixes: #1475

[roncapat]

@MichaelOrlov I see one issue here:
offered_qos_profiles field of metadata.yaml until now used the numerical encoding of the policies. This means that we may "break with the past" here.
We may make the parsing support both numbers & strings to make this retro-compatible with existing rosbags, but newer ones would be saved with policies encoded as strings and not numbers. Thus, an older release may not be able to read a rosbag from a newer release.

[MichaelOrlov]

@roncapat The proper way to handle such a situation with backward compatibility support is to increment BagMetadata::version

rosbag2/rosbag2_storage/include/rosbag2_storage/bag_metadata.hpp

Lines 43 to 45 in a80adfd

	struct BagMetadata
	{
	int version = 8; // upgrade this number when changing the content of the struct

field in data structure.
And will need to keep old tests for the old *8 version and create new tests for a newer version where offered_qos_profiles encoded as string names.

[roncapat]

@MichaelOrlov where is the piece of code that, based on this metadata version tag, handles different metadata formats?

Edit: it seems to me there is no centralized handling for this. Instead, around the repo, there are switches like:

  if (metadata_.version == 4) {
...
  }

when needed. Am I right?

[roncapat]

The problem is that:

/// Pass metadata version to the sub-structs of BagMetadata for deserializing.
/**
  * Encoding should always use the current metadata version, so it does not need this value.
  * We cannot extend the YAML::Node class to include this, so we must call it
  * as a function with the node as an argument.
  */
template<typename T>
T decode_for_version(const Node & node, int version)
{
  static_assert(
    std::is_default_constructible<T>::value,
    "Type passed to decode_for_version that has is not default constructible.");
  if (!node.IsDefined()) {
    throw TypedBadConversion<T>(node.Mark());
  }
  T value{};
  if (convert<T>::decode(node, value, version)) {
    return value;
  }
  throw TypedBadConversion<T>(node.Mark());
}

template<>
struct convert<rosbag2_storage::TopicMetadata>
{
  static Node encode(const rosbag2_storage::TopicMetadata & topic)
  {
    Node node;
    node["name"] = topic.name;
    node["type"] = topic.type;
    node["serialization_format"] = topic.serialization_format;
    node["offered_qos_profiles"] = topic.offered_qos_profiles;
    node["type_description_hash"] = topic.type_description_hash;
    return node;
  }

  static bool decode(const Node & node, rosbag2_storage::TopicMetadata & topic, int version)
  {
    topic.name = node["name"].as<std::string>();
    topic.type = node["type"].as<std::string>();
    topic.serialization_format = node["serialization_format"].as<std::string>();
    if (version >= 4) {
      topic.offered_qos_profiles = node["offered_qos_profiles"].as<std::string>();
    } else {
      topic.offered_qos_profiles = "";
    }
    if (version >= 7) {
      topic.type_description_hash = node["type_description_hash"].as<std::string>();
    } else {
      topic.type_description_hash = "";
    }
    return true;
  }
};

keeps the offered_qos_profiles as serialized string. Then, when the string is parsed in other parts of the code, the version is no more available as info.

So why don't we change TopicMetadata.offered_qos_profile to a deserialized structure instead of a string and do the deserialization in the function above?

[MichaelOrlov]

@roncapat I see the problem.
The reason why we don't parse qos_profiles when we are reading or getting it from yaml is that we are saving it later on in the storage as a separate entity either in the database table in the case of the SQlite3 or in the separate metadata item in the case of the MCAP and we need it in the serialized form.
See

rosbag2/rosbag2_storage_mcap/src/mcap_storage.cpp

Lines 813 to 820 in a80adfd

	if (channel_it == channel_ids_.end()) {
	mcap::Channel channel;
	channel.topic = topic.name;
	channel.messageEncoding = topic_info.topic_metadata.serialization_format;
	channel.schemaId = schema_id;
	channel.metadata.emplace("offered_qos_profiles",
	topic_info.topic_metadata.offered_qos_profiles);
	channel.metadata.emplace("topic_type_hash", topic_info.topic_metadata.type_description_hash);

and

rosbag2/rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp

Lines 461 to 473 in a80adfd

	std::lock_guard<std::mutex> db_lock(database_write_mutex_);
	if (topics_.find(topic.name) == std::end(topics_)) {
	auto insert_topic =
	database_->prepare_statement(
	"INSERT INTO topics"
	"(name, type, serialization_format, offered_qos_profiles, type_description_hash) "
	"VALUES (?, ?, ?, ?, ?)");
	insert_topic->bind(
	topic.name,
	topic.type,
	topic.serialization_format,
	topic.offered_qos_profiles,
	topic.type_description_hash);

However, we can reconsider this approach and serialize it one more time when we need to save it.
There shouldn't be a concern regarding performance since we are saving the topic's metadata not so often and usually at the beginning of the recording.
Another alternative approach would be handling Metadata version on the upper levels such as Player and Recorder classes where we are currently deserializing and serializing TopicMetadata.offered_qos_profile. That will likely involve API changes as well.

@roncapat If you would address all required changes when changing TopicMetadata.offered_qos_profile to a deserialized structure I would support it and help as I can.

[MichaelOrlov]

@roncapat There are one caveate.
If we will be deserializing qos_profiles on the storage plugging level we will need to serialize-deserialize it in old format with integers enum values instead of the stringified names to be able to achieve backward compatability. Please note that for Metadata we can use the new serialization with stringified names at the same time.

[roncapat]

I've just pushed some -ugly but may work- code. As I'm working from an old laptop, I need some help from the GH CI, since the build & test jobs are heavy.

I did some sub-optimal stuff with templates to manage the version, but I would like then to refactor everythin to use decode_for_version() strategy used in rosbag2_storage/yaml.hpp.

[roncapat]

@MichaelOrlov I think I avoided the problem via adding one version field to the TopicMetadata. This field is filled when parsing a metadata.yaml file.
Then, basically, is all YAML conversion code (I mimicked the decode_for_version() pattern widely used in rosbag2_storage).

Care to review?

[MichaelOrlov]

@roncapat I would like to review, but I am afraid that I will not have the capacity in the next 3-5 days.

[roncapat]

Don't worry :) I'm also a bit overloaded by work!

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-10-12/34178/1

[roncapat]

@MichaelOrlov may I ask you if you have any update on this?

[MichaelOrlov]

@roncapat Sorry, I was a bit busy with other tasks than it was a ROSCon time...
Getting back to business and going to review it one more time.

[MichaelOrlov]

@roncapat I briefly reviewed your latest changes with inserting bag metadata version to the TopicMetadata.
I would say that this is not the way I would prefer to do it.
It looks like a hacky way with workarounds. I don't like to keep the upper level of the bag metadata version in the embedded TopicMetadata and use it as a flag to "patch" a code in multiple places.

The better and more efficient way would be to adhere to my first suggestion when we would change data type for offrered_qos_profiles in the TopicMetadata to be as rclcpp::QoS since Rosbag2QoS is just a tiny wrapper. And do parsing from yaml string to rclcpp::QoS when we are parsing the rest of fields on metadata readout, serializing it one more time on the rosbag2 storage plugins layer when we do need to save them to the storage.

There are no advantages in the currently implemented approach since adding the version field to the TopicMetadata already making breaking API and ABI changes.

[roncapat]

No problem, as soon as I have some more time I'll try to follow the proposed approach. Thanks for the review.

[roncapat]

Ok, I'm trying to reimplement as requested.

It seems to me that rosbag2_storage will need to depend on rclcpp (for rclcpp::QoS) and implement YAML adapter for rclcpp::QoS (not default constructible, so a thin wrapper class is needed, again like Rosbag2QoS).
However, the YAML adapter code for rosbag2_transport::Rosbag2QoS is defined in rosbag2_transport.
Making rosbag2_storage depend on rosbag2_transport would lead to a circular dependency.

May we move the rosbag2_transport::Rosbag2QoS to rosbag2_storage::Rosbag2QoS to invert the dependency order? I wouldn't like to just duplicate the implementation, better to move IMHO.

[MichaelOrlov]

@roncapat I am thinking about if we would have two data members for the offrered_qos_profiles in the TopicMetadata?
One serialized and another one parsed to the enum. In this case we will not need to serialize it on the storage plugins layer.

[roncapat]

I don't understand what we would solve then. If I understood correctly, the desired approach is to deserialize/decode everything in one place. To deserialize, we need parse code currently found in rosbag2_transport/qos.hpp.

[MichaelOrlov]

@roncapat As regards:

May we move the rosbag2_transport::Rosbag2QoS to rosbag2_storage::Rosbag2QoS to invert the dependency order? I wouldn't like to just duplicate the implementation, better to move IMHO.

Yes, I agree. It would be better to move it down somewhere to the rosbag2_storage or rosbag2_cpp layer.

[roncapat]

@roncapat I am thinking about if we would have two data members for the offrered_qos_profiles in the TopicMetadata? One serialized and another one parsed to the enum. In this case we will not need to serialize it on the storage plugins layer.

Ahhh undestood now, sorry but I lost the match between problem/solution comments. Here you were trying to solve the re-serialization need for storage plugins, nothing related to QoS parsing code location.

[roncapat]

Just to recap what's going on in the last WIP commits:

• Rosbag2QoS (& related YAML conversion code) moved to rosbag2_storage
• Everything in metadata is getting decoded in rosbag2_storage
• Trying to make everything build
• Still in doubt whether to use rclcpp::QoS everywhere vs. Rosbag2QoS
  • At least for now (draft code!) multiple points in code where std::vector<rclcpp::QoS> needs to be casted to std::vector<rosbag2_storage::Rosbag2QoS>, and casting vectors requires 3-4 lines of code every time. It may be the case to define functions to do it, or go and change more code to move to rclcpp::QoS in the highest number of locations we can.

[roncapat]

Hit a problem in rclcpp_py... that would be exactly what I need but it was never implemented per this comment:

  // NOTE: it is non-trivial to add a constructor for PlayOptions and RecordOptions
  // because the rclcpp::QoS <-> rclpy.qos.QoS Profile conversion cannot be done by builtins.
  // It is possible, but the code is much longer and harder to maintain, requiring duplicating
  // the names of the members multiple times, as well as the default values from the struct
  // definitions.

EDIT: I failed to understand the comment. I think to have implemented the binding correctly...

[roncapat]

@MichaelOrlov in order to make some tests work, like metadata_reads_v4_fills_offered_qos_profiles in rosbag2_storage, I need to implement also version-depedent encode of TopicMetadata. The point is, in my understanding this is useful only to make tests, but in general there is no need to retain write support for old metadata versions in rosbag2 itself. What is the best approach here?

During the mentioned test, a temporary file is generated like this one:

rosbag2_bagfile_information:
  version: 4
  storage_identifier: ""
  duration:
    nanoseconds: 0
  starting_time:
    nanoseconds_since_epoch: 0
  message_count: 0
  topics_with_message_count:
    - topic_metadata:
        name: topic
        type: type
        serialization_format: rmw
        offered_qos_profiles: "- history: keep_last\n  depth: 1\n  reliability: reliable\n  durability: volatile\n  deadline:\n    sec: 0\n    nsec: 0\n  lifespan:\n    sec: 0\n    nsec: 0\n  liveliness: system_default\n  liveliness_lease_duration:\n    sec: 0\n    nsec: 0\n  avoid_ros_namespace_conventions: false"
        type_description_hash: ""
      message_count: 100
  compression_format: ""
  compression_mode: ""
  relative_file_paths:
    []
  files:
    []
  custom_data: ~
  ros_distro: ""

as you see, it is a V4 metadata, but I'm currently encoding disregarding version, hence, the "keep last" string and alike.

[roncapat]

I'm checking both serialization & deserialization code paths.

For serialization, I may be wrong but:

std::string serialize_rclcpp_qos_vector(const std::vector<rclcpp::QoS> & in, int version) 
 { 
   auto node = YAML::convert<std::vector<rclcpp::QoS>>::encode(in, version); 
   return YAML::Dump(node); 
 }

isn't already serializing as inline YAML like @emersonknapp did in his commit? Edit: well, maybe it's a bit different since here we don't have offered_qos_profiles as std:string. If I understood correctly, in his case he loaded the raw string as YAML just to re-serialize it via YAML facilities.

And for deserialization, we should not have any particular problem, am I right?

[roncapat]

I'm disabling temp folder destruction in TemporaryDirectoryFixture so to have a look at all the serialization tests... have you noticed that TemporaryDirectoryFixture destroys the temp files but leaves a lot of empty folders like tmp_test_dir_kWE5LW in /tmp?

[roncapat]

Sorry, I went to read #609, and maybe now I catched the request here...

Having:

offered_qos_profiles: 
  - history: 3
    depth: 0
    reliability: 1
    durability: 2
    deadline:
      sec: 2147483647
      nsec: 4294967295
    lifespan:
      sec: 2147483647
      nsec: 4294967295
    liveliness: 1
    liveliness_lease_duration:
      sec: 2147483647
      nsec: 4294967295
    avoid_ros_namespace_conventions: false

instead of

offered_qos_profiles: "- history: 3\n  depth: 0\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 2147483647\n    nsec: 4294967295\n  lifespan:\n    sec: 2147483647\n    nsec: 4294967295\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 2147483647\n    nsec: 4294967295\n  avoid_ros_namespace_conventions: false"

so proper YAML instead of std::string. Please let me know if this is the desired approach or something way simpler and I'm just misunderstanding :)

[roncapat]

If we change:

std::string serialize_rclcpp_qos_vector(const std::vector<rclcpp::QoS> & in, int version)
{
  auto node = YAML::convert<std::vector<rclcpp::QoS>>::encode(in, version);
  return YAML::Dump(node);
}

to

std::string serialize_rclcpp_qos_vector(const std::vector<rclcpp::QoS> & in, int version)
{
  YAML::Emitter emitter;
  auto node = YAML::convert<std::vector<rclcpp::QoS>>::encode(in, version);
  emitter << YAML::Literal << node;  
   return emitter.c_str();
}

it will keep newlines in the string. But we have still serialization.

Example:

offered_qos_profiles: "  - history: 3
    depth: 0
    reliability: 1
    durability: 2
    deadline:
      sec: 2147483647
      nsec: 4294967295
    lifespan:
      sec: 2147483647
      nsec: 4294967295
    liveliness: 1
    liveliness_lease_duration:
      sec: 2147483647
      nsec: 4294967295
    avoid_ros_namespace_conventions: false"

[roncapat]

Instead if do this changes:

we will get plain YAML in metadata.yaml:

rosbag2_bagfile_information:
  version: 7
  storage_identifier: ""
  duration:
    nanoseconds: 0
  starting_time:
    nanoseconds_since_epoch: 0
  message_count: 0
  topics_with_message_count:
    - topic_metadata:
        name: topic
        type: type
        serialization_format: rmw
        offered_qos_profiles:
          - history: 1
            depth: 1
            reliability: 1
            durability: 2
            deadline:
              sec: 0
              nsec: 0
            lifespan:
              sec: 0
              nsec: 0
            liveliness: 0
            liveliness_lease_duration:
              sec: 0
              nsec: 0
            avoid_ros_namespace_conventions: false
        type_description_hash: hash
      message_count: 1
  compression_format: ""
  compression_mode: ""
  relative_file_paths:
    []
  files:
    []
  custom_data: ~
  ros_distro: ""

The problem, as you see, might be in how older versions are wired in the ser/deser code. This example is a dump of a test-generated metadata.yaml, it has version 7 but it has been affected. Will need to add detection of version 9 to use plain YAML.

Edit: I think I already fixed this via if/else handling.

[roncapat]

Waaay better now IMHO. The last commit implements the desired behaviour of #609 and frankly speaking this is very user friendly, I mean, now everything in metadata.yaml is treated as YAML. Let me know your thoughts of course.

[MichaelOrlov]

@roncapat Thanks for taking care of the proper serialization formatting and sorry that I didn't explain what those fixes intended to do.
I like your proposal about

std::string serialize_rclcpp_qos_vector(const std::vector<rclcpp::QoS> & in, int version)
{
  YAML::Emitter emitter;
  auto node = YAML::convert<std::vector<rclcpp::QoS>>::encode(in, version);
  emitter << YAML::Literal << node;  
   return emitter.c_str();
}

it will keep newlines in the string. But we have still serialization.

Can we add it too? perhaps we can use it only for version > 8 for backward compatibility.
I hope It will give us the same readable serialization for the cases when we are storing TopicMetadata in the mcap and slite3 database files as well.

[roncapat]

Of course :) Let me just add it locally and run test on my workstation first. When everything is good I push!

[roncapat]

Test report... this may not be as straightforward as it seems unfortunately.
test.log

[MichaelOrlov]

@roncapat Did you put it under the if (version > 8) ?

I made it as

std::string serialize_rclcpp_qos_vector(const std::vector<rclcpp::QoS> & in, int version)
{
  if (version > 8) {
    YAML::Emitter emitter;
    auto node = YAML::convert<std::vector<rclcpp::QoS>>::encode(in, version);
    emitter << YAML::Literal << node;
    return emitter.c_str();
  } else {
    auto node = YAML::convert<std::vector<rclcpp::QoS>>::encode(in, version);
    return YAML::Dump(node);
  }
}

And all tests from the rosbag2_stroage passed on my local machine.

[roncapat]

I was just telling you the same, yep, I forgot! :)

[MichaelOrlov]

Actually emmiter output is not as we would expect
I am getting failures in the rosbag2_transport tests

- rosbag2_transport.RecordIntegrationTestFixture qos_is_stored_in_metadata
  <<< failure message
    /home/morlov/ros2_rolling/src/ros2/rosbag2/rosbag2_transport/test/rosbag2_transport/test_record.cpp:182
    Value of: serialized_profiles
    Expected: contains regular expression "reliability: reliable\n"
      Actual: "- ? |\n    history\n  : |\n    keep_all\n  ? |\n    depth\n  : |\n    0\n  ? |\n    reliability\n  : |\n    reliable\n  ? |\n    durability\n  : |\n    volatile\n  ? |\n    deadline\n  : ? |\n      sec\n    : |\n      9223372036\n    ? |\n      nsec\n    : |\n      854775807\n  ? |\n    lifespan\n  : ? |\n      sec\n    : |\n      9223372036\n    ? |\n      nsec\n    : |\n      854775807\n  ? |\n    liveliness\n  : |\n    automatic\n  ? |\n    liveliness_lease_duration\n  : ? |\n      sec\n    : |\n      9223372036\n    ? |\n      nsec\n    : |\n      854775807\n  ? |\n    avoid_ros_namespace_conventions\n  : |\n    false"
    /home/morlov/ros2_rolling/src/ros2/rosbag2/rosbag2_transport/test/rosbag2_transport/test_record.cpp:183
    Value of: serialized_profiles
    Expected: contains regular expression "durability: volatile\n"
      Actual: "- ? |\n    history\n  : |\n    keep_all\n  ? |\n    depth\n  : |\n    0\n  ? |\n    reliability\n  : |\n    reliable\n  ? |\n    durability\n  : |\n    volatile\n  ? |\n    deadline\n  : ? |\n      sec\n    : |\n      9223372036\n    ? |\n      nsec\n    : |\n      854775807\n  ? |\n    lifespan\n  : ? |\n      sec\n    : |\n      9223372036\n    ? |\n      nsec\n    : |\n      854775807\n  ? |\n    liveliness\n  : |\n    automatic\n  ? |\n    liveliness_lease_duration\n  : ? |\n      sec\n    : |\n      9223372036\n    ? |\n      nsec\n    : |\n      854775807\n  ? |\n    avoid_ros_namespace_conventions\n  : |\n    false"

it is serilzes as reliability\n : |\n reliable\n instead of the expected "reliability: reliable\n"

[roncapat]

Strange... adding a doc reference here to start investigating from:
https://github.com/jbeder/yaml-cpp/wiki/How-To-Emit-YAML#using-manipulators
Sorry but probably I will be able to dig into it only tomorrow morning (CET).

[MichaelOrlov]

@roncapat I've tried to change emmiter config to the YAML::Newline

emitter << YAML::Newline << node;

And it seems tests passing without errors.
However I will try to debug and see how it looks like in reality.

[roncapat]

I fear it will just add a newline at the start of the string, but let me know if you find out.

[MichaelOrlov]

@roncapat I realized that what we are trying to achieve is not doable because we a trying to convert in one string.
If we need the string to be formated we need actually replace it with multiple strings with endlines.

I double checked in MCAP binary file in current implementation it is already looks like multiple strings with descent formating with new lines if open mcap file in text editor you can see something like this:

string<=22 bounded_string_value_default4 'Hello\'world!'
string<=22 bounded_string_value_default5 "Hello\"world!"
^D.^A^@^@^@^@^@^@^A^@^A^@^K^@^@^@/test_topic^C^@^@^@cdr.^A^@^@^T^@^@^@offered_qos_profiles:^A^@^@- history: unknown
  depth: 0
  reliability: reliable
  durability: volatile
  deadline:
    sec: 9223372036
    nsec: 854775807
  lifespan:
    sec: 9223372036
    nsec: 854775807
  liveliness: automatic
  liveliness_lease_duration:
    sec: 9223372036
    nsec: 854775807
  avoid_ros_namespace_conventions: false^O^@^@^@topic_type_hashG^@^@^@RIHS0

I think we don't need to do anything more. It is already good enough.

I am going to re-run full CI and merge this PR if it will be no test failures.

[MichaelOrlov]

ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12879

• Linux
• Linux-aarch64
• Windows

[roncapat]

Fantastic! :)

[MichaelOrlov]

@roncapat I changed the title for this PR to the Use enum values for offered_qos_profiles in code and string names in serialized metadata to better describe what this PR doing in a very short sentence.
If you don't mind I will merge it with the new title.

[roncapat]

No problem at all! Thanks

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-11-16/34757/1