Skip to content

Create DDSTransport DDSPubSubBase#1036

Closed
Kaweees wants to merge 18 commits intodevfrom
miguel_pickle_dds
Closed

Create DDSTransport DDSPubSubBase#1036
Kaweees wants to merge 18 commits intodevfrom
miguel_pickle_dds

Conversation

@Kaweees
Copy link
Member

@Kaweees Kaweees commented Jan 16, 2026

Related: #1033, #1058

uv run pytest -svm tool dimos/protocol/pubsub/benchmark/test_benchmark.py --override-ini="addopts=" -k "DDS"
uv run pytest -svm tool dimos/protocol/pubsub/benchmark/test_benchmark.py

@Kaweees Kaweees requested a review from a team January 16, 2026 02:10
@Kaweees Kaweees self-assigned this Jan 16, 2026
@Kaweees Kaweees marked this pull request as draft January 16, 2026 02:10
@Kaweees Kaweees changed the base branch from main to dev January 16, 2026 02:13
@Kaweees
Copy link
Member Author

Kaweees commented Jan 16, 2026

@greptile

@Kaweees Kaweees changed the title Add Create PickleDDS, Jan 16, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 16, 2026

Greptile Overview

Greptile Summary

This PR introduces a DDS (Data Distribution Service) transport layer using CycloneDDS, creating DDSPubSubBase, DDSService, and DDSTransport classes with benchmark tests.

Major changes:

  • Implements proper DDS pub/sub with DataWriter/DataReader and listener-based message dispatch via _DDSMessageListener
  • Uses lazy initialization for DomainParticipant with double-checked locking for thread safety
  • Adds benchmark tests comparing DDS performance with other transports (LCM, shared memory)
  • Uses IdlStruct with sequence[uint8] for binary payloads to minimize serialization overhead

Critical issues:

  • Attribute mismatch in transport.py:233: DDSTransport.__reduce__ accesses self.topic.dds_type but the Topic class only has typename attribute, causing AttributeError at runtime during serialization
  • Duplicate imports and variable definitions in transport.py (lines 17, 23-26, 37)

The implementation correctly addresses previously reported architectural issues by actually publishing to the DDS network and receiving messages through listeners, rather than just using local callbacks.

Confidence Score: 2/5

  • This PR has a critical runtime error that will break serialization
  • The attribute mismatch in DDSTransport.__reduce__ will cause AttributeError when the transport is serialized (e.g., during pickling for multiprocessing). This is a blocking issue that needs to be fixed before merge.
  • Pay close attention to dimos/core/transport.py - the __reduce__ method has incorrect attribute access that will fail at runtime

Important Files Changed

Filename Overview
dimos/core/transport.py Added DDSTransport class with incorrect attribute access - uses self.topic.dds_type but Topic class only has typename attribute
dimos/protocol/pubsub/ddspubsub.py Implements DDSPubSubBase with proper DataWriter/DataReader creation and listener-based message dispatch, resolving previous architectural issues
dimos/protocol/service/ddsservice.py New DDSService base class with lazy DomainParticipant initialization using double-checked locking pattern

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant DT as DDSTransport
    participant DDS as DDS (DDSPubSubBase)
    participant DS as DDSService
    participant DP as DomainParticipant
    participant DW as DataWriter
    participant DR as DataReader
    participant L as _DDSMessageListener

    Note over App,L: Initialization
    App->>DT: __init__(topic, type)
    DT->>DT: Create Topic(name, typename)
    DT->>DDS: Create DDS instance
    
    App->>DT: start()
    DT->>DDS: start()
    DDS->>DS: get_participant()
    DS->>DP: Create DomainParticipant (lazy init)
    
    Note over App,L: Publishing Flow
    App->>DT: broadcast(msg)
    DT->>DDS: publish(topic, msg)
    DDS->>DDS: _get_writer(topic)
    DDS->>DW: Create DataWriter (if needed)
    DDS->>DW: write(message)
    DW->>DP: Send to DDS network
    
    Note over App,L: Subscription Flow
    App->>DT: subscribe(callback)
    DT->>DDS: subscribe(topic, wrapped_callback)
    DDS->>DDS: _get_reader(topic)
    DDS->>L: Create _DDSMessageListener
    DDS->>DR: Create DataReader with listener
    
    Note over App,L: Message Reception
    DP->>DR: Incoming DDS message
    DR->>L: on_data_available(reader)
    L->>DR: take() samples
    L->>L: Iterate over samples
    L->>App: callback(sample, topic)
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@Kaweees Kaweees changed the title Create PickleDDS, Create DDSPubSubBase and PickleDDS Jan 16, 2026
@Kaweees Kaweees changed the title Create DDSPubSubBase and PickleDDS [DRAFT] Create DDSPubSubBase and PickleDDS Jan 16, 2026
@Kaweees
Copy link
Member Author

