Skip to content

fix(manipulation): detect Viser execution completion and consume plans - #3982

Merged
TomCC7 merged 3 commits into
mainfrom
cc/fix/viser-cancel
Sep 8, 2026
Merged

fix(manipulation): detect Viser execution completion and consume plans#3982
TomCC7 merged 3 commits into
mainfrom
cc/fix/viser-cancel

Conversation

@TomCC7

@TomCC7 TomCC7 commented Sep 8, 2026

Copy link
Copy Markdown
Member

Contribution path

Bug fix; no tracking issue.

Problem

  • Viser stays in EXECUTING after a trajectory finishes because nonblocking execution never refreshes its cached status. Planning again requires manually cancelling.
  • The panel keeps the plan after dispatch.

Solution

  • Refresh active execution during status reads using the existing zero-timeout wait. This makes one synchronous coordinator RPC and allows planning again when execution completes.
  • Clear the plan before dispatch, including on rejection or error. During motion, the plan is NONE and the module is EXECUTING.
  • Remove the redundant None check for the execution manager, which is initialized at startup.

How to Test

uv run dimos run dual-xarm6-planner-coordinator

drag the arms and execute the plan, see UI updates correctly when motion finishes.

AI assistance

Codex (GPT-6) wrote the fix and regression tests, ran validation, and drafted this description.

Checklist

  • I have read and approved the CLA.

Comment thread dimos/manipulation/manipulation_module.py Outdated
Comment thread dimos/manipulation/visualization/viser/state.py
Comment thread dimos/manipulation/manipulation_module.py
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.44961% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/manipulation/visualization/viser/gui.py 66.66% 1 Missing and 1 partial ⚠️
@@            Coverage Diff             @@
##             main    #3982      +/-   ##
==========================================
+ Coverage   77.92%   77.98%   +0.05%     
==========================================
  Files        1399     1399              
  Lines      132324   132442     +118     
  Branches    11492    11493       +1     
==========================================
+ Hits       103111   103280     +169     
+ Misses      25946    25891      -55     
- Partials     3267     3271       +4     
Components Coverage Δ
Tests 94.73% <100.00%> (+0.01%) ⬆️
Flag Coverage Δ
OS-ubuntu-24.04-arm 74.24% <30.89%> (-0.02%) ⬇️
OS-ubuntu-latest 74.82% <98.44%> (+0.06%) ⬆️
Py-3.10 74.82% <98.44%> (+0.06%) ⬆️
Py-3.11 74.82% <98.44%> (+0.05%) ⬆️
Py-3.12 74.82% <98.44%> (+0.05%) ⬆️
Py-3.13 74.82% <98.44%> (+0.06%) ⬆️
Py-3.14 74.82% <98.44%> (+0.06%) ⬆️
Py-3.14t 74.82% <98.44%> (+0.05%) ⬆️
SelfHosted-Large 30.69% <16.27%> (-0.02%) ⬇️
SelfHosted-Linux 35.74% <16.27%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
dimos/manipulation/manipulation_module.py 69.87% <100.00%> (+1.77%) ⬆️
dimos/manipulation/test_plan_execution.py 100.00% <100.00%> (ø)
dimos/manipulation/visualization/viser/state.py 78.60% <ø> (-0.09%) ⬇️
dimos/manipulation/visualization/viser/test_gui.py 98.75% <100.00%> (+0.44%) ⬆️
dimos/manipulation/visualization/viser/gui.py 33.22% <66.66%> (+1.75%) ⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@TomCC7
TomCC7 marked this pull request as ready for review September 8, 2026 20:19
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 8, 2026
@github-actions github-actions Bot removed the ready-to-merge Required CI checks have passed on this PR label Sep 8, 2026
@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

Not safe to merge until pre-start status reads handle the absent execution manager; callers can otherwise receive an exception during module startup.

Findings

  1. P1 Guard pre-start status reads

Summary

  • T-Rex validation blocked

  • The focused construction-before-start status check could not complete because the available virtual environment initially had an invalid interpreter and then lacked required dependencies; the execution limit was reached after the environment was repaired.
  • Status reads before startup

  • The manipulation module now dereferences its execution manager while serving state reads, even though that manager is only created during startup. A status read in the construction-to-startup window can therefore fail instead of returning the idle state.

Comment thread dimos/manipulation/manipulation_module.py Outdated
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 8, 2026
@TomCC7
TomCC7 added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 4345030 Sep 8, 2026
32 of 33 checks passed
@TomCC7
TomCC7 deleted the cc/fix/viser-cancel branch September 8, 2026 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants