Use rosbag2_py API instead of metadata.yaml + sqlite3

[emersonknapp]

Depends on ros2/rosbag2#1082 Depends on https://github.com/ros2/rosbag2/pull/1083 Closes #121

• Use the rosbag2_py API to get metadata about a bag being read, instead of parsing metadata.yaml directly.
• Use SequentialReader API with new ReadOrder feature, to do all message queries, removing sqlite3 usage

Removes hardcoded client knowledge, so that rqt_bag can open rosbag2 files with any supported storage backend.

[mjeronimo]

@emersonknapp I tried running this branch against the latest ROS 2 code using the talker/listener. I'm getting the following error when trying to record:

michael@bluenote:~/src/maintenance/rqt_bag$ rqt_bag
[INFO] [1663284671.316031917] [rqt_bag.BagWidget]: Recording to /home/michael/bags/2022-09-15-16-31-04.
[INFO] [1663284671.333140073] [rosbag2_storage]: Opened database '/home/michael/bags/2022-09-15-16-31-04/2022-09-15-16-31-04_0.db3' for READ_WRITE.
Error opening bag for recording [/home/michael/bags/2022-09-15-16-31-04]: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. rosbag2_py._storage.BagMetadata(version: int, bag_size: int, storage_identifier: str, relative_file_paths: List[str], files: List[rosbag2_py._storage.FileInformation], duration: datetime.timedelta, starting_time: datetime.datetime, message_count: int, topics_with_message_count: List[rosbag2_py._storage.TopicInformation], compression_format: str, compression_mode: str)

Invoked with: 

Seems to be an issue with the BagMetadata constructor. Any ideas what could be going on?

[emersonknapp]

Oops, bad FileInformation in the BagMetadata pybind code. I need to add some python side tests to that branch before merging it!

EDIT: actually that's not it, i'll have to look into it, i haven't tested this side since the latest updates over there

[emersonknapp]

Ah - I used a default constructor for BagMetadata() for recording here, but it isn't actually defined, all arguments were required. I just pushed a new commit to the rosbag2 branch that provides this, so you should be able to pull and run now.

I'm probably not going to get to the tests until Tuesday, but that's the next step to merge that and get to this. Then https://github.com/ros2/rosbag2/pull/1083/files, once complete, fully enables the rosbag2_py all the way story

[mjeronimo]

Great. Glad to see this move to the legit interface.

[mjeronimo]

@emersonknapp I still see BagMetadata using the default constructor.

[emersonknapp]

Yes - it is now a real constructor as of the latest on that branch, the rosbag2_py side got updated for usability

[mjeronimo]

Ah, OK. I'll update my source code and give it a try.

[emersonknapp]

Gist: https://gist.githubusercontent.com/emersonknapp/1b625fa82c71544f5b66549721b7e0cf/raw/366735edcad491ddcc7cf962e401573867fd3530/ros2.repos BUILD args: --packages-above-and-dependencies rqt_bag TEST args: --packages-above rqt_bag ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10962

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

@ivanpauno @mjeronimo The upstream functionality is now merged, and this feels in good shape based on my testing. Any opinions on the latest version? Note: to try it out, it will need to be built against latest rosbag2 rolling sources, as it uses new features

[mjeronimo]

@emersonknapp I'm getting the following when running this code against the latest ROS2 code:

• First, run the talker and listener
• Next, run this rqt_bag branch
• Start recording, let it record for a while
• Pause recording by hitting the record button again
• Exit rqt_bag. All seems to have run smoothly.
• However, the output directory has the .db3 file, but no metadata.yaml file, so I'm not able to subsequently open the bag with a new sessio of rqt_bag

[emersonknapp]

I think this is already true with the master version, unless I'm mistaken! I made a new issue for it at https://github.com/ros-visualization/rqt_bag/issues/127 . I think it has something to do with the shutdown signaling not going through (I always have to ctrl+/ because ctrl+C seems to hang).

But if this is only true given this PR I will figure it out. Can't check today, only at my phone

[mjeronimo]

Ah, yes, I'm seeting it on master as well. I'm good with integrating.

[emersonknapp]