Kaweees commented Jan 16, 2026

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@Kaweees
Copy link
Member Author

Kaweees commented Jan 16, 2026

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@Kaweees
Copy link
Member Author

Kaweees commented Jan 16, 2026

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@Kaweees
Copy link
Member Author

Kaweees commented Jan 18, 2026

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@Kaweees Kaweees changed the title [DRAFT] Create DDSPubSubBase and PickleDDS [DRAFT] Create DDSTransport DDSPubSubBase Jan 18, 2026
* raw rospubsub and benchmarks

* typefixes, shm added to the benchmark

* SHM is not so important to tell us every time when it starts

* greptile comments

* Add co-authorship line to commit message filter patterns

* Remove unused contextmanager import

---------

Co-authored-by: Ivan Nikolic <lesh@sysphere.org>
Replace base64 string encoding with native IDL bytearray type to eliminate
buffer overflow issues. The original base64 encoding exceeded CycloneDDS's
default string size limit (~256 bytes) and caused crashes on messages >= 1KB.

Key changes:
- Use make_idl_struct with bytearray field instead of string
- Convert bytes to bytearray when publishing to DDS
- Convert bytearray back to bytes when receiving from DDS
- Add _DDSMessageListener for async message dispatch
- Implement thread-safe DataWriter/DataReader management
- Add pickle support via __getstate__/__setstate__

Result: All 12 DDS benchmark tests pass (64B to 10MB messages).
@Kaweees
Copy link
Member Author

Kaweees commented Jan 19, 2026

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +82 to +95
class Message(IdlStruct):
"""DDS message with binary data payload following IdlStruct format."""

payload: str

def dds_encode(self) -> bytes:
"""Encode message to bytes for DDS transmission."""
return self.payload.encode("latin-1")

@classmethod
def dds_decode(cls, data: bytes) -> "Message":
"""Decode bytes back to Message instance."""
return cls(payload=data.decode("latin-1"))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Test Message class uses string encoding which breaks with binary data. The make_data() function generates binary bytes, but encoding as latin-1 string will fail for many byte sequences.

DDS IdlStruct should use bytes type directly:

Suggested change
class Message(IdlStruct):
"""DDS message with binary data payload following IdlStruct format."""
payload: str
def dds_encode(self) -> bytes:
"""Encode message to bytes for DDS transmission."""
return self.payload.encode("latin-1")
@classmethod
def dds_decode(cls, data: bytes) -> "Message":
"""Decode bytes back to Message instance."""
return cls(payload=data.decode("latin-1"))
@dataclass
class Message(IdlStruct):
"""DDS message with binary data payload following IdlStruct format."""
payload: bytes

Remove the encode/decode methods as CycloneDDS handles serialization of IdlStruct fields automatically.

The double-checked locking pattern avoids lock contention on every
call after initial object creation. Initial benchmarking shows this
pattern performs better than simple locking for repeated accesses
to the same topics.
@Kaweees
Copy link
Member Author

Kaweees commented Jan 20, 2026

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 20, 2026

Additional Comments (2)

dimos/core/transport.py
TypeVar imported twice


dimos/core/transport.py
TypeVar defined twice with conflicting type ignore comments

@@ -0,0 +1,213 @@
#!/usr/bin/env python3
Copy link
Contributor

@paul-nechifor paul-nechifor Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file in on dev now. You might want to rebase onto dev so that it it disappears from the diff of this PR.

@Kaweees Kaweees changed the title [DRAFT] Create DDSTransport DDSPubSubBase Create DDSTransport DDSPubSubBase Jan 23, 2026
@Kaweees Kaweees closed this Jan 24, 2026
@Kaweees Kaweees deleted the miguel_pickle_dds branch January 24, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants