Python Message Container Type Misinformation

[InvincibleRMC]

Issue Type

• 🐛 Bug / Problem
• ✏️ Typo / Grammar
• 📖 Outdated Content
• 🚀 Enhancement

Generated by Generative AI

No response

Distribution

All

Description

The documentation here claims that the container type for arrays/unbounded/bounded is builtins.list. But at run time it can be various other container types. E.j Sequence or set. This can matter for users since they cannot use list specific methods type safely e.j. append.

Affected Pages/Sections

No response

Screenshots or Examples (if applicable)

Suggested Fix

Add a note about what container actually exist for each type.

Additional Context

No response

[christophebedard]

But at run time it can be various other container types. E.j Sequence or set

Can you point to the corresponding code just to confirm this? Then a PR to clarify/fix the docs would be appreciated!

[InvincibleRMC]

Can you point to the corresponding code just to confirm this? Then a PR to clarify/fix the docs would be appreciated!

Just one example. Its honesly easiest to see by building test_interface_files/Arrays.msg

https://github.com/ros2/rosidl_python/blob/e4c419f880e266533297573931f4576bc368ae13/rosidl_generator_py/resource/_msg.py.em#L543-L547

[christophebedard]

Interesting:

>>> import os
>>> os.environ['ROS_PYTHON_CHECK_FIELDS'] = '1'
>>> from std_msgs.msg import ByteMultiArray
>>> msg = ByteMultiArray()
>>> msg.data
[]
>>> msg.data = 'abc'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/christophe.bedard/ros2_ws/install/std_msgs/lib/python3.12/site-packages/std_msgs/msg/_byte_multi_array.py", line 182, in data
    ((isinstance(value, Sequence) or
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: The 'data' field must be a set or sequence and each value of type 'bytes'
>>> msg.data = [1, 2, 3]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/christophe.bedard/ros2_ws/install/std_msgs/lib/python3.12/site-packages/std_msgs/msg/_byte_multi_array.py", line 182, in data
    ((isinstance(value, Sequence) or
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: The 'data' field must be a set or sequence and each value of type 'bytes'
>>> msg.data = [b'a', b'b', b'c']
>>> msg.data
[b'a', b'b', b'c']
>>> msg.data = {b'a', b'b', b'c'}
>>> msg.data
{b'a', b'b', b'c'}
>>> msg.data.append(b'd')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'set' object has no attribute 'append'
>>> msg.data = [b'a', b'b', b'c']
>>> msg.data.append(b'd')
>>> msg.data
[b'a', b'b', b'c', b'd']

Yeah, it would be good to mention this!

[InvincibleRMC]

@christophebedard How does one actually test the python type_support? I want make sure sets actually work when turned from Python into C.

[christophebedard]

good question 🤔 have you tried just creating a pub/sub pair with a message type with a field that can map to a set?

As for making sure this is covered by the tests, rosidl_generator_py's tests would be ideal (e.g., test_interfaces.py), but it's Python only. Python messages only get converted to C on publication and converted from C after they get taken.

Depending on the type, it might be covered by the rclpy tests or the tests in test_communication. I think test_communication runs pub/sub tests using all test messages (e.g., test_interface_files and test_msgs' own msgs). Looks like it properly checks that the received message is equal to the test message that the publisher retrieved from test_msgs: https://github.com/ros2/system_tests/blob/976a7a6021fa629ad97741969101685af386fe01/test_communication/test/subscriber_py.py#L28-L42.

In conclusion, check that something here uses Python sets, otherwise add it (and make necessary downstream changes in the actual tests): https://github.com/ros2/rcl_interfaces/blob/rolling/test_msgs/src/test_msgs/message_fixtures.py

[InvincibleRMC]

So there isn't any set() being used in any tests but, more concerning is that set order is not deterministic.

# Sometimes works, sometimes crashes
b = ["", "max value", "min value"]
a = set(b)

for i, j in zip(a, b, strict=True):
    print(f'list: {i} set: {j}')
    assert i == j

So sometimes python message will be equal each other but, sometimes not. I find it extremely hard to believe anyone is actually using sets. When I added it to system_tests the amount of failures changes on every run.

[InvincibleRMC]

I think there should probably be a discussion about the continued support of set. Either we need to implement/import some OrderedSet implementation to prevent data re-ordering. Or we need to deprecate/remove. Unfortunately the isinstance(value, Set) check has been around 8 years so someone could theoretical be using it even though it hasn't been working or listed in the documentation.

[christophebedard]

sets are inherently not ordered, so that shouldn't matter when comparing them, no?

>>> set(['a', 'b', 'c']) == set(['b', 'a', 'c'])
True

[InvincibleRMC]

Yeah when comparing sets its fine but, when the message gets converted into C and back out again it uses PySequence_Fast. Which works by first checking if the incoming object is a list or tuple then returns them else if attempt a list cast of objects iterator. PySequence_Fast Implementation.

Since the incoming object is a set the calls look like. Which is non-deterministic since the iter method of sets in non-deterministic.

print(list(iter({'a', 'b', 'c'})))

Reference of one array field from _arrays_s.c

{  // bool_values
    PyObject * field = PyObject_GetAttrString(_pymsg, "bool_values");
    if (!field) {
      return false;
    }
    {
      PyObject * seq_field = PySequence_Fast(field, "expected a sequence in 'bool_values'");
      if (!seq_field) {
        Py_DECREF(field);
        return false;
      }
      Py_ssize_t size = 3;
      bool * dest = ros_message->bool_values;
      for (Py_ssize_t i = 0; i < size; ++i) {
        PyObject * item = PySequence_Fast_GET_ITEM(seq_field, i);
        if (!item) {
          Py_DECREF(seq_field);
          Py_DECREF(field);
          return false;
        }
        assert(PyBool_Check(item));
        bool tmp = (item == Py_True);
        memcpy(&dest[i], &tmp, sizeof(bool));
      }
      Py_DECREF(seq_field);
    }
    Py_DECREF(field);
  }