Skip to content

refactor(manipulation): compose canonical pick and place - #3715

Open
ruthwikdasyam wants to merge 6 commits into
ruthwik/feat/osr-owlvit-edgetamfrom
ruthwik/refactor/canonical-pick-place
Open

refactor(manipulation): compose canonical pick and place#3715
ruthwikdasyam wants to merge 6 commits into
ruthwik/feat/osr-owlvit-edgetamfrom
ruthwik/refactor/canonical-pick-place

Conversation

@ruthwikdasyam

Copy link
Copy Markdown
Contributor

Summary

  • Replace the legacy pick-and-place subclass with a capability-composed module that coordinates OSR, grasp generation, and manipulation execution.
  • Add stable-ID scan, explicit grasp selection, verified pick, and explicit place workflow skills.
  • Compose the canonical workflow into real and simulated xArm perception blueprints.

Stack

Validation

  • TMPDIR=/dev/shm uv run --no-sync pytest dimos/manipulation/test_pick_and_place_unit.py dimos/perception/experimental/test_object_scene_registration_temporal.py dimos/robot/test_all_blueprints_generation.py -q
  • Ruff format/check and mypy on all changed files.
  • Imported real and simulated xArm perception blueprints.

Deliberately Deferred

  • Public method-description polish, candidate filtering/reranking, visualization, obstacle suppression, and table/box features.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
4881 1 4880 87
View the top 1 failed test(s) by shortest run time
dimos.utils.test_shm::test_create_or_attach_splits_owner_and_reader
Stack Traces | 0.624s run time
name = 'dimos_test_b5e6c5cb29a6'
slow_ftruncate = <function slow_ftruncate.<locals>.delayed at 0x7f637ad28b80>

    def test_create_or_attach_splits_owner_and_reader(name, slow_ftruncate):
        """Concurrent symmetric callers: exactly one owns, the other waits and attaches."""
        out: list[tuple[bool, int]] = []
        lock = threading.Lock()
        barrier = threading.Barrier(2)
    
        def racer() -> None:
            barrier.wait()
            shm, owner = create_or_attach_shm(name, SIZE, timeout=5.0)
            with lock:
                out.append((owner, shm.size))
            shm.close()
    
        threads = [threading.Thread(target=racer, daemon=True) for _ in range(2)]
        for t in threads:
            t.start()
        for t in threads:
            t.join(timeout=8)
    
        assert len(out) == 2, f"a racer did not finish: {out}"
        assert sorted(owner for owner, _ in out) == [False, True]
>       assert {size for _, size in out} == {SIZE}
E       assert {0, 65536} == {65536}
E         
E         Extra items in the left set:
E         #x1B[0m#x1B[94m0#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
E         
E         Full diff:
E         #x1B[0m#x1B[90m #x1B[39;49;00m {#x1B[90m#x1B[39;49;00m
E         #x1B[92m+     0,#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
E         #x1B[90m #x1B[39;49;00m     65536,#x1B[90m#x1B[39;49;00m
E         #x1B[90m #x1B[39;49;00m }#x1B[90m#x1B[39;49;00m

barrier    = <threading.Barrier at 0x7f637b5ea660: waiters=0/2>
lock       = <unlocked _thread.lock object at 0x7f637ac66440>
name       = 'dimos_test_b5e6c5cb29a6'
out        = [(True, 65536), (False, 0)]
racer      = <function test_create_or_attach_splits_owner_and_reader.<locals>.racer at 0x7f637ad28d60>
slow_ftruncate = <function slow_ftruncate.<locals>.delayed at 0x7f637ad28b80>
t          = <Thread(Thread-4813 (racer), stopped daemon 140063967012544)>
threads    = [<Thread(Thread-4812 (racer), stopped daemon 140062171784896)>, <Thread(Thread-4813 (racer), stopped daemon 140063967012544)>]

dimos/utils/test_shm.py:148: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@ruthwikdasyam
ruthwikdasyam force-pushed the ruthwik/refactor/canonical-pick-place branch 3 times, most recently from c313d61 to 6e9d305 Compare August 28, 2026 00:36
@ruthwikdasyam
ruthwikdasyam force-pushed the ruthwik/refactor/canonical-pick-place branch from 8efb261 to 28e0f4a Compare August 28, 2026 02:23
@ruthwikdasyam
ruthwikdasyam marked this pull request as ready for review August 28, 2026 02:27
@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change replaces the inherited pick-and-place implementation with composed scene registration, grasp generation, and manipulation services, and wires the new skills into real and simulated xArm blueprints. The held-object scan-and-place flow, blueprint imports, movement capability declarations, and targeted pick-and-place tests completed successfully. However, a failed plan for the first grasp proposal stops the operation instead of trying later ranked proposals that may be reachable.

Confidence Score: 4/5

Not ready to merge because a reachable object can be left unpicked when the highest-ranked grasp is not plannable.

