eclipse-cyclonedds / cyclonedds-python

Other
59 stars 46 forks source link

get_types_for_typeid: AttributeError: 'PlainCollectionHeader' object has no attribute 'IS_OPTIONAL' #80

Closed ghorn closed 2 years ago

ghorn commented 2 years ago

I'm not sure if this is a bug report or incorrect usage.

I'm trying to get type discovery working. My goal is to write an application that discovers types on a domain and can receive, introspect, and publish data of these dynamically discovered types. This includes types sent by the C++ and python bindings to cyclonedds.

Here's what I came up with:

from cyclonedds.dynamic import get_types_for_typeid
from cyclonedds.domain import DomainParticipant
from cyclonedds.builtin import BuiltinDataReader, BuiltinTopicDcpsPublication
from cyclonedds.util import duration

def dynamic_subscribe():
    # Create a DomainParticipant in domain 0
    dp = DomainParticipant()

    # Create a datareader that can read from the builtin participant topic
    publication_reader = BuiltinDataReader(dp, BuiltinTopicDcpsPublication)

    for sample in publication_reader.take_iter(timeout=duration(milliseconds=10)):
        # print out information about the sample
        print(f'detected publication: {sample.topic_name}, {sample.type_name}')
        print(sample)
        print(f'sample.type_id: {sample.type_id}')

        # get the type so that we can dynamically read and send messages
        print('getting types for typeid...')
        datatype, _ = get_types_for_typeid(dp, sample.type_id, duration(seconds=1))  # this crashes

if __name__ == "__main__":
    dynamic_subscribe()

This code raises an exception:

Traceback (most recent call last):
  File "dynamic_listener.py", line 162, in <module>
    dynamic_subscribe()
  File "dynamic_listener.py", line 130, in dynamic_subscribe
    datatype, _ = get_types_for_typeid(dp, sample.type_id, duration(seconds=1))
  File "cyclonedds/dynamic.py", line 57, in get_types_for_typeid
    return XTInterpreter.xt_to_class(type_id, typemap)
  File "cyclonedds/idl/_xt_builder.py", line 1417, in xt_to_class
    main_type = cls._from_typeid(ident, state)
  File "cyclonedds/idl/_xt_builder.py", line 1457, in _from_typeid
    return cls._from_typeobject(ident, state)
  File "cyclonedds/idl/_xt_builder.py", line 1478, in _from_typeobject
    return cls._make_complete_struct(ident, complete_obj.struct_type, state)
  File "cyclonedds/idl/_xt_builder.py", line 1521, in _make_complete_struct
    m_type = cls._from_typeid(m.common.member_type_id, state)
  File "cyclonedds/idl/_xt_builder.py", line 1451, in _from_typeid
    if descriptor.header.IS_OPTIONAL:
AttributeError: 'PlainCollectionHeader' object has no attribute 'IS_OPTIONAL'

Am I approaching this correctly? Does the functionality I'm looking for supported? Thanks.

thijsmie commented 2 years ago

Hi @ghorn!

Yeah this should work, but this part of the code definitely needs some battle testing and ideally should be part of the type-fuzzing testsuite. In this case it is a simple typo, the IS_OPTIONAL flag is part of header.element_flags and not directly set on header. Could you verify that my branch here: https://github.com/thijsmie/cyclonedds-python/tree/element_flags_typo fixes your problem?

ghorn commented 2 years ago

I tested your patch (not your branch) and I confirm it works and I recover the IdlStruct. Thanks!

Side note, I had to monkey-patch it in because I can only run with a pip binary distribution at the moment. I thought you might be interested because of how dirty it was:

"""
Basic app that listens to control telemetry and prints it out.
"""

# avert your eyes
import os
os.system("sed -i 's/descriptor.header.IS_OPTIONAL/descriptor.header.element_flags.IS_OPTIONAL/g' /long/path/to/pip/install/location/cyclonedds/idl/_xt_builder.py")

from cyclonedds.dynamic import get_types_for_typeid
from cyclonedds.domain import DomainParticipant
from cyclonedds.builtin import BuiltinDataReader, BuiltinTopicDcpsParticipant, BuiltinTopicDcpsSubscription, BuiltinTopicDcpsPublication, BuiltinTopicDcpsTopic
from cyclonedds.util import duration
from cyclonedds.core import DDSException

# etc etc

Now that I think about it, I guess I could just commit this locally.........

ghorn commented 2 years ago

for those who are morbidly curious, the "portable" version is

# Monkey patch in a fix for dynamic discovery.
# See https://github.com/eclipse-cyclonedds/cyclonedds-python/issues/80#issuecomment-1112889825.
# This is actually the "portable" version, lol.
# Only God can judge me.
import importlib.util
cyclonedds_path = importlib.util.find_spec('cyclonedds').loader.path.removesuffix('/__init__.py')
import os
sed_expr = "s/descriptor.header.IS_OPTIONAL/descriptor.header.element_flags.IS_OPTIONAL/g"
os.system(f"find {cyclonedds_path} -name '_xt_builder.py' | xargs sed -i '{sed_expr}'")

from cyclonedds.dynamic import get_types_for_typeid
.........
thijsmie commented 2 years ago

That is... creative πŸ˜…. I submitted the PR, we'll make sure it works in the next release.

ghorn commented 2 years ago

Thank you! What's the normal release schedule, btw?

thijsmie commented 2 years ago

The release schedule boils down to: "minor releases for big new stuff, patch releases to clean up stuff where needed". What that means for the python backend is to be figured out as we go along, we simply don't have a tradition yet!

Oh, one note for future reference, please leave the issue open till a fix merged. That allows the merge to autoclose the issue, providing some traceability.

ghorn commented 2 years ago

Would you mind making a release for this issue? How automated is the release process?

thijsmie commented 2 years ago

Hi @ghorn,

We're looking to make a release this week. The release process for Python is a simple question of putting a tag on the repository, the rest is automated via CI. There are also pending fixes for C and C++ backends so the plan is to make a synchronised release of 0.9.1.

ghorn commented 2 years ago

nice!

thijsmie commented 2 years ago

Hi @ghorn, Just checking in with you if the release 0.9.1 indeed fixed your issue.

ghorn commented 2 years ago

Thanks for checking in. This does fix the issue. Where should I send the beer?

thijsmie commented 2 years ago

Good to hear! Hope you enjoy using CycloneDDS-Python 😁. Please redirect any beer-money for me towards django girls as I have enough πŸ˜„.

ghorn commented 2 years ago