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

Adds play until functionality -- follow up to #962 #1005

Merged
merged 10 commits into from
Jun 21, 2022

Conversation

agalbachicar
Copy link
Contributor

This PR offers the same functionality than #962 but is done directly on top of the latest version of #960 ( commit 59c52ef ). It has been done this way because to re-work the PR so it follows the style agreed on #960 a revert was required of the entire patch.

This PR requires #960 to be merged first (at which point it will probably need to be rebased).

This PR was co-authored with @gbiggs

Fore reviewers, you should start from df793c7 onwards.

Copy link
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.

@agalbachicar How customer will know what number for timestamp to use for command line playback_until option?
If it's going to be expressed in seconds and floating point representation.

Sorry I haven't seen unit tests yet.

rosbag2_py/src/rosbag2_py/_transport.cpp Outdated Show resolved Hide resolved
@agalbachicar
Copy link
Contributor Author

@agalbachicar How customer will know what number for timestamp to use for command line playback_until option?
If it's going to be expressed in seconds and floating point representation.

I concur with you. That's not the best way to determine it because of floating point to integer conversions, but I think it is better than receiving a timestamp in nanoseconds. I am open to suggestions if you have any.

@agalbachicar agalbachicar marked this pull request as ready for review May 11, 2022 13:43
@agalbachicar agalbachicar requested a review from a team as a code owner May 11, 2022 13:43
@agalbachicar agalbachicar requested review from MichaelOrlov and jhdcs and removed request for a team May 11, 2022 13:43
Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

