Display bag summary using `ros2 bag info` (reduced size) #45

greimela-si · 2018-10-11T11:36:10Z

This PR adds a new ros2 bag CLI verb info which prints a summary of a bag content.

The printout looks like this:

$ ros2 bag info 2018_10_04-11_08_21

Files:            2018_10_04-11_08_21.db3
Bag size:         44.4 KiB
Storage id:       sqlite3
Storage format:   cdr
Duration:         21.3s
Start:            Oct  4 2018 11:08:21.483 (1538644101.483)
End               Oct  4 2018 11:08:42.486 (1538644122.486)
Messages:         22
Topics with Type: /string_topic; std_msgs/String; 22 msgs

These infos are stored in a metadata file at the end of the record process to allow getting infos about a bag without inspecting its content.
This is necessary since the user might not have access to the storage backend used to record the bagfile.
As a result, rviz_yaml_cpp_vendor is required as a dependency.

This PR is based on the changes introduced in #43 and contains these changes.
Therefore, this PR should be merged after #43.

CI:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-10-17T15:55:31Z

New CI after rebase and fixes:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-10-18T08:01:50Z

New CI after rebase:

Linux
Linux-aarch64
macOS
Windows

Karsten1987

There is one bug IMO:

When running ros2 bag record -o MY_TEST_BAG the execution fails correctly because no topics were specified (-a | <topic1>). However, the folder MY_TEST_BAG is already created.
That means when running the command a second time correctly with -a, the execution fails again because the folder already exists. This behavior doesn't happen when no explicit bag name is specified.

Then further, I believe it would be nice to put that exception in a nicer format:

 ➭ ros2 bag record -a -s my_plugin
DISCLAIMER
ros2 bag is currently under development and not ready to use yet
[ERROR] [rosbag2_storage]: Could not load/open plugin with storage id 'my_plugin'.
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: No storage could be initialized. Abort
Abort trap: 6

Karsten1987 · 2018-10-21T22:02:51Z

rosbag2_storage/include/rosbag2_storage/bag_metadata.hpp

