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.

Sign up for GitHub

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

Jump to bottom

rosbag2_py pybind wrapper for "record" - remove rosbag2_transport_py #702

Merged
merged 8 commits into from
Mar 31, 2021
Merged

rosbag2_py pybind wrapper for "record" - remove rosbag2_transport_py #702

merged 8 commits into from
Mar 31, 2021

Conversation

emersonknapp
Copy link
Collaborator

Depends on #693

Followup to rosbag2_py::Player

Add a rosbag2_py::Recorder - and expose RecordOptions to Python. Remove rosbag2_transport_python.cpp - it is fully replaced.

Emerson Knapp added 4 commits March 30, 2021 15:08
rosbag2_py pybind wrapper for "record" verb
8adeef3 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Export recorder
a88aec5 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Expose StorageOptions::storage_preset_profile
57c0e96 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Misnamed argument in recordoptions init
cae21f6 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp requested a review from a team as a code owner March 31, 2021 01:19
@emersonknapp emersonknapp requested review from jaisontj and prajakta-gokhale and removed request for a team March 31, 2021 01:19
Karsten1987
Karsten1987 approved these changes Mar 31, 2021
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

sweet!

LGTM with green CI.

@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/e82c17523b5d1535935dc776b2246fc9/raw/dc1536d0818be644c16e0d3e7341e1f4c8107608/ros2.repos
BUILD args: --packages-up-to rosbag2_tests rosbag2_transport ros2bag rosbag2_py rosbag2
TEST args: --packages-select rosbag2_tests rosbag2_transport ros2bag rosbag2_py rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8045

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

Emerson Knapp added 4 commits March 31, 2021 01:30
WIP checkpoint
ed2b173 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Actually seems to work
b8f4976 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Simplify code by getting rid of initializers for Options
854ebdd 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Fix flake8
1f289f1 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator Author

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

@emersonknapp
Copy link
Collaborator Author

Windows cmake warning is unrelated - it's from the rti cmake module

@emersonknapp emersonknapp merged commit 8b1308b into master Mar 31, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/ros2bag-record-pybind branch March 31, 2021 18:04
@emersonknapp emersonknapp mentioned this pull request Mar 31, 2021
Closed
50 tasks
@Karsten1987
Copy link
Collaborator

it looks like that change introduced a regression on the tests. We see the same test failures on all platforms:
https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_debug/1559/testReport/junit/(root)/projectroot/test_transport_py/

@emersonknapp
Copy link
Collaborator Author

That's no good - I am looking into it

@Karsten1987
Copy link
Collaborator

according to @sloretz we might want to get rid of pycapsules completely:
We're using the pycapsules QoS here: https://github.com/ros2/rosbag2/pull/702/files#diff-c85fd024e7d68c762d60d25e2ba155bcc527f04c937ed09adc893ea241fdc61cR45 but apparently that got removed.

see https://github.com/ros2/rclpy/pull/741/files

@emersonknapp
Copy link
Collaborator Author

Oh - we must have had a race condition in that I ran my CI before that got pushed. Thanks for the context. I am thinking that I would prefer to patch fix this instead of revert, right now.

@emersonknapp emersonknapp mentioned this pull request Apr 1, 2021
Merged
emersonknapp pushed a commit that referenced this pull request Apr 1, 2021
Fix rosbag2_py transport test for py capsule
5cf91cc 
Change introduced by ros2/rclpy#741 conflicted with parallel work in #702, the combination causing nightly test failure for `rosbag2_py`'s `_transport.cpp` tests. Fix to use the new `py::object` for `rmw_qos_profile_t` to fix the tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
emersonknapp pushed a commit that referenced this pull request Apr 1, 2021
Fix rosbag2_py transport test for py capsule
f2b2401 
Change introduced by ros2/rclpy#741 conflicted with parallel work in #702, the combination causing nightly test failure for `rosbag2_py`'s `_transport.cpp` tests. Fix to use the new `py::object` for `rmw_qos_profile_t` to fix the tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp mentioned this pull request Apr 1, 2021
Merged
emersonknapp added a commit that referenced this pull request Apr 1, 2021
@emersonknapp
Fix rosbag2_py transport test for py capsule (#707)
f1d7d92 
* Fix rosbag2_py transport test for py capsule

Change introduced by ros2/rclpy#741 conflicted with parallel work in #702, the combination causing nightly test failure for `rosbag2_py`'s `_transport.cpp` tests. Fix to use the new `py::object` for `rmw_qos_profile_t` to fix the tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Kettenhoax pushed a commit to Kettenhoax/rosbag2 that referenced this pull request Apr 9, 2021
@emersonknapp
Fix rosbag2_py transport test for py capsule (ros2#707)
ecc691b 
* Fix rosbag2_py transport test for py capsule

Change introduced by ros2/rclpy#741 conflicted with parallel work in ros2#702, the combination causing nightly test failure for `rosbag2_py`'s `_transport.cpp` tests. Fix to use the new `py::object` for `rmw_qos_profile_t` to fix the tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@jaisontj jaisontj Awaiting requested review from jaisontj

@prajakta-gokhale prajakta-gokhale Awaiting requested review from prajakta-gokhale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emersonknapp @Karsten1987