Fix: sam3 rpc - #131
Conversation
wilburx813
left a comment
There was a problem hiding this comment.
Thanks for your contribution! The fix for Bug 2 looks great.
For Bug 1, I think it would be better fixed upstream in https://github.com/RLinf/openpi/tree/rpent, rather than monkey-patch in RPent. Would you be willing to open a pr there? If you don’t have the bandwidth, please let us know and we can take it over.
|
Thanks so much for the review! I'm glad the fix for Bug 2 looks good. I also agree that fixing Bug 1 upstream in RLinf/openpi would be the better approach, since it addresses the dtype mismatch at its source rather than monkey-patching it in RPent. Unfortunately, I'm not very familiar with the OpenPI codebase yet, so I'd appreciate it if you could take it over. Please feel free to adapt or reuse the patch there if helpful. Thank you again for maintaining such an excellent project! |
Thanks for your understading, we’ll take over the bug1 fix in upstream. could you please keep this pr focused on bug2? Thanks again for your contribution! |
2320fa0 to
8d53f88
Compare
|
Thanks for your contribution! |
Summary
This PR fixes the bug:
SAM3 RPC readiness fails because
Sam3Facade._dispatch()bypasses the baseRpcFacadehandling of lifecycle methods such ashealthz.Problem
Sam3Facade._dispatch() only handled segment and raised directly for every other method. After RPC lifecycle handling was lifted into RpcFacade, this bypassed the base implementation. As a result, healthz returned:
Dashboard readiness checks therefore never succeeded, causing the session to wait for 300 seconds and enter a fatal state. Since env_server is task-scoped and starts after shared services become ready, it was never spawned.
Fix
super()._dispatch().healthzandshutdown.segmentbehavior is unchanged.Testing
SAM3 server starts successfully.