Add lab_sim joint and object-pose end-state checks#706
Open
shaur-k wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates CI to construct Jazzy GPU images with CUDA 13.2/cuDNN9 and adds deterministic end-state assertions for two objectives in the objectives integration test, with helper/data and injection into the test runner. ChangesCI CUDA Suffix Update
End-state Assertions for Objectives
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Consider whether the change should land upstream in Overlapping files
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lab_sim/test/objectives_integration_test.py (1)
364-379: 💤 Low valueConsider modernizing the type annotation for consistency.
The return type uses
Optional[dict[str, EndStateSpec]]while the hangar_sim equivalent (Context snippet 1) uses the PEP 604 union syntaxdict[str, EndStateSpec] | None. For consistency across similar test files and alignment with modern Python practices, consider updating to the union syntax.♻️ Proposed change
def _expected_end_state_by_id( objective_id: str, resource: ExecuteObjectiveResource, -) -> Optional[dict[str, EndStateSpec]]: +) -> dict[str, EndStateSpec] | None: """Build the end-state spec for objectives we assert on; None otherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lab_sim/test/objectives_integration_test.py` around lines 364 - 379, Update the return type annotation of _expected_end_state_by_id from Optional[dict[str, EndStateSpec]] to the modern PEP 604 union form dict[str, EndStateSpec] | None; adjust imports accordingly (remove typing.Optional if it becomes unused) and ensure the annotation appears exactly on the _expected_end_state_by_id function signature so it matches the hangar_sim style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 111-120: The inline comment near the gpu_image_suffixes definition
incorrectly states that the old jazzy -cuda12.6 variant is “no longer built”;
update the comment to accurately reflect current Docker Hub state and/or clarify
intent: verify that gpu_image_suffixes = { jazzy: '-cuda13.2-cudnn9' } is the
default used by CI, then edit the comment to state that the CI/matrix now
defaults to -cuda13.2-cudnn9 (and that -cuda12.6 tags still exist on Docker Hub
but are intentionally not used by the matrix), or remove the categorical “no
longer built” phrase and replace it with a clear note that moveit_pro’s payload
overrides this value on repository_dispatch.
---
Nitpick comments:
In `@src/lab_sim/test/objectives_integration_test.py`:
- Around line 364-379: Update the return type annotation of
_expected_end_state_by_id from Optional[dict[str, EndStateSpec]] to the modern
PEP 604 union form dict[str, EndStateSpec] | None; adjust imports accordingly
(remove typing.Optional if it becomes unused) and ensure the annotation appears
exactly on the _expected_end_state_by_id function signature so it matches the
hangar_sim style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3da937f-f8c3-43ab-ac85-3d2faf3fd93a
📒 Files selected for processing (2)
.github/workflows/ci.yamlsrc/lab_sim/test/objectives_integration_test.py
Wire expected_end_state_by_id into the lab_sim integration suite for two deterministic objectives: 'Look at Table' asserts the final joint state against the saved waypoint (resolved at runtime via /get_saved_waypoints; full joint set validated live at worst 2.6e-3 rad), and 'Move Flasks to Burners' asserts flask_1/flask_2 landing positions against MuJoCo TF ground truth (new mj_world -> <body> frames; repeatability measured < 3 mm, burners 0.11 m apart so the 0.05 m default tolerance catches a wrong-burner landing). Position-only for the flasks: final yaw is not deterministic. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Since the CUDA 13.2 migration, moveit_pro publishes jazzy images with the -cuda13.2-cudnn9 suffix only; the conservative -cuda12.6 default made every pull_request needs: run 404 on a tag that is no longer built. The repository_dispatch path is unaffected (payload overrides). Co-Authored-By: Claude Opus 4.8 <[email protected]>
shaur-k
force-pushed
the
claude/lab-sim-end-state-checks
branch
from
2cc8f85 to
b4f44f3
Compare
|
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.
needs: moveit_pro/#19732
Motivation
Extends end-state correctness coverage (#698 added the first joint check) with a lab_sim joint check and the first manipulated-object pose check, so a pick/place that reports SUCCESS while dropping or misplacing the object is caught.
Brief description
/get_saved_waypoints. Unlike hangar_sim, the full stored joint set (arm + rail + gripper) holds at its stored values, so no per-joint filtering (validated live: worst error 2.6e-3 rad across 8 joints).flask_1/flask_2against MuJoCo TF ground truth (mj_world → <body>frames added by moveit_pro #19732). Landing repeatability measured < 3 mm across runs; the burners sit 0.11 m apart, so the default 0.05 m tolerance absorbs physics noise yet fails a flask on the wrong burner. Position-only — the flask is radially symmetric so final yaw is not deterministic.Stacked on moveit_pro #19732 via the
needs:token above: the object check reads TF frames that only exist with that PR's backend image.How it was tested
Live against a lab_sim backend running this branch's moveit_pro counterpart: both objectives SUCCESS; real targets pass (joint worst 2.6e-3 rad; flask position error 0.8–3 mm); perturbed targets (+0.5 rad elbow / +0.10 m x) correctly fail — anti-no-op gate holds for both checks.
Note: temporarily carries the cuda13.2
needs:-suffix fix so this PR can run green before that fix merges. Once it lands onmain, rebase and the commit drops.