Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[foxy backport] Latest rosbag2 #625

Closed
wants to merge 77 commits into from

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Jan 29, 2021

Proposal to fix #657

Backport the entire master branch to foxy. This is a proposal - not a final review, just to see what changes would be needed to be compatible.

Notes:

@emersonknapp emersonknapp force-pushed the emersonknapp/backport-latest-to-foxy branch 2 times, most recently from 08ee91a to 9bd1d6e Compare February 2, 2021 20:34
@jacobperron
Copy link
Member

Releasing pybind11_vendor for Foxy sounds fine to me. Note, we should also add it to the ros2.repos file for Foxy if we end up not ignoring rosbag2_py.

@emersonknapp
Copy link
Collaborator Author

Releasing pybind11_vendor for Foxy sounds fine to me. Note, we should also add it to the ros2.repos file for Foxy if we end up not ignoring rosbag2_py.

ros2/ros2#1088

Wasn't sure if we should create a foxy release branch, lmk what you think

@emersonknapp emersonknapp force-pushed the emersonknapp/backport-latest-to-foxy branch from f17351a to 316d830 Compare February 3, 2021 00:34
Copy link
Collaborator Author

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Leaving a comment on every changed line with exact justification

@@ -29,7 +29,7 @@ jobs:
sqlite3_vendor
rosbag2_test_common
rosbag2_tests
target-ros2-distro: rolling
target-ros2-distro: foxy
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allows action CI to run on the release branch correctly

@@ -14,7 +14,7 @@
<exec_depend>rosbag2_compression</exec_depend>
<exec_depend>rosbag2_converter_default_plugins</exec_depend>
<exec_depend>rosbag2_cpp</exec_depend>
<exec_depend>rosbag2_py</exec_depend>
<!-- <exec_depend>rosbag2_py</exec_depend> -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Depends on pybind11_vendor (has been bloomed into Foxy) ros2/ros2#1088

@@ -67,7 +67,7 @@ void SequentialCompressionReader::preprocess_current_file()
* Because we have no way to check whether the bag was written correctly,
* check for the existence of the prefixed file as a fallback.
*/
const rcpputils::fs::path base{base_folder_};
rcpputils::fs::path base{base_folder_};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be allowed to be const, but that was fixed in ros2/rcpputils#119 which is API-changing and therefore has not been backported to Foxy

@@ -110,6 +110,7 @@ void SequentialWriter::open(
}

bool dir_created = rcpputils::fs::create_directories(db_path);
dir_created &= db_path.is_directory();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This stands in for https://github.com/ros2/rcpputils/pull/98/files (maybe it should be backported to Foxy?)

@@ -166,7 +166,7 @@ function(create_tests_for_rmw_implementation)
rosbag2_transport_add_gmock(test_record_regex
test/rosbag2_transport/test_record_regex.cpp
LINK_LIBS rosbag2_transport
AMENT_DEPS test_msgs rosbag2_test_common
AMENT_DEPS test_msgs rosbag2_cpp rosbag2_test_common
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allows this to be correctly built against the local workspace, when rosbag2_cpp is binary installed in the environment. This could correctly be an independent fix to master.

throw std::runtime_error(
std::string("Exception on calculating the size of directory :") + uri);
}
metadata.bag_size = rcutils_calculate_directory_size(uri.c_str(), allocator);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adapts to API change from ros2/rcutils#306

@emersonknapp
Copy link
Collaborator Author

@j-rivero I am wondering if you have any ideas how we can get the ABI report for rosbag2. The problem right now is that zstd_vendor installs some incorrect headers, so these are already in the Foxy version that's being used as a baseline - is there any way to configure the ABI checker, or do we need to first backport and bloom the fix that removes the bad headers, so that Foxy can be used as a baseline? If we do go through those steps, is there any way to check ahead of time that this fixes all the problems?

ABI report: https://build.ros2.org/job/Fpr__rosbag2__ubuntu_focal_amd64/116/API_5fABI_20report/
Fix (in theory): #631

@jacobperron jacobperron mentioned this pull request Feb 5, 2021
@ros-discourse
Copy link

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

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/1

@dejanpan
Copy link

@emersonknapp thanks. In the Discourse post we mentioned the following:

Unfortunately, these performance improvements come with a price. And that price is a breaking ABI/API compatibility with the currently released Foxy version. The changes are affecting the public API in rosbag2_cpp, rosbag2_storage as well as the sqlite3 storage plugin

Is this commit really the only API/ABI that has changed between the master and the foxy 316d830 or is there more? If there is more, would it be possible to list those APIs/ABIs?

@ros-discourse
Copy link

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

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/13

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Feb 15, 2021

Is this commit really the only API/ABI that has changed between the master and the foxy 316d830 or is there more? If there is more, would it be possible to list those APIs/ABIs?

You've misunderstood - that commit contains the changes necessary to make rosbag2:master build against the Foxy core. It indicates nothing about the API changes in rosbag2, instead it indicates API changes in the ROS 2 core from Foxy->latest that are used by rosbag2.

