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

Added method to introspect QoS in Python #1648

Merged
merged 12 commits into from
Aug 21, 2024
Merged

Added method to introspect QoS in Python #1648

merged 12 commits into from
Aug 21, 2024

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 9, 2024

Required to fix the QoS of the topic in rqt. Related PR ros-visualization/rqt_bag#156

@ahcorde ahcorde self-assigned this May 9, 2024
@ahcorde ahcorde requested a review from a team as a code owner May 9, 2024 21:44
@ahcorde ahcorde requested review from MichaelOrlov and hidmic and removed request for a team May 9, 2024 21:44
MichaelOrlov
MichaelOrlov reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ahcorde Looks good to me. However, we need to regenerate Python stubs to pass CI. Please see https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/README.md

@ahcorde ahcorde requested a review from MichaelOrlov May 10, 2024 15:18
clalancette
clalancette reviewed May 10, 2024
View reviewed changes
rosbag2_py/rosbag2_py/_storage.pyi Outdated
Comment on lines 57 to 60
def depth(self) -> int: ...
@overload
def durability(self) -> rmw_qos_durability_policy_t: ...
@overload
Copy link
Contributor

Choose a reason for hiding this comment

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

So the more we add to this, the more I wonder why we have this structure at all; it is essentially duplicating what is in https://github.com/ros2/rclpy/blob/rolling/rclpy/rclpy/qos.py .

@MichaelOrlov Can you explain more about this structure, and why we are using this rather than rclpy.QoSProfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess here, it's that QoS on rclpy is a Python class not a pybind11 wrapper class. Make impossible to return this type, that's why it's duplicated. is this assumption right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@clalancette Yes. basically, because this is a pybind11 wrapper class https://github.com/ros2/rosbag2/pull/1648/files#diff-6fb0986aa5f23a37be72468378f8959ba9702304ec4504d318a8c7602211c625R191-R206 sort of converter to the rclcpp::qos class.
I don't know for sure, but I think it is because the all other functions in the rosbag2_py are pybind11 wrappers and need to convert returning or incoming values to/from the rclcpp::qos class.

Copy link

Choose a reason for hiding this comment

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

@MichaelOrlov Just stumbled upon the issue when trying to adapt for the new API changes in our ros2bag_tools.

Two questions that I have is it not possible to just use a typecaster or otherwise directly map rclcpp QoS or make rclpy QoSProfile somehow a direct bind?

As a side note I still think storage QoS is not publically imported.

Copy link
Contributor

Choose a reason for hiding this comment

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

@devrite I am sorry, I don't have a better solution on top of my head. However, constructive suggestions and contributions are welcome.

As regards

As a side note I still think storage QoS is not publically imported.

Could you please elaborate on that?

Copy link

@devrite devrite May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Have not worked with pybind enough to be sure it works but either way, if QoSProfiles can not be replaced by the QoS bind of the current PR on the long run a converter function on the python side or pybind side (typecaster) may be useful.

Regarding my last comment. In the import list above I do not see the QoS type listed. Meaning if somebody wants to use it, when creating a TopicInfo object, you would need to import it currently from _storage and not the public module.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 13, 2024

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

@ahcorde ahcorde marked this pull request as draft May 13, 2024 15:39
@ahcorde
Copy link
Contributor Author

ahcorde commented May 13, 2024

Let's see if we can find a better solution instead of duplicate the class in rclcy and rosbag2_py

@ahcorde ahcorde mentioned this pull request May 13, 2024
Merged
@MichaelOrlov
Copy link
Contributor

@ahcorde Am I understand correctly that this PR is not a blocker for Jazzy release?
cc: @clalancette @marcoag

@ahcorde
Copy link
Contributor Author

ahcorde commented May 14, 2024

@MichaelOrlov, no, it's not a blocker

@ahcorde
Added convert_rosbag_qos_to_rlcpy_qos function
ebca8d1 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/rolling/python_qos branch from ef6c985 to ebca8d1 Compare May 16, 2024 14:10
@ahcorde
Copy link
Contributor Author

ahcorde commented May 16, 2024

@clalancette @MichaelOrlov waht about this solution ? convert the Rosbag rclcpp::QoS warpper into rclcpy QoSProfile Python class with a new function

Code in rqt_bag is much more cleaner https://github.com/ros-visualization/rqt_bag/pull/164/files

ahcorde added 7 commits May 16, 2024 16:14
@ahcorde
make linters happy
910fc2e 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
df8407b 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
98596c7 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
2d8372a 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
6ede3d9 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
61d04ef 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
e7b66bf 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
MichaelOrlov
MichaelOrlov requested changes May 21, 2024
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.

@ahcorde In general, I don't mind having a helper conversion function.
However, the name of the convert_rosbag_qos_to_rlcpy_qos function is misleading.
There is no specific rosbag2::QoS class. We have only rclcpp::QoS.

rosbag2_py/src/rosbag2_py/_storage.cpp Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_storage.cpp Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_storage.cpp Outdated Show resolved Hide resolved
@ahcorde
Added feedback
9b7cca9 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from MichaelOrlov May 22, 2024 13:17
@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-jazzy-jalisco-released/37862/1

@devrite devrite mentioned this pull request Jul 16, 2024
Open
@clalancette clalancette mentioned this pull request Aug 6, 2024
Closed
ahcorde added 2 commits August 20, 2024 13:26
@ahcorde
Merge remote-tracking branch 'origin/rolling' into ahcorde/rolling/py…
0c2cf6d 
…thon_qos
@ahcorde
updated code
77a6e34 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde marked this pull request as ready for review August 20, 2024 11:50
@ahcorde
Fix
f1237a7 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
MichaelOrlov
MichaelOrlov approved these changes Aug 20, 2024
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.

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 21, 2024

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

@ahcorde ahcorde merged commit f0f3cc5 into rolling Aug 21, 2024
12 checks passed
@ahcorde ahcorde deleted the ahcorde/rolling/python_qos branch August 21, 2024 10:47
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 21, 2024

https://github.com/Mergifyio backport jazzy

@mergify Mergify
Copy link

mergify bot commented Aug 21, 2024 •
edited
Loading

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 21, 2024
@ahcorde @mergify
Added method to introspect QoS in Python (#1648)
6e76197 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit f0f3cc5)
@mergify mergify bot mentioned this pull request Aug 21, 2024
Merged
ahcorde added a commit that referenced this pull request Aug 22, 2024
@mergify @ahcorde
Added method to introspect QoS in Python (#1648) (#1790)
173337c 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit f0f3cc5)

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@mergify mergify bot mentioned this pull request Aug 22, 2024
Merged
@MichaelOrlov MichaelOrlov linked an issue Aug 23, 2024 that may be closed by this pull request
[Feature Request] Add getters to the python binding of rclcpp::QoS #1770
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devrite devrite devrite left review comments

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

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

@clalancette clalancette Awaiting requested review from clalancette

Assignees

@ahcorde ahcorde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add getters to the python binding of rclcpp::QoS
5 participants
@ahcorde @MichaelOrlov @ros-discourse @clalancette @devrite