search

ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
268 stars 221 forks source link

Revert "Add types to TypeHash and moved away from __slots__ usage" #1243

Closed clalancette closed 3 months ago

clalancette commented 3 months ago

Reverts ros2/rclpy#1232

This is causing problems in CI: https://ci.ros2.org/job/ci_linux/20452/testReport/junit/ros2cli.ros2cli.test/test_daemon/test_get_subscriptions_info_by_topic/

In particular, it isn't clear to me why we removed the __slots__ (as that is a performance enhancement), and it is also causing downstream packages to fail.

InvincibleRMC commented 3 months ago

Ah my apologies. I misunderstood the purpose of __slots__.

clalancette commented 3 months ago

CI:

fujitatomoya commented 3 months ago

In particular, it isn't clear to me why we removed the slots (as that is a performance enhancement), and it is also causing downstream packages to fail.

@clalancette thanks for taking care of this. sorry i thought there is no dependency for TypeHash.__slot__ but there is https://github.com/ros2/ros2cli/blob/64d216cb8fafef83d046b79ee6294afb06b7c595/ros2cli/ros2cli/xmlrpc/marshal/rclpy.py#L90-L91

and as you mentioned, i think we should keep __slots__ for performance and security.