EDIT: It will be possible to list the changed APIs - listing the changed ABIs will be more difficult until we can make the reporting tool work - I have had no luck locally or on the buildfarm yet.

Karsten1987 and others added 12 commits February 16, 2021 18:38
* export shared_queues_vendor

Signed-off-by: Knese Karsten <[email protected]>

* Revert "find rosbag2_cpp (tinyxml2) before rcl (#423)"

This reverts commit 48fd15e.

Signed-off-by: Emerson Knapp <[email protected]>
* First attempt at time splitting logic
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Give default value for max duration
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Added bagfile duration as a storage option
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Initialize duration off of storage optios
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Switch duration in StorageOptions to take int
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Add duration to command line interface
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Suppress lint warning
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Add missing K parameter to the parsing string
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Fix typo
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Switch duration measurement to seconds
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Get project to compile again
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Change logic to allow simultaneous split modes
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Clarifying help comments on splitting behavior
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Fix typo
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Properly split by time
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Initial implementation of duration split test
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Linting whitespace
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Move curly brace for lint
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Remove magic constant
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Finally found and fixed unit error
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Force starting_time to be a system_clock time_point
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Change another high_resolution clock instance
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Convert everything to steady_clock
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Convert everything to use high_resolution_clock
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>

* Switch Windows testing code to newer version
Distro A, OPSEC #2893

Signed-off-by: Jacob Hassold <[email protected]>
* expect only 60 percent of messages to arrive

Signed-off-by: Karsten Knese <[email protected]>

* test for only one message

Signed-off-by: Karsten Knese <[email protected]>
* minimal c++ API test

Signed-off-by: Karsten Knese <[email protected]>

* linters

Signed-off-by: Karsten Knese <[email protected]>
* Add per-message ZSTD compression

This implements the per-messages compression and decompression
functions for the ZSTD compressor and also adds unit tests
for them.

Distro A, OPSEC #2893

Signed-off-by: P. J. Reed <[email protected]>
* reenable cppcheck

Signed-off-by: Karsten Knese <[email protected]>

* suppress unknown macro inline

Signed-off-by: Karsten Knese <[email protected]>
* Use foxy testing apt repos to install linters for Actions

Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
* Consolidate ZSTD utility functions

The zstd_compressor and zstd_decompressor implementations had a
number of duplicated utility functions between them; this consolidates
them into one file.

Signed-off-by: P. J. Reed <[email protected]>
* added db directory creation to storage factory

Signed-off-by: Marwan Taher <[email protected]>

* moved db directory creation to rosbag2_cpp

Signed-off-by: Marwan Taher <[email protected]>

* rasing exception if dir already exists

Signed-off-by: Marwan Taher <[email protected]>

* removed dir creation from record.py, added dir creation to sequential_compression_writer and refactored dir creation in sequential_writer

Signed-off-by: Marwan Taher <[email protected]>

* fixed failing tests

Signed-off-by: Marwan Taher <[email protected]>

* fixing review comments

Signed-off-by: Marwan Taher <[email protected]>

* Apply suggestions from code review

Co-authored-by: Karsten Knese <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Barry-Xu-2018 and others added 22 commits February 16, 2021 18:38
* Updating performance writer
- utilizes --storage-config-file with two examples
- message reading from metadata yaml file (ready for cache double-buffers PR 546)
- removed performance-affecting outputs
- supports bag splitting in benchmarking

Signed-off-by: Adam Dabrowski <[email protected]>

* typo fix

Signed-off-by: Adam Dabrowski <[email protected]>

* corrected compression so that it doesn't break the command line in case it is empty (the default)

Signed-off-by: Adam Dabrowski <[email protected]>

* address CI warns/fails on MacOS and Windows

Signed-off-by: Adam Dabrowski <[email protected]>
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior

Signed-off-by: Adam Dabrowski <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* Change all 'fastrtps-unspecified' duration values in test resources to 0 for cross-implementation readability, pending a clarification of the rmw qos duration api