@mjeronimo @ivanpauno it looks like this got force-pushed over, the commits are not present in any branch. Did this get merged to the wrong branch ros2 when it should have been merged to rolling instead? I just found the mirroring rule, I guess that's what happened. If we're mirroring branches, why have the two at all?

[mjeronimo]

@emersonknapp Looking at the revision history on the ros2 branch, I believe what happened is that one of the first projects I had when I joined Open Robotics was porting rqt_bag to ros2. At that time, it looks like I created a 'ros2' branch for this work, not realizing the significance of having both 'ros2' and 'rolling'. I may have been following other repos, as I see that at least rqt_graph also has both 'ros2' and 'rolling' branches (although I wasn't involved in that one).

My apologies for introducing this mistake and the rework that it has caused. Moving forward, it seems like to avoid confusion and issues like this, these repos should have only a single branch for the latest ros2 development. I'd like to get some other input here to see how we can get out of this situation.

@clalancette Any suggestions here?

[mergify[bot]]

:warning: The sha of the head commit of this PR conflicts with #133. Mergify cannot evaluate rules on this PR. :warning:

[clalancette]

For what it is worth, we ended up in this situation in every single ROS 2 repository. This was the migration strategy we ended up with, in case any downstream users will still using the old branch names. I also don't love deleting old branches that were used for past releases, as that essentially deletes/rewrites history.

Further, the automation has no ability to force push to the old ros2 branch; I did that when doing the latest release, because I wasn't careful and I didn't realize something had been merged onto it.

My suggestion here is that we actually investigate whether we can protect the ros2 branch so that no new PRs can be made against it, but the automation can still write to it. That should solve most of the problem, I think.

[emersonknapp]

I looked into options, and we could do the "Lock branch" option for ros2 - so that nothing can be written to it at all.

So that would cover "don't delete old branches" - but can you explain why the branch mirroring automation should exist? The ros2 branch was used for the Galactic release - which is now likely broken because the latest source is using newer Humble features. I see I'm not seeing any reason that ros2 branch should continue to evolve at all, locking it in place where it is now makes sense to me.

[clalancette]

The ros2 branch was used for the Galactic release - which is now likely broken because the latest source is using newer Humble features

Yes, this was a large part of the motivation to do the switch to rolling in the first place; we had wildly diverging branching strategies on our repositories, so it was hard to figure out which branch belonged to what and what was safe to merge. I think it is much clearer now, though it hasn't been without a few hiccups (such as this one).

So that would cover "don't delete old branches" - but can you explain why the branch mirroring automation should exist?

The reason we added it is that there are downstream CI projects that don't actually use ros2.repos to get the branch to use; they just have some hard-coded branch names. We didn't want to silently break them by leaving them behind on a old commit, so we added this branch automation for all repositories.

That said, it has been 6 months so I think it is OK at this point to start removing the automation. Here's my suggestion for this case:

1. Make a new branch on the repository called galactic, branched off of the 1.1.2 tag (that matches what is released into Galactic currently).
2. Update https://github.com/ros/rosdistro/blob/be8f0d91be4d4e0721e4ecfc13c858f1e3c38ef2/galactic/distribution.yaml#L4830-L4847 to point to the galactic branch.
3. Update https://github.com/ros2-gbp/rqt_bag-release/blob/ed7e1905e212e045cb9e44d1dd866467345829b2/tracks.yaml#L17 to point to the galactic branch.
4. https://github.com/ros2/ros2/blob/bbc5d3d9eb56917f7c448ef2557289740547b4f4/ros2.repos#L97 to point to the galactic branch.
5. Remove the automatic mirroring (essentially merge https://github.com/ros-visualization/rqt_bag/pull/134)
6. Lock the ros2 branch.

Does that seem reasonable?

[emersonknapp]

Yes, that seems quite reasonable

[syaroshenko]

Hello folks, are there plans to merge this change into "humble"?

[clalancette]

Hello folks, are there plans to merge this change into "humble"?

No plans currently. The ReadOrder API doesn't exist in humble, so if you'd like to attempt a backport you'll have to workaround that lack somehow. Otherwise we are happy to review a backport attempt.