Store ROS_DISTRO as metadata.yaml and in the storage file

[wkalt]

Description

I recently discovered that ros2 db3 bag files recorded by different versions of ROS can have different database schemas. For example, bags recorded on eloquent do not have the offered_qos_profiles column in the topics table, which caused our (Foxglove's) db3 to mcap converter to fail on eloquent bags.

It would be helpful if the ros2 bag recorder wrote its ROS version to the db3 file, to enable consumers to cleanly dispatch to different parsing behavior.

Implementation Notes / Suggestions

Perhaps a new table like "metadata" or some similar concept.

[MichaelOrlov]

@wkalt Could you please provide some rosbag2 db3 bag file which is failing converter to process it?
It will significantly help for issue analysis and possible fixes.

[wkalt]

@MichaelOrlov thanks, here's one @amacneil recorded on eloquent:
eloquent-twist.db3.zip

[wkalt]

patch to fix the converter for additional context: foxglove/mcap#570

[MichaelOrlov]

@wkalt In rosbag2_storage we already storing metadata version and taking it in to account when parsing metadata from yaml file

rosbag2/rosbag2_storage/include/rosbag2_storage/yaml.hpp

Lines 246 to 260 in e179c79

	static bool decode(const Node & node, rosbag2_storage::BagMetadata & metadata)
	{
	metadata.version = node["version"].as<int>();
	metadata.storage_identifier = node["storage_identifier"].as<std::string>();
	metadata.duration = node["duration"].as<std::chrono::nanoseconds>();
	metadata.starting_time = node["starting_time"]
	.as<std::chrono::time_point<std::chrono::high_resolution_clock>>();
	metadata.message_count = node["message_count"].as<uint64_t>();
	metadata.topics_with_message_count =
	decode_for_version<std::vector<rosbag2_storage::TopicInformation>>(
	node["topics_with_message_count"], metadata.version);
	metadata.relative_file_paths = node["relative_file_paths"].as<std::vector<std::string>>();
	if (metadata.version >= 3) { // fields introduced by rosbag2_compression

In particular offered_qos_profiles was introduced since metadata.version = 4

rosbag2/rosbag2_storage/include/rosbag2_storage/yaml.hpp

Lines 109 to 113 in e179c79

	if (version >= 4) {
	topic.offered_qos_profiles = node["offered_qos_profiles"].as<std::string>();
	} else {
	topic.offered_qos_profiles = "";
	}

[MichaelOrlov]

However we are not handling medata.version properly when trying to read out metadata from database directly.

[amacneil]

@MichaelOrlov to clarify, the specific request here is that this metadata (the ROS 2 release) is stored in the db3 file when using the sqlite plugin.

We have users who open a sqlite file directly (i.e. no metadata.yaml file available to us) and having the metadata fully contained within the db3 file makes it easier to reason about. More generally, I think it would be best if all information in the metadata.yaml file is also stored in the db3 file directly.

[MichaelOrlov]

@amacneil I agree it would be nice if all metadata information from metadata.yaml file would be duplicated in db3 file directly.
But currently it is not.
We have an issue that bag file recorded on Eloquent is not usable in Rolling without metadata.yaml because our sqlite storage plugin will throw exception with error from database due to trying to read non existent offered_qos_profiles filed and will terminate application.

I can provide some fix to workaround this problem and support db3 files recorded on Eloquent without metadata.yaml on Rolling and perhaps Humble.
However saving all metadata in db3 looks like much bigger feature which is require bigger effort and I am not ready to commit on it at the moment. As always PRs are welcome :)

[amacneil]

That's funny, we had the exact same issue in the mcap convert CLI, which is what led to creating this ticket. We fixed it by testing for that column before reading it.

I also think it would be fine to say that ROS 2 Iron+ does not support reading Eloquent bags.

[MichaelOrlov]

Now rolling and humble support reading Eloquent bags.

[amacneil]

Nice! #1090 for reference.

[amacneil]

Regarding the original title of this ticket, I think it would still be good to record the ROS distro in the db3 file. E.g. a metadata table like this:

key TEXT	value TEXT
ros_distro	foxy

Probably some other keys could be added, like what DDS middleware was in use?

It will help when we need to do things like guess which version of the message definitions to use.

[MichaelOrlov]

@amacneil As regards

It will help when we need to do things like guess which version of the message definitions to use.

Can you please clarify how dds middleware version and and ROS version could help in determining message definition version?

We have a thoughts to store schema version, metadata version, metadata, and message definition directly to the .db3 files during recording . It should cover all cases. Please correct me if I am wrong.

[amacneil]

Yes, I think both would be good (storing the schema, and also storing the ros distro).

[amacneil]

cc @jhurliman @defunctzombie @james-rms

[amacneil]

@MichaelOrlov I noticed your comment in #1143 (comment):

It doesn't make sense to store ROS_DISTRO in addition to the DB schema versioning. At least I am not envision any cases when it would be useful.

Storing ROS_DISTRO specifically would be useful for Foxglove, because we try to open db3 files using a bundled copy of the standard ROS schemas, but those schemas change from release to release (e.g. Marker message is different from Galactic to Humble). Storing ROS_DISTRO would allow us to use the correct version of Marker to deserialize that file.

I don't feel strongly about how this is captured in the db3 file, but I think it would be useful. Are you open to that?

Alternatively, if the full message schemas are stored in the db3 file it would also solve this problem, but we don't have bandwidth for that as all energy is going into MCAP. Storing ROS_DISTRO in db3 is a quick and easy fix (especially if it was backported to Humble, so that we can correctly interpret those files going forward).

[MichaelOrlov]

@amacneil Thanks a lot for the clarification.
I am open to consider to store ROS_DISTRO if it will be really helpful.
Let's discuss, consider some details.

• I had a plan to add support to store full schema in db3 files the similar way as it currently done in mcap. Although we need to address Store DB schema versioning in .db3 files #1108 first. I anticipate that I will not have capacity to add full support to store full schema in db3 files until new year. May be in December will be more clear.
• To distinguish between Humble and Galactic distro we will need to backport relevant changes to the appropriate branches. I am very doubt that it will be possible since we need to keep ABI and API compatibility and it looks like we are going to break this condition if we want to store ROS_DISTRO in db3 file. One more point to not do this is that backward compatibility will be difficult without Store DB schema versioning in .db3 files #1108.
• You mentioned that (e.g. Marker message is different from Galactic to Humble) Highly likely marker message has been changed somewhere in between Galactic and Humble development and if this is a case relying on the ROS distro is incorrect approach since it will not cover all cases (e.g. bag could be recorded on some intermediate rosbag2 version). The same issue as when db3 schema was changed somewhere in between Foxy and Eloquent releases ros2 bag info and play are not able to open db3 bag files recorded on eloquent #1089.

Thoughts? Does it still make sense to pursue trying to store ROS_DISTRO?

[defunctzombie]

• I am very doubt that it will be possible since we need to keep ABI and API compatibility and it looks like we are going to break this condition if we want to store ROS_DISTRO in db3 file.

How is the ABI/API compatibility broken? If this information is not passed to the storage plugin the plugin could look at the environment ROS_DISTRO itself and write this to a table. What is considered ABI/API for the .db3 files? Is adding a new table considered breaking?

[MichaelOrlov]

@defunctzombie Good point. We might be able to add ROS_DISTRO as extra field when doing changes in the scope of the #1108.
We need to make backward comparability any way and it make sense to combine these changes.

[amacneil]

Agreed, I think we can add ROS_DISTRO to Humble and Galactic without breaking backwards compatibility, it is it strictly an addition to the db3 file, and shouldn't require changing any public interfaces?

I understand your point about this not being a perfect heuristic for the different Marker message. Storing the full schemas in db3 file would be a better solution, but I assume you would not be able to backport db3 schemas to Humble because it would require breaking ABI compatibility?

Since Humble is LTS and supported until 2027, I would be in favor of adding ROS_DISTRO to Galactic/Humble without breaking ABI, so that we have a heuristic (even if not perfect) we can use. We can still add schemas to db3 for Iron (unless you can think of a way to do that for Humble) which is a better long term solution.

[MichaelOrlov]

@amacneil I think you are right that we would not be able to backport db3 schemas to Humble because it would require breaking ABI compatibility.
Will try to add ROS_DISTRO as extra field when doing changes in the scope of the #1108.

[clalancette]

Agreed, I think we can add ROS_DISTRO to Humble and Galactic without breaking backwards compatibility, it is it strictly an addition to the db3 file, and shouldn't require changing any public interfaces?

For what it is worth, Galactic is going EOL this month. So it is probably not worth making changes to it at this point.

For Humble, as long as the new db3 files can be read by the old code and vice-versa, it is fine to add something to the schema.

[amacneil]

@clalancette good point, only identifying Humble db3 files would be sufficient. We can assume the pre-Humble Marker message unless we explicitly detect ROS_DISTRO=Humble (technically it will not work for "early Humble" bags, but it is better than nothing).

[MichaelOrlov]

@wkalt @amacneil Can we count this issue as resolved since I've added ROS_DISTRO to the new schema_version table and
corresponding API std::string SqliteStorage::get_recorded_ros_distro() const in SQLite3 plugin as part of the #1156?
These changes has been also propagated to the Humble branch.

[defunctzombie]

Awesome!

@MichaelOrlov I agree this issue is resolved since when this issue was created the ask was to have it in the sql file. I don't see it in the mcap storage plugin so it does look like we'll need a new issue to add this information to that the file.

cc @emersonknapp