The failure was reproduced with a realistic-fake workflow harness that exercised an unplannable first grasp and a plan-valid second grasp. The targeted unit suite passed, but it does not cover retrying ranked grasp proposals after a planning failure.

Files Needing Attention: dimos/manipulation/pick_and_place_module.py needs a bounded ranked-grasp retry path, with corresponding coverage in dimos/manipulation/test_pick_and_place_unit.py.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the focused PR 3715 capability workflow harness to validate the posted P1 finding and collected its outputs across the base retry, composed harness, targeted unit tests, and xArm blueprint imports.
  • Validated the contract by running the capability workflow harness and the unit-test suite, observing a planning failure with planned_candidate_x_values [0.1] and no selected rank, running pytest with 17 tests passing, and completing the real/sim xArm blueprint import successfully.
  • Posted a second P1 finding-comment-proof to document the additional review finding.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 pick_object abandons ranked grasp fallback after the first planning failure

    • Bug
      • pick_object selects only candidates.candidates[0] at dimos/manipulation/pick_and_place_module.py:144 and returns immediately from the pregrasp motion failure at lines 155-158. The executed harness provided two ranked proposals: rank 0 was deliberately unplannable while rank 1 was plan-valid. The observed result was PLANNING_FAILED with planned candidate positions [0.1]; candidate rank 1 (x=0.2) was never planned. This regresses the base workflow’s bounded candidate loop and contradicts the new API’s “Generate ranked grasps” contract.
    • Cause
      • The PR replaced the previous bounded loop over candidate grasps with a single hard-coded candidates.candidates[0] selection, while retaining ranked-candidate terminology and GRASP_ATTEMPTS_EXHAUSTED in the manipulation error contract.
    • Fix
      • Iterate over the ranked candidates (with an explicit bounded attempt limit), attempt each candidate’s pregrasp/grasp plan until one completes, return the winning rank and score, and return GRASP_ATTEMPTS_EXHAUSTED only after all eligible candidates fail. Preserve safe gripper/state handling between attempts.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "chore(manipulation): use default grasp R..." | Re-trigger Greptile

Comment on lines +144 to +158
candidate = candidates.candidates[0]
grasp = self._apply_yaw_policy(
PoseStamped(
ts=candidates.header.timestamp,
frame_id=candidates.header.frame_id,
position=candidate.pose.position,
orientation=candidate.pose.orientation,
),
group,
)
pregrasp = self._offset_pose(grasp, self.config.pregrasp_offset)
if failure := self._open_gripper(group, "pre-grasp open"):
return failure
if failure := self._move(pregrasp, group):
return failure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Ranked grasp fallback is never attempted

pick_object selects only candidates.candidates[0] and returns when that candidate's pregrasp plan fails. Later ranked proposals are never considered, even if one is reachable. The executed harness provided two proposals, made rank 0 unplannable, and observed PLANNING_FAILED after planning only rank 0; the plan-valid rank-1 proposal was not attempted. Iterate through a bounded set of ranked candidates and return GRASP_ATTEMPTS_EXHAUSTED only after all eligible proposals have failed.

Artifacts

Focused PR 3715 capability workflow review harness

  • Authored realistic-fake harness that executes ranked pick fallback, scan-while-holding then place, and blueprint/capability composition checks; it is the executable proof source.

Base workflow pick retry implementation

  • Captured base revision pick implementation showing its loop over up to five grasp candidates and continuation after an approach planning failure; the comparison establishes the removed fallback behavior.

PR 3715 composed workflow harness output

  • Captured output from the executed PR harness: rank 0 planning failed without trying rank 1, while the holding scan then place flow and blueprint/capability checks succeeded; the takeaway is one reproducible retry regression.

PR 3715 targeted pick and place unit suite

  • Captured output from the existing targeted pick/place unit suite, which completed successfully with 17 passing tests; the takeaway is that current tests do not cover ranked planning fallback.

Real and simulated xArm blueprint import output

  • Captured output from importing the real and MuJoCo simulated xArm perception blueprints; both imported as Blueprint objects, confirming practical composition importability.

View artifacts

T-Rex Ran code and verified through T-Rex

@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Aug 28, 2026
@mustafab0
mustafab0 force-pushed the ruthwik/refactor/canonical-pick-place branch from 28e0f4a to 7cf1338 Compare August 28, 2026 06:31
@github-actions github-actions Bot removed the ready-to-merge Required CI checks have passed on this PR label Aug 28, 2026
# plunging deep and colliding with the object body.
_TALL_OBJECT_MIN_HEIGHT = 0.06
class PickAndPlaceModuleConfig(ModuleConfig):
planning_frame: str = "world"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
planning_frame: str = "world"
planning_frame: str = "base_link"

class PickAndPlaceModuleConfig(ModuleConfig):
planning_frame: str = "world"
pregrasp_offset: float = Field(default=0.10, gt=0.0)
yaw_policy: Literal["generated", "preserve_current"] = "generated"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a heuristic grasp config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants