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

Add support for compression to python API #1425

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Add support for compression to python API #1425

merged 3 commits into from
Jul 20, 2023

Conversation

asymingt
Copy link
Contributor

@asymingt asymingt commented Jul 17, 2023 •
edited
Loading

Attempt to fix #1407.

This PR adds Python bindings for CompressionOptions and CompressionMode, which enables us to correctly construct a SequentialCompressionWriter, which in turn allows us greater flexibility with writing compressed bag files. It also adds a few tests to exercise the new bindings and show that a SequentialCompressionWriter performs as expected using the zstd compression plugin.

@asymingt asymingt requested a review from a team as a code owner July 17, 2023 19:26
@asymingt asymingt requested review from gbiggs and jhdcs and removed request for a team July 17, 2023 19:26
@asymingt asymingt force-pushed the asymingt/export-compression-options branch from 7750791 to 04d394c Compare July 17, 2023 19:26
@asymingt asymingt mentioned this pull request Jul 17, 2023
Closed
@asymingt asymingt force-pushed the asymingt/export-compression-options branch from 5360fde to 3ed0db8 Compare July 17, 2023 22:30
@asymingt
Add support for compression to python API
2e9d2e0 
Signed-off-by: Andrew Symington <andrew.c.symington@gmail.com>
@asymingt asymingt force-pushed the asymingt/export-compression-options branch from 056855b to 2e9d2e0 Compare July 18, 2023 16:33
@asymingt
Copy link
Contributor Author

Looks like I am having some trouble setting up an environment in CI in such a way where the zstd compression context is available: https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/937/testReport/junit/(root)/projectroot/test_compression_py/

@asymingt
Add the zstd compression plugin as a test dependency for the python API
fa37d34 
Signed-off-by: Andrew Symington <andrew.c.symington@gmail.com>
@asymingt asymingt force-pushed the asymingt/export-compression-options branch from e0714c5 to fa37d34 Compare July 18, 2023 17:57
@asymingt
Copy link
Contributor Author

FIxed the CI issue -- @emersonknapp this is ready for review!

MichaelOrlov
MichaelOrlov requested changes Jul 19, 2023
View reviewed changes
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.

@asymingt Thank you for the PR.
The implementation looks good to me.
However, could you please add verification with the bag read in the test?

rosbag2_py/test/test_compression.py Show resolved Hide resolved
@asymingt
Add reading to the sequential compression test
e24c3a5 
Signed-off-by: Andrew Symington <andrew.c.symington@gmail.com>
@asymingt asymingt force-pushed the asymingt/export-compression-options branch from 3a12ecc to e24c3a5 Compare July 19, 2023 22:37
MichaelOrlov
MichaelOrlov approved these changes Jul 19, 2023
View reviewed changes
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.

LGTM with green CI.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/d3b52e3761849bce82ac7327b8857aad/raw/dd03423ee8be86276b58780846908b53c66573f3/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_py
TEST args: --packages-above rosbag2_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12404

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

@asymingt
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/d3b52e3761849bce82ac7327b8857aad/raw/dd03423ee8be86276b58780846908b53c66573f3/ros2.repos BUILD args: --packages-above-and-dependencies rosbag2_py TEST args: --packages-above rosbag2_py ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12404

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

Looks like the one failure is unrelated, right?
https://ci.ros2.org/job/ci_linux/19193/console

00:01:04 curl failed to verify the legitimacy of the server and therefore could not
00:01:04 establish a secure connection to it. To learn more about this situation and
00:01:04 how to fix it, please visit the web page mentioned above.
00:01:04 gpg: no valid OpenPGP data found.

@MichaelOrlov
Copy link
Contributor

Yes. It seems CI infrastructure issue.
I've scheduled to re-run it one more time.

  • Linux Build Status

@MichaelOrlov MichaelOrlov merged commit 197bb8d into ros2:rolling Jul 20, 2023
@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-2023-07-20/32525/1

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

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

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

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

@emersonknapp emersonknapp Awaiting requested review from emersonknapp

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

Successfully merging this pull request may close these issues.

Setting compression mode when using rosbag2_py.SequentialCompressionWriter
3 participants
@asymingt @MichaelOrlov @ros-discourse