Conversation
Greptile SummaryThis PR adds a complete Zenoh transport layer implementation, providing a high-performance pub/sub protocol option alongside existing LCM, SharedMemory, DDS, and ROS transports. Key Changes:
Implementation Quality:
Minor Note:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ZenohTransport
participant ZenohPubSub
participant ZenohService
participant Session as zenoh.Session
User->>ZenohTransport: __init__(topic, **kwargs)
ZenohTransport->>ZenohPubSub: create instance
ZenohPubSub->>ZenohService: inherit from
User->>ZenohTransport: broadcast(msg) or subscribe(callback)
ZenohTransport->>ZenohTransport: check _started (lazy init)
alt not started
ZenohTransport->>ZenohPubSub: start()
ZenohPubSub->>ZenohService: start()
ZenohService->>Session: zenoh.open(config)
Note over ZenohService,Session: Session stored in _sessions dict
end
alt broadcast
ZenohTransport->>ZenohPubSub: publish(topic, msg)
ZenohPubSub->>ZenohPubSub: _get_publisher(topic)
ZenohPubSub->>Session: declare_publisher(topic.name)
ZenohPubSub->>Session: publisher.put(message)
else subscribe
ZenohTransport->>ZenohPubSub: subscribe(topic, callback)
ZenohPubSub->>Session: declare_subscriber(topic.name, on_sample)
Session-->>ZenohPubSub: subscriber
Note over Session: Background thread spawned
Session->>ZenohPubSub: on_sample(sample)
ZenohPubSub->>User: callback(msg, topic)
end
User->>ZenohTransport: stop()
ZenohTransport->>ZenohPubSub: stop()
ZenohPubSub->>ZenohPubSub: undeclare all publishers
ZenohPubSub->>ZenohPubSub: undeclare all subscribers
Note over Session: Session remains open (singleton)
Last reviewed commit: 3899edc |
| # Transport Protocols | ||
| "dimos-lcm", | ||
| "PyTurboJPEG==1.8.2", | ||
| "eclipse-zenoh>=1.7.2", |
There was a problem hiding this comment.
Check that ZenohTransport implementation in dimos/core/transport.py:322 is complete - currently it's just an empty stub (class ZenohTransport(PubSubTransport[T]): ...)
| # Transport Protocols | ||
| "dimos-lcm", | ||
| "PyTurboJPEG==1.8.2", | ||
| "eclipse-zenoh>=1.7.2", |
There was a problem hiding this comment.
Check if docs/usage/transports.md should be updated to document Zenoh transport alongside LCM, SharedMemory, DDS, and ROS transports
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Where are you handling bridge vs host discovery mode? pretty importatnt for zenoh |
Stash I mentioned those two things briefly but there are also 55 other things that are zenoh. someone familair with transport protocols needs to seriously go through the documentation and think about this integration |
Here is what @leshy is referring to. I will extend |
|
@greptile |
| config.insert_json5("connect/endpoints", json.dumps(self.config.connect)) | ||
| if self.config.listen: | ||
| config.insert_json5("listen/endpoints", json.dumps(self.config.listen)) | ||
| _sessions[key] = zenoh.open(config) |
There was a problem hiding this comment.
Zenoh sessions in _sessions dict are never closed - sessions accumulate without cleanup. Consider adding reference counting or explicit cleanup in stop().
| config.insert_json5("connect/endpoints", json.dumps(self.config.connect)) | ||
| if self.config.listen: | ||
| config.insert_json5("listen/endpoints", json.dumps(self.config.listen)) | ||
| _sessions[key] = zenoh.open(config) |
There was a problem hiding this comment.
Zenoh sessions in _sessions dict are never closed when services stop. This matches the DDS pattern but could leak resources if many different session configs are created over time.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This merge request implements Zenoh transport layer, providing a new high-performance pub/sub transport option.
Quick Start
Note
We currently use the python bindings for Eclipse Zenoh as our Zenoh implementation. Its IdlStruct feature lets you define DDS topic types in pure Python, eliminating the need for separate IDL files, with automatic serialization support.
Unit Tests/Benchmarks
Related: #927