-
Notifications
You must be signed in to change notification settings - Fork 5
RPC Rework: no var renames #1145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
…lueprint to be consistent with docs
Greptile OverviewGreptile SummaryImplements a new RPC system with module references and Spec-based dependency injection. This PR introduces:
Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant autoconnect
participant ModuleBlueprintSet
participant ModuleCoordinator
participant Calculator as Calculator Module
participant Client as Client Module
participant RPCClient as ModuleProxy
User->>autoconnect: autoconnect(Calculator.blueprint(), Client.blueprint())
autoconnect->>ModuleBlueprintSet: Create blueprints with module_refs
Note over ModuleBlueprintSet: Extract StreamRefs (In/Out)<br/>Extract ModuleRefs (Spec/Module annotations)
User->>ModuleBlueprintSet: .build()
ModuleBlueprintSet->>ModuleCoordinator: start()
ModuleBlueprintSet->>ModuleCoordinator: _deploy_all_modules()
ModuleCoordinator->>Calculator: Deploy module
ModuleCoordinator->>RPCClient: Wrap in RPCClient (ModuleProxy)
ModuleCoordinator->>Client: Deploy module
ModuleCoordinator->>RPCClient: Wrap in RPCClient (ModuleProxy)
ModuleBlueprintSet->>ModuleBlueprintSet: _connect_transports()
Note over ModuleBlueprintSet: Connect In/Out streams via LCMTransport
ModuleBlueprintSet->>ModuleBlueprintSet: _connect_rpc_methods()
Note over ModuleBlueprintSet: Register RPC methods for discovery
ModuleBlueprintSet->>ModuleBlueprintSet: _connect_module_refs()
Note over ModuleBlueprintSet: Match Client.calc (ComputeSpec)<br/>to Calculator module
ModuleBlueprintSet->>Client: set_module_ref("calc", calculator_proxy)
Note over Client: Client.calc now references Calculator
ModuleBlueprintSet->>ModuleCoordinator: start_all_modules()
Client->>Calculator: self.calc.compute1(2, 3) via RPC
Calculator-->>Client: Return 5
Client->>Calculator: self.calc.compute2(1.5, 2.5) via RPC
Calculator-->>Client: Return 4.0
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 1 comment
| key = (remapped_name, conn.type) | ||
| if conn.direction == "out": | ||
| producers[key].append(bp.module) | ||
| producers[key].append(bp.module) # type: ignore[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For connections (streams) the replacement will only ever be a str, even though the remapping type annotation has str | type[Spec] because replacement of module ref's will be with Specs
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| from __future__ import annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen some places where we use annotations, some places where we don't. If you guys have a preference let me know and I'll use that.
|
|
||
| @rpc | ||
| def set_module_ref(self, name: str, module_ref: RPCClient) -> None: | ||
| setattr(self, name, module_ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important bit: lets us connect the module reference after the module is running/deployed
| ModuleBlueprintSet, | ||
| ModuleConnection, | ||
| _make_module_blueprint, | ||
| StreamRef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It made sense to have ModuleRef for this PR, and then to be consistent in naming I changed ModuleConnection (which is now easy to confuse with ModuleRef) to StreamRef to be less confusing
Reverts the _BlueprintAtom/Blueprint rename