feat: rust transform support (get only)#2816
Open
aclauer wants to merge 15 commits into
Open
Conversation
…sform-get # Conflicts: # dimos/robot/unitree/go2/blueprints/navigation/unitree_go2_nav_3d.py
❌ 2 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
aclauer
requested review from
leshy,
mustafab0,
paul-nechifor and
spomichter
as code owners
Contributor
Greptile SummaryThis PR adds a consumer-side transform client (
Confidence Score: 4/5Safe to merge for the current two-hop TF tree; the BFS bug only triggers in graphs with multiple paths where one alternate route has a tolerance-failing edge. The BFS visited-set ordering is wrong: frames get marked visited before the edge tolerance check, so an alternative path to the same node can be silently discarded. For the robot's typical two-hop tree this never triggers, but the bug will surface as a hard-to-diagnose None return once the TF graph grows or a clock-skewed edge is introduced. native/rust/dimos-module/src/tf.rs — the BFS function around line 263. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Py as Python (NativeModule)
participant Stdin as stdin JSON
participant Builder as Rust Builder
participant TfRoute as TfRoute (Route)
participant Buffer as MultiTBuffer (Arc<RwLock>)
participant Tf as Tf handle
Py->>Stdin: "inject {"tf": tf_channel()}"
Builder->>Builder: "tf() called (#[tf] field)"
Builder->>TfRoute: tf_subscription(topic, buffer_size)
TfRoute-->>Buffer: "creates shared Arc<RwLock<MultiTBuffer>>"
Builder->>Tf: clones Tf handle (Arc ref)
Builder->>Builder: register TfRoute under topic
Note over TfRoute,Buffer: On every /tf message
TfRoute->>Buffer: "write lock -> receive(parent, child, ts, iso)"
Note over Tf,Buffer: On get_latest() / get() call
Tf->>Buffer: "read lock -> BFS(parent, child, time, tol)"
Buffer-->>Tf: composed Transform (nalgebra Isometry3)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Py as Python (NativeModule)
participant Stdin as stdin JSON
participant Builder as Rust Builder
participant TfRoute as TfRoute (Route)
participant Buffer as MultiTBuffer (Arc<RwLock>)
participant Tf as Tf handle
Py->>Stdin: "inject {"tf": tf_channel()}"
Builder->>Builder: "tf() called (#[tf] field)"
Builder->>TfRoute: tf_subscription(topic, buffer_size)
TfRoute-->>Buffer: "creates shared Arc<RwLock<MultiTBuffer>>"
Builder->>Tf: clones Tf handle (Arc ref)
Builder->>Builder: register TfRoute under topic
Note over TfRoute,Buffer: On every /tf message
TfRoute->>Buffer: "write lock -> receive(parent, child, ts, iso)"
Note over Tf,Buffer: On get_latest() / get() call
Tf->>Buffer: "read lock -> BFS(parent, child, time, tol)"
Buffer-->>Tf: composed Transform (nalgebra Isometry3)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+263
to
+275
| for next in self.connections(&frame) { | ||
| if visited.insert(next.clone()) { | ||
| if let Some(edge) = self.edge(&frame, &next, time, tolerance) { | ||
| let mut extended = path.clone(); | ||
| extended.push(edge); | ||
| queue.push_back((next, extended)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
visited before the edge lookup, so if the edge fails the tolerance check the frame is permanently excluded from BFS exploration. Any alternative path through a different intermediate that would reach next within tolerance is then silently skipped. Move visited.insert inside the successful-edge branch so other routes to the same frame remain explorable.
Suggested change
| for next in self.connections(&frame) { | |
| if visited.insert(next.clone()) { | |
| if let Some(edge) = self.edge(&frame, &next, time, tolerance) { | |
| let mut extended = path.clone(); | |
| extended.push(edge); | |
| queue.push_back((next, extended)); | |
| } | |
| } | |
| } | |
| } | |
| None | |
| } | |
| } | |
| for next in self.connections(&frame) { | |
| if !visited.contains(&next) { | |
| if let Some(edge) = self.edge(&frame, &next, time, tolerance) { | |
| visited.insert(next.clone()); | |
| let mut extended = path.clone(); | |
| extended.push(edge); | |
| queue.push_back((next, extended)); | |
| } | |
| } | |
| } | |
| } | |
| None | |
| } | |
| } |
| @@ -255,7 +256,8 @@ def start(self) -> None: | |||
| assert self._process.stdin is not None | |||
| if self.config.stdin_config: | |||
| config_dict = self.config.to_config_dict() | |||
Contributor
There was a problem hiding this comment.
{**topics, "tf": tf_channel()} unconditionally clobbers whatever value topics["tf"] might have had. The comment explains this is intentional (every module reserves "tf"), but if a native module ever declares a non-tf port named "tf" the override will be silent and hard to debug. Worth a brief inline note that the key is reserved and must not be used for user ports.
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!
aclauer
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
We need transforms with Rust
Closes DIM-XXX
Solution
Right now just supporting get, since we don't have any publishers yet. i'll add that asap.
Add
#[tf]attribute to subscribe a module to the transform topicget_latest()-> get the latest transform between two framesget()-> get transform at a specified timeTransforms are collected in a hash map and inter-frame transforms are built on demand
How to Test
uv run python examples/native-modules/rust_tf.pyContributor License Agreement