Skip to content

rosbag2_storage: add type description hash to topic metadata#1272

Merged
james-rms merged 13 commits intorollingfrom
jrms/topic-metadata-type-description-hash
Apr 14, 2023
Merged

rosbag2_storage: add type description hash to topic metadata#1272
james-rms merged 13 commits intorollingfrom
jrms/topic-metadata-type-description-hash

Conversation

@james-rms
Copy link
Copy Markdown
Contributor

@james-rms james-rms commented Apr 4, 2023 •
edited
Loading

Adds a type description hash to topic metadata.

Depends on ros2/rclcpp#2137

@james-rms james-rms requested a review from MichaelOrlov April 4, 2023 02:59
@james-rms james-rms requested a review from a team as a code owner April 4, 2023 02:59
@james-rms james-rms requested review from emersonknapp and hidmic and removed request for a team April 4, 2023 02:59
@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch 12 times, most recently from a9479fc to b55434f Compare April 4, 2023 05:15
@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Apr 4, 2023

Gist: https://gist.githubusercontent.com/james-rms/0f3cca8fa197c87ba9e3bdb15cf55c3d/raw/c11aae907ae19932cdffcefbe11c930ff3d890bc/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap
TEST args: --packages-above rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11766

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch 3 times, most recently from 74894dc to b3caa30 Compare April 4, 2023 21:06
@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Apr 4, 2023

Gist: https://gist.githubusercontent.com/james-rms/ff8241cdc1abf5fd3359098ddff54941/raw/c11aae907ae19932cdffcefbe11c930ff3d890bc/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_examples rosbag2_py rosbag2_storage rosbag2_storage_sqlite3 rosbag2_transport
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_examples rosbag2_py rosbag2_storage rosbag2_storage_sqlite3 rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11780

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Apr 4, 2023

Gist: https://gist.githubusercontent.com/james-rms/fab2a65dd9c9a9677e8d9fc0481d9042/raw/c11aae907ae19932cdffcefbe11c930ff3d890bc/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_examples_cpp rosbag2_py rosbag2_storage rosbag2_storage_sqlite3 rosbag2_transport
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_examples_cpp rosbag2_py rosbag2_storage rosbag2_storage_sqlite3 rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11782

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

emersonknapp
emersonknapp reviewed Apr 5, 2023
View reviewed changes
@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch 2 times, most recently from b3fe1ab to fb8b047 Compare April 7, 2023 02:56
MichaelOrlov
MichaelOrlov requested changes Apr 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@james-rms Thanks for PR.
Overall changes make sense to me.
I am curious about when implementation of storing type_hash in sqlite3_storage and mcap_storage will be implemented?
I saw some place holders in sqlite3_storage but nothing in mcap_storage plugin.

  • Since we are changing metadata version need to increment it in one more place

    rosbag2/rosbag2_py/src/rosbag2_py/_storage.cpp

    Line 235 in fb8b047

    pybind11::arg("version") = 6,

    Also I found some missing variable declarations (see details in code comments) - seems code not compiles after your recent changes.

Please note that we will have feature freeze at April 17 and it will be nice to merge this PR before then.

@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch from a2d6e26 to fc8fbfd Compare April 11, 2023 11:18
@james-rms
rosbag2_storage: add type description hash to topic metadata
1a02f38 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms
increase versions
c9c5f5a 
Signed-off-by: James Smith <james@foxglove.dev>
@emersonknapp
Fix syntax errors
48e9d04 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@MichaelOrlov MichaelOrlov mentioned this pull request Apr 12, 2023
Closed
MichaelOrlov
MichaelOrlov reviewed Apr 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@james-rms Overall already looks good, although I think it would be great to add test coverage.
I've made some exercise in #1288. Feel free to merge or cherry pick it.

@james-rms
test against multiple endpoint infos
833918b 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch from 547859d to 833918b Compare April 13, 2023 01:42
@morlov-apexai @james-rms
Add test coverage for get_all_topics_and_types(..) and get_metadata(..)
71ba37f 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Apr 13, 2023

@MichaelOrlov cherry-picked, also added more detailed coverage into recorder.

@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch 2 times, most recently from 42667ae to 71ba37f Compare April 13, 2023 02:14
@MichaelOrlov
Copy link
Copy Markdown
Contributor