Signed-off-by: Emerson Knapp <[email protected]>
…ge (#603)

* Mutex-protected writes and topic creation/removal in sqlite_storage

Signed-off-by: Adam Dabrowski <[email protected]>
* Regex and exclude options for recording topics
- you can use -e or --regex option now to specify how topics are recorded
- can use -x or --exclude to exclude topics from recording
- regex is exclusive with -a and specifying topics (for simplicity)

Signed-off-by: Adam Dabrowski <[email protected]>
…to find bagfiles in incorrectly-written metadata (#612)

* Deduplicate sequentialcompressionreader business logic, add fallback to find bagfiles in incorrectly-written metadata

Signed-off-by: Emerson Knapp <[email protected]>
* Refactoring of rosbag2 performance package:
- renamed since now it no longer benchmarks writer only
- generalized byte_producer so that it uses a callback instead of queue, so it can be reused in publisher scheme

Signed-off-by: Adam Dabrowski <[email protected]>

* Benchmark publishers based on yaml configuration
- can specify multiple groups of publishers (see attached example yaml)
- reuses byte producer

Signed-off-by: Adam Dabrowski <[email protected]>

* Applying configured QoS settings for publishers.
Also included in yaml example.

Signed-off-by: Adam Dabrowski <[email protected]>

* linters

Signed-off-by: Adam Dabrowski <[email protected]>

* Towards common configuration
- separating out common structures
- utility class for common parameter parsing

Signed-off-by: Adam Dabrowski <[email protected]>

* Barebone launchfile for benchmarks.

Signed-off-by: Piotr Jaroszek <[email protected]>

* writer benchmark adapted to yaml file and publisher groups

Signed-off-by: Adam Dabrowski <[email protected]>

* refactored result writing and bag parameters

Signed-off-by: Adam Dabrowski <[email protected]>

* linters

Signed-off-by: Adam Dabrowski <[email protected]>

* Launchfile for benchmarks

Signed-off-by: Piotr Jaroszek <[email protected]>

* Change storage config file from non optimized to resilient

Signed-off-by: Piotr Jaroszek <[email protected]>

* Max bag size for benchmark launchfile. Launchfile refactor.

Signed-off-by: Piotr Jaroszek <[email protected]>

* Copy yaml configs after benchmark is finished.

Signed-off-by: Piotr Jaroszek <[email protected]>

* Benchmark results csv file extended

Signed-off-by: Piotr Jaroszek <[email protected]>

* added disclaimer about random data and compression

Signed-off-by: Adam Dabrowski <[email protected]>

* Report gen tool for benchmarks

Signed-off-by: Piotr Jaroszek <[email protected]>

* Benchmarks out dir name changed

Signed-off-by: Piotr Jaroszek <[email protected]>

* results writer node

Signed-off-by: Adam Dabrowski <[email protected]>

* documentation

Signed-off-by: Adam Dabrowski <[email protected]>

* Transport and transportless in launchfile

Signed-off-by: Piotr Jaroszek <[email protected]>

* Benchmark launchfile refactor

Signed-off-by: Piotr Jaroszek <[email protected]>

* Wait for rosbag listening in benchmark launchfile

Signed-off-by: Piotr Jaroszek <[email protected]>

* Uncrustify and some comments for benchmarking tools

Signed-off-by: Piotr Jaroszek <[email protected]>

* Added new producers config for benchmarks

Signed-off-by: Piotr Jaroszek <[email protected]>

* Missing parameters in transport benchmark

Signed-off-by: Piotr Jaroszek <[email protected]>

added comment in storage_optimized.yaml

* Missing rosbag record parameters in transport benchmark

Signed-off-by: Piotr Jaroszek <[email protected]>

* Wait for subscriptions parameter in producers config

Signed-off-by: Piotr Jaroszek <[email protected]>

* moved utils code from header to source

Signed-off-by: Adam Dabrowski <[email protected]>

* changed compiler shortcut uint to unsigned int (should fix Windows build)

Signed-off-by: Adam Dabrowski <[email protected]>

Co-authored-by: Piotr Jaroszek <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
* Synchronize compression shutdown correctly, avoiding occasional deadlock

Signed-off-by: Emerson Knapp <[email protected]>
* Fix relative metadata path writing in compression by deduplicating business logic

Signed-off-by: Emerson Knapp <[email protected]>
* Regex and exclude fix for rosbag recorder

Signed-off-by: Piotr Jaroszek <[email protected]>
Several packages were failing to build from source when rosbag2_storage
was installed from a binary package because the path to the binary
install was being added to CMake's include path before the workspace
path.  This tweaks the dependencies and include orders to ensure the
source workspace path is preferred.

Fixes #583

Distro A, OPSEC #4584

Signed-off-by: P. J. Reed <[email protected]>
* Implement streaming compression/decompression

Distro A, OPSEC #4584

Signed-off-by: P. J. Reed <[email protected]>
Make compressor implementation into a plugin via pluginlib

Signed-off-by: Emerson Knapp <[email protected]>
* Update CHANGELOG
* 0.6.0

Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp emersonknapp force-pushed the emersonknapp/backport-latest-to-foxy branch from 7b52eda to e5c7951 Compare February 17, 2021 02:38
@JWhitleyWork
Copy link

Is there a next step for this MR?

@JWhitleyWork
Copy link

It looks like pybind11_vendor has been released for Foxy.

@emersonknapp
Copy link
Collaborator Author

Is there a next step for this MR?

Yes - we are considering releasing the rosbag2 as rosbag2-future into Foxy, instead of doing a breaking backport to the package. We're expecting to make the final decision in the Tooling WG meeting tomorrow.

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Mar 17, 2021

After deciding to go forward with the rosbag2-future approach instead of breaking the existing release, this PR is not needed.

Please see the Discourse post for more context

@JWhitleyWork
Copy link

Thank you!

@emersonknapp emersonknapp deleted the emersonknapp/backport-latest-to-foxy branch March 25, 2021 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.