+
+struct BagMetadata
+{
+  int version = 1;


What is version 1 meaning here? How is the user supposed to change this version?

This version field was introduced to allow easy transition to new formats if necessary (e.g. we want to add information about service calls, parameters, etc.). Having a version field then simplifies backwards compatibility with old yaml files.

Therefore, it should never be changed by the user.

where is this field currently used in the code? Can we extract that info somehow from incoming messages? Maybe store the MD5 sum of the messages instead a running version number?

It is not used yet and its sole purpose is to distinguish the yaml format in case of changes. It must be independent of the content so a md5 sum (or something similar) is not useful.
An example use would be the case of a format change (after the first release). For that case rosbag can distinguish the to formats and provide an automatic conversion (or at least read also the old format correctly).

As you can see, the yaml file has a specific format. This is basically specified by the metadata struct. If you add a field to it (e.g. "number of service calls"), you won't be able to parse old yaml files. The version field is introduced to help us with this in the future. We can just ask "what metadata.yaml version is this?" and then choose the parsing algorithm accordingly.

Think of it like the version field of the package.xml. It's there to allow the schema to change in the future and simplify without any hassle.

It is currently not used in the code, because until the first official release, there are no "old" bagfiles for rosbag2 and there is only one version of yaml files. Since it will always be hidden inside rosbag2_storage, there is no need to write the logic just yet.

Karsten1987 · 2018-10-21T22:04:23Z

rosbag2_storage/include/rosbag2_storage/metadata_io.hpp

@@ -12,25 +12,30 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-#ifndef ROSBAG2_STORAGE__STORAGE_INTERFACES__BASE_INFO_INTERFACE_HPP_
-#define ROSBAG2_STORAGE__STORAGE_INTERFACES__BASE_INFO_INTERFACE_HPP_
+#ifndef ROSBAG2_STORAGE__METADATA_IO_HPP_


Why this change? What's the problem with base_info_interface? Remember that these interfaces were designed to be composable for many specific interfaces.

See other comment referring to this change.

rosbag2_storage/include/rosbag2_storage/metadata_io.hpp

Karsten1987 · 2018-10-21T22:08:11Z

rosbag2_storage/src/rosbag2_storage/metadata_io.cpp

+namespace rosbag2_storage
+{
+
+void MetadataIo::write_metadata(const std::string & uri, BagMetadata metadata)


Suggested change

void MetadataIo::write_metadata(const std::string & uri, BagMetadata metadata)

void MetadataIo::write_metadata(const std::string & uri, const BagMetadata & metadata)

Good point.

Karsten1987 · 2018-10-21T22:12:34Z

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

+    rosbag2_storage::MetadataIo metadata_io;
+    return std::make_unique<rosbag2_storage::BagMetadata>(metadata_io.read_metadata(uri));
+  } catch (std::exception & e) {
+    (void) e;


Shouldn't there at least be some kind of logging?

You're right, we'll do so.

Karsten1987 · 2018-10-21T22:17:52Z

rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp

@@ -28,7 +27,7 @@ namespace storage_interfaces
 {

 class ROSBAG2_STORAGE_PUBLIC ReadOnlyInterface
-  : public BaseInfoInterface, public BaseIOInterface, public BaseReadInterface


I don't understand this change, or maybe just don't agree with it.

When I open a bag file read only, I shall still have the opportunity to read the meta data info, right? How would I do this with this current change?

Here is our rationale:

Reading the metadata yaml file should always be possible, even without opening the storage (e.g. when the storage is not known in advance) - and indeed, the storage should not be opened if it is not necessary.

The logic to read the yaml file is the same for every plugin hence a storage plugin author does not need to implement it

Therefore, it seems logical to expose this functionality outside of the storage_interface hierarchy.
Note that the information we get from the storage plugin itself is necessarily incomplete - it must be enriched with additional information from the filesystem.

It is of course possible to expose a non-abstract method inside the storage interface which just calls the the outside method. I'm just not sure I see the benefit.

In other words, with the current implementation, you can open your storage read-only (or read-write) and you can (in a separate call) query the information from the metadata file without having a storage handle.

That assumption only holds when bag files are written with rosbag2. When, however, no yaml file is specified for let's say ros1 bags, then how do I get this type of info?

Also, from an interface point of view. If I get a ReadWriteInterface, I would like to be able to call get_info() or get_metadata() on that instance. I don't see how I would do that right now, because with this change I have to rely on the plugin that this function is available.

I see your first point. That's a part where we do indeed need a function to read the information from the storage directly (for read write, this is possible).

botteroa-si · 2018-10-22T16:01:57Z

New CI after implementing requested changes:

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2018-10-22T18:46:14Z

The problem with the folder still persists:

✔  11:45:06  ~/workspace/osrf/rosbag2_ws
 ➭ ros2 bag record -a -s my_plugin
DISCLAIMER
ros2 bag is currently under development and not ready to use yet
[ERROR] [rosbag2_storage]: Could not load/open plugin with storage id 'my_plugin'.
[ERROR] [rosbag2_transport]: Failed to record: No storage could be initialized. Abort

Even tho the plugin couldn't be found, and thus the execution is aborted, the folder gets created.

Karsten1987 · 2018-10-22T18:57:28Z

rosbag2_transport/include/rosbag2_transport/rosbag2_transport.hpp

+   * \param uri path to the metadata yaml file.
+   */
+  ROSBAG2_TRANSPORT_PUBLIC
+  void print_bag_info(const std::string & uri);


I don't think that the transport layer should print anything to console. I know this will most likely go away in future PRs, but at least this function should return a BagMetaData struct and let the calling side handle the printing.

That's impossible: Since the BagMetaData struct is not a C type, there is no convenient way to pass it to the calling side (read: the python layer). So I think this will have to stay for now until moved.

This will be done separately -> #51.

Karsten1987 · 2018-10-22T19:07:06Z

rosbag2_storage/include/rosbag2_storage/metadata_io.hpp

+  static constexpr const char * const metadata_filename = "metadata.yaml";
+
+  ROSBAG2_STORAGE_PUBLIC
+  void write_metadata(const std::string & uri, const BagMetadata & metadata);


Suggested change

void write_metadata(const std::string & uri, const BagMetadata & metadata);

void write_metadata(const std::string & uri, const BagMetadata & metadata);

Karsten1987 · 2018-10-22T19:08:50Z

rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp

@@ -28,7 +27,7 @@ namespace storage_interfaces
 {

 class ROSBAG2_STORAGE_PUBLIC ReadOnlyInterface
-  : public BaseInfoInterface, public BaseIOInterface, public BaseReadInterface
+  : public BaseIOInterface, public BaseReadInterface
 {
 public:
  virtual ~ReadOnlyInterface() = default;


Suggested change

virtual ~ReadOnlyInterface() = default;

virtual ~ReadOnlyInterface() = default;

The same change should be done in read_write_interface.hpp and base_io_interface.hpp

I hopefully found all occurences

The change above is outdated. The interface is back in, so the only line changing in that file is the addition of a newline.

Karsten1987 · 2018-10-22T19:14:34Z

rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp

@@ -28,7 +27,7 @@ namespace storage_interfaces
 {

 class ROSBAG2_STORAGE_PUBLIC ReadOnlyInterface
-  : public BaseInfoInterface, public BaseIOInterface, public BaseReadInterface


Also, from an interface point of view. If I get a ReadWriteInterface, I would like to be able to call get_info() or get_metadata() on that instance. I don't see how I would do that right now, because with this change I have to rely on the plugin that this function is available.

Karsten1987 · 2018-10-23T19:16:29Z

 ➭ ros2 bag record -a
DISCLAIMER
ros2 bag is currently under development and not ready to use yet
[ERROR] [rosbag2_storage]: Failed to load metadata: Exception on parsing info file: bad file

Karsten1987 · 2018-10-23T19:19:32Z

ros2bag/ros2bag/verb/record.py


        if args.all:
            rosbag2_transport_py.record(uri=uri, storage_id=args.storage, all=True)
        elif args.topics and len(args.topics) > 0:
            rosbag2_transport_py.record(uri=uri, storage_id=args.storage, topics=args.topics)
        else:
            self._subparser.print_help()
+
+        if os.path.isdir(uri) and not os.listdir(uri):


It seems to be there is a design flaw here. I believe there should be a function (exposed) which can check whether the specified plugin is correct. That is, before calling straight up record, I think we should be able to initialize and setup the recorder at first and then call record. Having the recorder setup first allows for error checking.

Then second, the code for removing the folder only works right now because record has to be cancelled by ctrl-c (because of the spin inside). I believe this should be handled by a signint handler, to cancel the record deliberately.

IMHO the "design flaw" is that the directory creation happens already in the python layer before instead of having the storage taking care of that. The reason for this is that cross platform file handling is hard to do in C++ but much easier in Python.
Creating a separate initialize method would only solidify this workaround and would still leave some scenarios uncovered that cannot be detected beforehand.

Regarding the second point I agree that we will need to do our own signal handling for that and then we also need to change this "failure detection". I would prefer to do this with another pull request.

anhosi

As discussed offline this relies on the current capabilities of the rosbag2 interface and needs to be adopted when the #50 is implemented.

Karsten1987 · 2018-10-26T21:59:25Z

has to be rebased.

- On Windows, the process is killed hard and thus does not write its metadata file - Since this is an issue with the test setup that seems very hard to fix, for now we just write the metadata file on our own

- For example is neither --all nor topics are specified or if a non exsisting storage plugin is specified

- For example if the storage plugin specified by the user at record does not exist

…ring

Currently only supported on rosbag2 side: - Allow passing a storage identifier to rosbag2::Info() - If a yaml file exists, read info from yaml - If no yaml file exists and a storage identifier was passed open storage and read info directly

…ing with ReadWrite io_flag - This avoids the logging of a 'failed to read metadata' error when recording a new bag

-In this way it is not confused with the storage id (e.g. sqlite3)

Martin-Idel-SI · 2018-10-29T07:45:13Z

Done. Add new CI:

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2018-10-30T15:18:17Z

rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

-
-  std::cout << info_stream.str() << std::endl;
+  try {
+    auto metadata = info_->read_metadata(uri);


wouldn't the exception come out here? I believe it makes more sense to catch the exception right after this method.

Martin-Idel-SI · 2018-10-30T15:29:19Z

Done. New CI:

Linux
Linux-aarch64
macOS
Windows

tfoote added the in review label Oct 11, 2018

greimela-si mentioned this pull request Oct 11, 2018

Display bag summary using ros2 bag info #42

Closed

greimela-si force-pushed the feature/ros2_bag_info_small branch from 6a5479e to 2083676 Compare October 11, 2018 14:49

greimela-si mentioned this pull request Oct 12, 2018

Use directory as bagfile and add additonal record options #43

Merged

greimela-si force-pushed the feature/ros2_bag_info_small branch 2 times, most recently from 9078de5 to efa75d6 Compare October 12, 2018 14:39

anhosi force-pushed the feature/ros2_bag_info_small branch from efa75d6 to dbc8c58 Compare October 17, 2018 14:03

Martin-Idel-SI force-pushed the feature/ros2_bag_info_small branch from dbc8c58 to 09516b6 Compare October 18, 2018 07:58

Karsten1987 requested changes Oct 21, 2018

View reviewed changes

Karsten1987 reviewed Oct 22, 2018

View reviewed changes

Martin-Idel-SI force-pushed the feature/ros2_bag_info_small branch 2 times, most recently from 8bb296e to 873026b Compare October 23, 2018 13:51

Karsten1987 reviewed Oct 23, 2018

View reviewed changes

anhosi reviewed Oct 25, 2018

View reviewed changes

greimela-si and others added 10 commits October 29, 2018 08:32

Display bag summary using ros2 bag info

83ad0c2

Improve process execution helper to handle the working directory

53f448e

Use metadata filename in sqlite storage to determine database name

ff240aa

ros2GH-109 Write metadata file on Windows by hand

1b22db1

- On Windows, the process is killed hard and thus does not write its metadata file - Since this is an issue with the test setup that seems very hard to fix, for now we just write the metadata file on our own

Remove empty bag folder if record gets aborted and no files are created

fdb1c9b

- For example is neither --all nor topics are specified or if a non exsisting storage plugin is specified

Fail gracefully if a runtime error occurs when trying to record or play

59f3558

- For example if the storage plugin specified by the user at record does not exist

Log error in case of failing when loading metadata, and minor refacto…

576097c

…ring

Add comment to version field

c8a8c75

Allow rosbag2 info without yaml file

5e09b5f

Currently only supported on rosbag2 side: - Allow passing a storage identifier to rosbag2::Info() - If a yaml file exists, read info from yaml - If no yaml file exists and a storage identifier was passed open storage and read info directly

ros2GH-7 Don't try to read database name from metadata file when open…

d5f74ee

…ing with ReadWrite io_flag - This avoids the logging of a 'failed to read metadata' error when recording a new bag

ros2GH-7 Rename 'storage format' into 'serialization format'

9356463

-In this way it is not confused with the storage id (e.g. sqlite3)

Martin-Idel-SI force-pushed the feature/ros2_bag_info_small branch from 9d18dd0 to 9356463 Compare October 29, 2018 07:43

Martin-Idel-SI added 2 commits October 30, 2018 10:06

ros2GH-7 Improve failure conditions

7d26d6a

ros2GH-7 Cleanup of superfluous forward declarations

e6dacb3

Karsten1987 reviewed Oct 30, 2018

View reviewed changes

ros2GH-7 Further improve exception handling

4e4b46a

Karsten1987 approved these changes Oct 30, 2018

View reviewed changes

Karsten1987 merged commit f37ffa3 into ros2:master Oct 30, 2018

Karsten1987 removed the in review label Oct 30, 2018

Martin-Idel-SI deleted the feature/ros2_bag_info_small branch November 7, 2018 15:10

	void MetadataIo::write_metadata(const std::string & uri, BagMetadata metadata)
	void MetadataIo::write_metadata(const std::string & uri, const BagMetadata & metadata)

	void write_metadata(const std::string & uri, const BagMetadata & metadata);
	void write_metadata(const std::string & uri, const BagMetadata & metadata);

	virtual ~ReadOnlyInterface() = default;
	virtual ~ReadOnlyInterface() = default;

Display bag summary using ros2 bag info (reduced size) #45

Display bag summary using ros2 bag info (reduced size) #45

Conversation

greimela-si commented Oct 11, 2018 • edited Loading

Martin-Idel-SI commented Oct 17, 2018

Martin-Idel-SI commented Oct 18, 2018

Karsten1987 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

botteroa-si commented Oct 22, 2018 • edited Loading

Karsten1987 commented Oct 22, 2018

Choose a reason for hiding this comment

Martin-Idel-SI Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

anhosi Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented Oct 23, 2018

Choose a reason for hiding this comment

anhosi Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anhosi left a comment

Choose a reason for hiding this comment

Karsten1987 commented Oct 26, 2018

Martin-Idel-SI commented Oct 29, 2018

Choose a reason for hiding this comment

Martin-Idel-SI commented Oct 30, 2018

Display bag summary using `ros2 bag info` (reduced size) #45

Display bag summary using `ros2 bag info` (reduced size) #45

greimela-si commented Oct 11, 2018 •

edited

Loading

Karsten1987 left a comment •

edited

Loading

Karsten1987 Oct 22, 2018 •

edited

Loading

botteroa-si commented Oct 22, 2018 •

edited

Loading

Martin-Idel-SI Oct 23, 2018 •

edited

Loading

anhosi Oct 25, 2018 •

edited

Loading

anhosi Oct 24, 2018 •

edited

Loading