I've left a couple questions that I was hoping you'd be able to clarify...

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
if (play_options_.playback_duration >= rclcpp::Duration(0, 0) ||
play_options_.playback_until >= rcutils_time_point_value_t{0})
{
play_until_time = std::max(
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone set both playback_duration and playback_until, you could run into a scenario that the starting_time + playback_duration could be a later timestamp than the playback_until value - which means, according to the std::max here, that the playback_until value would be effectively ignored in that instance, but NOT ignored in other instances.

Is this desired behavior? If so, we may want to document that behavior in the --help from the CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before answering, I would like to double check what an instance means for you. Can you elaborate a bit more on that? I will regardless expand the help documentation to make sure that everyone understands if we end up having the playback_until flag as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

An instance is a different usage of the program. So, one instance would be where the program is run with playback_duration being greater than playback_until, a different instance would be where playback_until is greater than playback_duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this desired behavior?

Yes, this is the desired behavior.

An instance is a different usage of the program. So, one instance would be where the program is run with playback_duration being greater than playback_until, a different instance would be where playback_until is greater than playback_duration.

I don't think it should be a problem from the player point of view (aside from the duplicated topics). Even if both requests land to the same instance, the first one should prevail over the second one.

6d55bc8 adds the docstring. Let me know what you think.

@MichaelOrlov
Copy link
Contributor

@agalbachicar

@agalbachicar How customer will know what number for timestamp to use for command line playback_until option?
If it's going to be expressed in seconds and floating point representation.

I concur with you. That's not the best way to determine it because of floating point to integer conversions, but I think it is better than receiving a timestamp in nanoseconds. I am open to suggestions if you have any.

I wouldn't expose this API to customer via CLI at all.
I am asking about real use case.
What would be a real use case and customer's steps to get those numbers in any form?

@agalbachicar
Copy link
Contributor Author

Thanks @jhdcs , I replied to your comments, PTAL.

@MichaelOrlov I haven't ignored yours, I am gathering data from the person that is actually handling all the set of features we are trying to add to rosbag but is currently using an adapted fork. That should help to the discussion more than my own experience.

@agalbachicar agalbachicar requested a review from jhdcs May 13, 2022 10:20
@MichaelOrlov
Copy link
Contributor

@MichaelOrlov I haven't ignored yours, I am gathering data from the person that is actually handling all the set of features we are trying to add to rosbag but is currently using an adapted fork. That should help to the discussion more than my own experience.

Just to clarify.

For play for API I can imagine that user may know that some interesting moment happened for instance after 32 seconds and he want to play from 32 seconds next 10 seconds. The user can specify command line options to start playback from 32 second and play for 32+10= 42 seconds. Or just play from the beginning for 42 seconds.

Although for play until API I have no idea how user will get timestamp in bag until which need to play in nanoseconds since 1970 year (linux format). And then need to convert those number of nanoseconds in seconds in floating point representation.

My takeaway is that if there are no easy understandable way to get value for the command line option we shouldn't expose it.
Since it's going to be useless for most users in most cases.

cc: @emersonknapp

@andrewbest-tri
Copy link

andrewbest-tri commented May 17, 2022

@MichaelOrlov and @agalbachicar I want to provide some use context for this API. We had requested this functionality as part of an experiment pipeline we run at TRI.

We have a large multi-machine system that has baggers running on more than one host. The baggers also do not start at the same time because of distributed system launching. When analyzing the data, we either have to run a bag_merger process (which is fine for small bags but can be frustrating or impossible for large bags) or we need to stream channels from bags we care about up to a point into a new bag (or a node that's doing analysis).

Playing until a specific timestamp avoids the need to know when the individual bag started and computing duration for each bag, so we can just run a bag player for each either from the host machines or after pulling the bags into a common location.

We can't reliably say that something interesting happened after 32 seconds, because the camera data is in a different bag than telemetry. We can however analyze the camera data and find something interesting, then request the telemetry back to play back that data by timestamp. The expectation is that we are getting the initial timestamps from looking through one of our ros bags from the experiments

@MichaelOrlov
Copy link
Contributor

@andrewbest-tri

The expectation is that we are getting the initial timestamps from looking through one of our ros bags from the experiments

I understand your use case. And it mostly dictated by the sub-optimal system design and lack of the Start/Stop API for the distributed recording. We at ApexAI also started development of the distributed recording and came across with the same problem that there are no API in rosbag2 to simultaneously start recording on remote machines.

I am about to design and implement 'Start/Stop' API for rosbag2 in nearest future.
If it will exist and your recordings will be synchronized can we say that this feature request for play until will be redundant and you will be able to rely on the time lapsed from start of the recording and use already existing play for API?

@andrewbest-tri
Copy link

Interesting. I am exciting about remote start/stop as a capability as its another gap we want to fill. However, I am not sure it totally replaces the use of play until. Play until remains more flexible if we add or remove data collection devices for testing purposes without extensive integration into the main pipeline start / stop. I think both features are still useful.

My specific case for that is when we may or may not want to log data from a source not part of the standard system bringup. We have a set of processes that run across multiple machines, some of which is the bagging. If we want to test out a new device without modifying the distributed configuration, we may want the ability to start and stop one-off recorders at will. We imagine a use case where the same data collection may be used by multiple sub-teams who only want specific data from certain sub-systems. I still want the ability to let them run their own data collector during an experiment and provide them with the overall system bag for them to dice as they please.

@MichaelOrlov
Copy link
Contributor

@andrewbest-tri For the cases when need to extract some certain subset of topics from bag in another bag you can use bag rewriter Please refer to the unit tests for example of the how to use it https://github.com/ros2/rosbag2/blob/master/rosbag2_transport/test/rosbag2_transport/test_rewrite.cpp
We also have command line interface "ros2 bag convert" for it.

I think we can add play until command line interface for a while.
The only concern is about format for the timestamp. Using floating point format representation in seconds looks like inaccurate and doesn't really represent timestamp which we are writing in the bag.
In bag we are writing integer value representing number of nanoseconds.
I would vote to use the same integer values as we store in bag for command line option.
Using floating point format is cumbersome, inaccurate and will involve useless double conversion to seconds and then back to the nanoseconds.
Thoughts?

@agalbachicar
Copy link
Contributor Author

Would it work to have two arguments (seconds and nanoseconds) as two integer input args?

ros2 bag play --play_until_sec X --play_until_nsec Y

We would compose them and create a timestamp. The logic would be that we would only consider the postive parts, and set the flags to be by default negative. That way, you can opt to pass only the seconds, only the nanoseconds, or both.

Thoughts?

@agalbachicar
Copy link
Contributor Author

See e5d83fe . It implements my proposal in #1005 (comment) . I can either revert it or amend it in future commits if it is not satisfactory.

I'm looking forward to hearing your thoughts.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented May 24, 2022

@agalbachicar --play_until_sec is going to be integer value in command line parameter. Is it what you want?
I was thinking actually to add --play_until_sec as floating point value and --play_until_nsec as integer value and define them to be able to use one or another but not together. i.e. Output an error message if they are both positive.

@agalbachicar
Copy link
Contributor Author

@agalbachicar --play_until_sec is going to be integer value in command line parameter. Is it what you want?
I was thinking actually to add --play_until_sec as floating point value and --play_until_nsec as integer value and define them to be able to use one or another but not together. i.e. Output an error message if they are both positive.

I don't have a strong preference and implemented the mutually exclusive parameters as you suggested. Here is what a user will see in the future when passing them both:

$ ros2 bag play --playback-until-sec 123.456 --playback-until-nsec 123456000000 a_bag_file
usage: ros2 bag play [-h] [-s {my_test_plugin,sqlite3,my_read_only_test_plugin}] [--read-ahead-queue-size READ_AHEAD_QUEUE_SIZE] [-r RATE] [--topics TOPICS [TOPICS ...]]
                     [--qos-profile-overrides-path QOS_PROFILE_OVERRIDES_PATH] [-l] [--remap REMAP [REMAP ...]] [--storage-config-file STORAGE_CONFIG_FILE] [--clock [CLOCK]] [-d DELAY]
                     [--playback-duration PLAYBACK_DURATION] [--playback-until-sec PLAYBACK_UNTIL_SEC | --playback-until-nsec PLAYBACK_UNTIL_NSEC] [--disable-keyboard-controls] [-p]
                     [--start-offset START_OFFSET] [--wait-for-all-acked TIMEOUT] [--disable-loan-message]
                     bag_file
ros2 bag play: error: argument --playback-until-nsec: not allowed with argument --playback-until-sec

And when asking for help:

$ ros2 bag play -h
usage: ros2 bag play [-h] [-s {my_test_plugin,my_read_only_test_plugin,sqlite3}] [--read-ahead-queue-size READ_AHEAD_QUEUE_SIZE] [-r RATE] [--topics TOPICS [TOPICS ...]]
                     [--qos-profile-overrides-path QOS_PROFILE_OVERRIDES_PATH] [-l] [--remap REMAP [REMAP ...]] [--storage-config-file STORAGE_CONFIG_FILE] [--clock [CLOCK]] [-d DELAY]
                     [--playback-duration PLAYBACK_DURATION] [--playback-until-sec PLAYBACK_UNTIL_SEC | --playback-until-nsec PLAYBACK_UNTIL_NSEC] [--disable-keyboard-controls] [-p]
                     [--start-offset START_OFFSET] [--wait-for-all-acked TIMEOUT] [--disable-loan-message]
                     bag_file

Play back ROS data from a bag

positional arguments:
  bag_file              bag file to replay

optional arguments:
  -h, --help            show this help message and exit
  -s {my_test_plugin,my_read_only_test_plugin,sqlite3}, --storage {my_test_plugin,my_read_only_test_plugin,sqlite3}
                        Storage implementation of bag. By default tries to determine from metadata.
  --read-ahead-queue-size READ_AHEAD_QUEUE_SIZE
                        size of message queue rosbag tries to hold in memory to help deterministic playback. Larger size will result in larger memory needs but might prevent delay of message playback.
  -r RATE, --rate RATE  rate at which to play back messages. Valid range > 0.0.
  --topics TOPICS [TOPICS ...]
                        topics to replay, separated by space. If none specified, all topics will be replayed.
  --qos-profile-overrides-path QOS_PROFILE_OVERRIDES_PATH
                        Path to a yaml file defining overrides of the QoS profile for specific topics.
  -l, --loop            enables loop playback when playing a bagfile: it starts back at the beginning on reaching the end and plays indefinitely.
  --remap REMAP [REMAP ...], -m REMAP [REMAP ...]
                        list of topics to be remapped: in the form "old_topic1:=new_topic1 old_topic2:=new_topic2 etc."
  --storage-config-file STORAGE_CONFIG_FILE
                        Path to a yaml file defining storage specific configurations. For the default storage plugin settings are specified through syntax:read: pragmas: ["<setting_name>" =
                        <setting_value>]Note that applicable settings are limited to read-only for ros2 bag play.For a list of sqlite3 settings, refer to sqlite3 documentation
  --clock [CLOCK]       Publish to /clock at a specific frequency in Hz, to act as a ROS Time Source. Value must be positive. Defaults to not publishing.
  -d DELAY, --delay DELAY
                        Sleep duration before play (each loop), in seconds. Negative durations invalid.
  --playback-duration PLAYBACK_DURATION
                        Playback duration, in seconds. Negative durations mark an infinite playback. Default is -1.0. When positive, the maximum of `playback-until` and the one that this attribute
                        yields will be used to determine which one stops playback execution.
  --playback-until-sec PLAYBACK_UNTIL_SEC
                        Playback until timestamp, expressed in seconds since epoch. Mutually exclusive argument with '--playback-until-nsec'. Use this argument when floating point to integer conversion
                        error is not a problem for your application. Negative stamps disable this feature. Default is -1.0. When positive, the maximum of the effective time that `--playback-duration`
                        yields and this attribute will be used to determine which one stops playback execution.
  --playback-until-nsec PLAYBACK_UNTIL_NSEC
                        Playback until timestamp, expressed in nanoseconds since epoch. Mutually exclusive argument with '--playback-until-sec'. Use this argument when floating point to integer
                        conversion error matters for your application. Negative stamps disable this feature. Default is -1. When positive, the maximum of the effective time that `--playback-duration`
                        yields and this attribute will be used to determine which one stops playback execution.
  --disable-keyboard-controls
                        disables keyboard controls for playback
  -p, --start-paused    Start the playback player in a paused state.
  --start-offset START_OFFSET
                        Start the playback player this many seconds into the bag file.
  --wait-for-all-acked TIMEOUT
                        Wait until all published messages are acknowledged by all subscribers or until the timeout elapses in millisecond before play is terminated. Especially for the case of sending
                        message with big size in a short time. Negative timeout is invalid. 0 means wait forever until all published messages are acknowledged by all subscribers. Note that this option
                        is valid only if the publisher's QOS profile is RELIABLE.
  --disable-loan-message
                        Disable to publish as loaned message. By default, if loaned message can be used, messages are published as loaned message. It can help to reduce the number of data copies, so
                        there is a greater benefit for sending big data.

@agalbachicar
Copy link
Contributor Author

@MichaelOrlov thanks for your input so far. I think we are close to conclude how the CLI should work. Also, I hope that unit tests are aligned with your expectations. If there is anything else that I should change about them let me know.

@MichaelOrlov
Copy link
Contributor

@agalbachicar Sorry for the delay on my side.
I think we are pretty close to final design and implementation. At least it seems no open questions anymore.
I will take a look on it one more time at Friday or over the weekend.

@MichaelOrlov
Copy link
Contributor

@agalbachicar Could you please clean up your branch?
I meant rebase to the latest master or cherry-pick only relevant commits to the new branch.
It's difficult to review.

Copy link
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.

@agalbachicar Overall looks good. I requested mostly minor changes or some nitpicks or improvements.

As regards to the tests. It would be nice to move common code as RosBag2PlayUntilTestFixture and RosBag2PlayForTestFixture in some common place and combined them to avoid code duplication. For instance it could be a common function

void InitPlayerWithPlaybackUntilAndDurationAndPlay(
    int64_t playback_until_millis,
    int64_t playback_duration_millis,
    size_t expected_number_of_messages_on_topic1 = 3,
    size_t expected_number_of_messages_on_topic2 = 3)

and specialized function

void InitPlayerWithPlaybackDurationAndPlay(
    size_t expected_number_of_messages_on_topic1 = 3,
    size_t expected_number_of_messages_on_topic2 = 3,
    int64_t playback_duration_millis) {
    return InitPlayerWithPlaybackUntilAndDurationAndPlay(
       expected_number_of_messages_on_topic1, 
       expected_number_of_messages_on_topic2,
       -1,
       playback_duration_millis);
}

Also I would appreciate if you will address residual renaming of the play_for to the playback_duration.

Looking forward to see rebased version when #960 will be merged on master.

ros2bag/ros2bag/verb/play.py Show resolved Hide resolved
ros2bag/ros2bag/verb/play.py Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
Signed-off-by: Agustin Alba Chicar <[email protected]>
Signed-off-by: Agustin Alba Chicar <[email protected]>
Signed-off-by: Agustin Alba Chicar <[email protected]>
… in PlayOptions to clarify usage.

Signed-off-by: Agustin Alba Chicar <[email protected]>
…ayback_until stamp

Signed-off-by: Agustin Alba Chicar <[email protected]>
- Unifies the use of '-' instead of '_' to split words in arguments.
- Uses mutually exclusive arguments: --playback-until-sec and --playback-until-nsec
  to pass the timestamp.

Signed-off-by: Agustin Alba Chicar <[email protected]>
@agalbachicar agalbachicar force-pushed the agalbachicar/play_until branch 2 times, most recently from 190c290 to 2ef415e Compare June 7, 2022 15:27
- Adds test cases to the player.
- Renames play_until and similar occurrences to contain timestamp.
- Renames play_for and similar occurrences to not contain for and include duration.

Signed-off-by: Agustin Alba Chicar <[email protected]>
@agalbachicar
Copy link
Contributor Author

As regards to the tests. It would be nice to move common code as RosBag2PlayUntilTestFixture and RosBag2PlayForTestFixture in some common place and combined them to avoid code duplication. For instance it could be a common function

Would you be OK with doing this in a follow up pull request?

@agalbachicar
Copy link
Contributor Author

@MichaelOrlov let me know if you prefer to merge #1024 into this branch or you prefer to keep them in different PRs.

@MichaelOrlov
Copy link
Contributor

@agalbachicar

@MichaelOrlov let me know if you prefer to merge #1024 into this branch or you prefer to keep them in different PRs.

Let's do it in a separate PR. I am afraid that it could last longer for code review and iterations.

Signed-off-by: Agustin Alba Chicar <[email protected]>
@agalbachicar
Copy link
Contributor Author

I found that test_play_services__rmw_fastrtps_cpp --> play_next test fails in https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/122/consoleFull with a timeout. However, I am not able to reproduce it locally. @MichaelOrlov are you able to re-run it?

@agalbachicar
Copy link
Contributor Author

BTW, @MichaelOrlov all comments were addressed. PTAL when you have a moment.

@MichaelOrlov
Copy link
Contributor

BTW, @MichaelOrlov all comments were addressed. PTAL when you have a moment.

@agalbachicar Overall looks good. However I found one not addressed request. Would you address my comment in regards to the play_until_is_equal_to_the_total_duration from #1005 (comment) ?

As regards to that tests test_play_services__rmw_fastrtps_cpp --> play_next test fails in https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/122/consoleFull with a timeout.
Yes I am able to reproduce this failure. See my comment here #862 (comment). And don't worry it doesn't relates to your changes in this PR.

The test body now evaluates a bag whose duration is exactly the same
as given timestamp in PlayOptions.

Signed-off-by: Agustin Alba Chicar <[email protected]>
@agalbachicar
Copy link
Contributor Author

@agalbachicar Overall looks good. However I found one not addressed request. Would you address my comment in regards to the play_until_is_equal_to_the_total_duration from #1005 (comment) ?

Sorry, I missed it. It should be done in fa3654f @MichaelOrlov

Copy link
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.

@agalbachicar Approving!
Thanks for you hard work.

@agalbachicar
Copy link
Contributor Author

@jhdcs anything to add?

BTW, I don't see the merge button so I guess you have merge powers @MichaelOrlov and @jhdcs

@MichaelOrlov
Copy link
Contributor

I will run CI today and if everything is good will merge it.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/a4d1767e3221ad9709f5bfe56bc172e5/raw/6c1a4538436c74628c38b102e682e71193e17ee9/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport ros2bag rosbag2_py rosbag2_tests
TEST args: --packages-above rosbag2_transport ros2bag rosbag2_py rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10438

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

@MichaelOrlov MichaelOrlov merged commit ff482f0 into ros2:master Jun 21, 2022
@ros-discourse
Copy link

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-8-18-2022/27050/1

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.

6 participants