MichaelOrlov commented Apr 13, 2023

@MichaelOrlov cherry-picked, also added more detailed coverage into recorder.

@james-rms May I ask you to rename newly added function parameters endpoints to a something more meaningful like
topic_endpoints_info?

@james-rms
add topic information equality operator
c745425 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms
sqlite3: fix test
4e93bfc 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms
mcap: fix read_metadata test
236a419 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch from 2472472 to 5a72fdf Compare April 13, 2023 05:06
@james-rms
rename arg
434bbb3 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/topic-metadata-type-description-hash branch from 5a72fdf to 434bbb3 Compare April 13, 2023 05:10
@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Apr 13, 2023 •
edited by emersonknapp
Loading

Gist: https://gist.githubusercontent.com/james-rms/46185d994e5da2f8702c59149550830d/raw/a510a74b328f208ffbc7608cad10504606c7ab89/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_transport rosbag2_py rosbag2_storage_mcap rosbag2_storage_sqlite3
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_transport rosbag2_py rosbag2_storage_mcap rosbag2_storage_sqlite3
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11885

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

MichaelOrlov
MichaelOrlov approved these changes Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving

@MichaelOrlov
Copy link
Copy Markdown
Contributor

MichaelOrlov commented Apr 13, 2023

re-run Windows build

  • Windows Build Status

@MichaelOrlov
Copy link
Copy Markdown
Contributor

MichaelOrlov commented Apr 13, 2023

@james-rms I am curious how CI build passing green on Linux if
RPR job fails with error message

22:12:44 --- stderr: rosbag2_transport
22:12:44 In file included from /tmp/ws/src/rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp:15:
22:12:44 /tmp/ws/src/rosbag2/rosbag2_transport/include/rosbag2_transport/recorder.hpp:190:64: error: ‘rosidl_type_hash_t’ does not name a type
22:12:44   190 | ROSBAG2_TRANSPORT_PUBLIC std::string type_hash_to_string(const rosidl_type_hash_t & type_hash);
22:12:44       |                                                                ^~~~~~~~~~~~~~~~~~
22:12:44 /tmp/ws/src/rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp:389:39: error: ‘rosidl_type_hash_t’ does not name a type
22:12:44   389 | std::string type_hash_to_string(const rosidl_type_hash_t & type_hash)
22:12:44       |                                       ^~~~~~~~~~~~~~~~~~

Which is looks reasonable to fail since we don't have #include "rosidl_runtime_c/type_hash.h" where rosidl_type_hash_t supposed to be defined.

@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Apr 13, 2023

@MichaelOrlov RPR is failing because that's a recent addition to rosidl_runtime_c that hasn't been released yet, it's just in the rolling branch of rosidl. rosidl_runtime_c is a transitive dependency, there's no way we can have access to the definition of TopicEndpointInfo without also having the definition of rosidl_type_hash_t available.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 14, 2023

@MichaelOrlov RPR is failing because that's a recent addition to rosidl_runtime_c that hasn't been released yet, it's just in the rolling branch of rosidl. rosidl_runtime_c is a transitive dependency, there's no way we can have access to the definition of TopicEndpointInfo without also having the definition of rosidl_type_hash_t available.

It's all been released now, it is still rebuilding. In theory the Rpr jobs should all work again in about 12 hours.

@james-rms james-rms merged commit 7b5cd7d into rolling Apr 14, 2023
@delete-merged-branch delete-merged-branch bot deleted the jrms/topic-metadata-type-description-hash branch April 14, 2023 01:24
@emersonknapp
Copy link
Copy Markdown
Collaborator

emersonknapp commented Apr 14, 2023

Note: CI was all green, updated the comment - somebody had removed something from the build queue, changing the pending ID, so we found the actual build and it was green. Action CI build_and_test also passed

@MichaelOrlov
Copy link
Copy Markdown
Contributor

MichaelOrlov commented Apr 14, 2023

@clalancette Thanks for heads up and letting us know. 👍

@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Apr 24, 2023

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-4-20-2023/31087/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emersonknapp emersonknapp emersonknapp left review comments

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic was automatically assigned from ros2/tooling-reviewers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@james-rms @MichaelOrlov @clalancette @emersonknapp @ros-discourse @morlov-apexai