Added method to introspect QoS in Python #1648
Conversation
ahcorde
requested review from
MichaelOrlov and
hidmic
and removed request for
a team
There was a problem hiding this comment.
@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
rosbag2_py/rosbag2_py/_storage.pyi
Outdated
| def depth(self) -> int: ... | ||
| @overload | ||
| def durability(self) -> rmw_qos_durability_policy_t: ... | ||
| @overload |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
|
|
Let's see if we can find a better solution instead of duplicate the class in |
|
@ahcorde Am I understand correctly that this PR is not a blocker for Jazzy release? |
|
@MichaelOrlov, no, it's not a blocker |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
ahcorde
force-pushed
the
ahcorde/rolling/python_qos
branch
from
ef6c985
to
ebca8d1
Compare
|
@clalancette @MichaelOrlov waht about this solution ? convert the Rosbag rclcpp::QoS warpper into rclcpy QoSProfile Python class with a new function Code in |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
There was a problem hiding this comment.
@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.
| auto history = rclcpp::HistoryPolicy::KeepLast; | ||
| if (qos_input.history() != rclcpp::HistoryPolicy::Unknown) { | ||
| history = qos_input.history(); | ||
| } |
There was a problem hiding this comment.
@ahcorde Out of curiosity why this "hack" with history is needed?
There was a problem hiding this comment.
For some reason the history is always returning unknown, I dediced to include this hack.
Maybe I should add a TODO here, what do you think?
There was a problem hiding this comment.
@ahcorde As regards
For some reason the history is always returning unknown
Could you please elaborate on that? In what cases? How to reproduce?
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
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 |
Required to fix the QoS of the topic in rqt. Related PR ros-visualization/rqt_bag#156