rosidl_python icon indicating copy to clipboard operation
rosidl_python copied to clipboard

Python type annotation

Open MrBlenny opened this issue 3 years ago • 7 comments

Feature request

Feature description

It would be useful if the python classes included type annotations.

For example:

Parent.msg

Child[] children
Child child

Child.msg

uint8 some_child_content

Would generate something like:


+from perception_msgs.msg import Child

class Parent(metaclass=Metaclass_Parent):
    """Message class 'Parent'."""

    __slots__ = [
        '_children',
        '_child',
    ]

    _fields_and_field_types = {
        'children': 'sequence<perception_msgs/Child>',
        'child': 'perception_msgs/Child',
    }

    SLOT_TYPES = (
        rosidl_parser.definition.UnboundedSequence(rosidl_parser.definition.NamespacedType(['perception_msgs', 'msg'], 'Child')),  # noqa: E501
        rosidl_parser.definition.NamespacedType(['perception_msgs', 'msg'], 'Child'),  # noqa: E501
    )

-   def __init__(self, **kwargs):
+   def __init__(self, children: Optional[List[Child]]=None, child: Optional[Child]=None):
-       assert all('_' + key in self.__slots__ for key in kwargs.keys()), \
-           'Invalid arguments passed to constructor: %s' % \
-           ', '.join(sorted(k for k in kwargs.keys() if '_' + k not in self.__slots__))
-       self.children = kwargs.get('children', [])
-       from perception_msgs.msg import Child
-       self.child = kwargs.get('child', Child())
+       self.children = children or []
+       self.child = child or Child()

    def __repr__(self):
        typename = self.__class__.__module__.split('.')
        typename.pop()
        typename.append(self.__class__.__name__)
        args = []
        for s, t in zip(self.__slots__, self.SLOT_TYPES):
            field = getattr(self, s)
            fieldstr = repr(field)
            # We use Python array type for fields that can be directly stored
            # in them, and "normal" sequences for everything else.  If it is
            # a type that we store in an array, strip off the 'array' portion.
            if (
                isinstance(t, rosidl_parser.definition.AbstractSequence) and
                isinstance(t.value_type, rosidl_parser.definition.BasicType) and
                t.value_type.typename in ['float', 'double', 'int8', 'uint8', 'int16', 'uint16', 'int32', 'uint32', 'int64', 'uint64']
            ):
                if len(field) == 0:
                    fieldstr = '[]'
                else:
                    assert fieldstr.startswith('array(')
                    prefix = "array('X', "
                    suffix = ')'
                    fieldstr = fieldstr[len(prefix):-len(suffix)]
            args.append(s[1:] + '=' + fieldstr)
        return '%s(%s)' % ('.'.join(typename), ', '.join(args))

    def __eq__(self, other):
        if not isinstance(other, self.__class__):
            return False
        if self.children != other.children:
            return False
        if self.child != other.child:
            return False
        return True

    @classmethod
    def get_fields_and_field_types(cls):
        from copy import copy
        return copy(cls._fields_and_field_types)

    @property
-   def children(self):
+   def children(self) -> List[Child]:
        """Message field 'children'."""
        return self._children

    @children.setter
-   def children(self, value):
+   def children(self, value: List[Child]):
        if __debug__:
            from perception_msgs.msg import Child
            from collections.abc import Sequence
            from collections.abc import Set
            from collections import UserList
            from collections import UserString
            assert \
                ((isinstance(value, Sequence) or
                  isinstance(value, Set) or
                  isinstance(value, UserList)) and
                 not isinstance(value, str) and
                 not isinstance(value, UserString) and
                 all(isinstance(v, Child) for v in value) and
                 True), \
                "The 'children' field must be a set or sequence and each value of type 'Child'"
        self._children = value

    @property
-   def child(self):
+   def child(self) -> Child: 
        """Message field 'child'."""
        return self._child

    @child.setter
-   def child(self, value):
+   def child(self, value: Child):    
        if __debug__:
            from perception_msgs.msg import Child
            assert \
                isinstance(value, Child), \
                "The 'child' field must be a sub message of type 'Child'"
        self._child = value

Implementation considerations

  • There seems to be an issue with typing @property getters/setters. For me, using pylance the above syntax thinks child is of type property rather than type Child
  • Backwards compatibility requirements - do we need to support python < 3.8? I assume type annotations break old python.
  • Another path.... from dataclasses import dataclass gives us most of these features and more. Plus, it can be used with other libraries like dacite

MrBlenny avatar Apr 19 '22 04:04 MrBlenny

  • Backwards compatibility requirements - do we need to support python < 3.8? I assume type annotations break old python.

For now, yes. We need to support Python 3.6 for RHEL-8 compatibility.

clalancette avatar Apr 19 '22 12:04 clalancette

What if separate stubs (pyi files, I think) were generated, leaving the actual source code alone?

russkel avatar Apr 20 '22 23:04 russkel

Hello. I implemented rosidl_generator_mypy, which generates .pyi file for each message/service/action file.

  • https://github.com/rospypi/rosidl_generator_mypy

Could you try this repository if you are interested in?

As I think it's good to have the feature in this repository, I haven't published this repository to rosdistro, yet. If it's okay, I am happy to create a pull request to this repository to implement the stub generator feature!

bonprosoft avatar May 03 '22 03:05 bonprosoft

Hi @bonprosoft, thanks for that, nice work.

As I think it's good to have the feature in this repository, I haven't published this repository to rosdistro, yet. If it's okay, I am happy to create a pull request to this repository to implement the stub generator feature!

Good idea. I think it should be included.

russkel avatar May 03 '22 04:05 russkel

This is fantastic! It works wonders.

I have added a small https://github.com/rospypi/rosidl_generator_mypy/pull/8 to help fix an issue I was having with some slight issues with array type annotations.

Thanks so much for your work. It would be fantastic to see this included in rosidl_python directly (and for the various common_interfaces be built directly with pyi support).

MrBlenny avatar May 16 '22 05:05 MrBlenny

Thank you @MrBlenny ! I'm so glad to hear that.

It would be fantastic to see this included in rosidl_python directly (and for the various common_interfaces be built directly with pyi support).

I think so, too! I would appreciate if I could get any feedback from maintainer.


Also, I noticed that py.typed is missing in rclpy, which is problematic for mypy to work with the type annotation of rclpy.

https://github.com/ros2/rclpy/issues/936

It would be wonderful if ROS2 packages work nicely with mypy.

bonprosoft avatar May 17 '22 16:05 bonprosoft

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/python-type-checking-in-ros2/28507/1

ros-discourse avatar Nov 24 '22 22:11 ros-discourse