Repository navigation
Conversation
han-xudong
left a comment
There was a problem hiding this comment.
Reviewed commit 253ae52fb against main. The eight inline findings cover asset materialization, Task Program configuration and inspection, and motion compatibility. The asset materialization regression is merge-blocking and is confirmed by the failed test job.
han-xudong
left a comment
There was a problem hiding this comment.
Re-reviewed b21c0f31 against main. The original eight findings have corresponding fixes. This review records five follow-up findings from checking the fixes and the updated PR diff.
han-xudong
left a comment
There was a problem hiding this comment.
Re-reviewed 6a07480c. I verified the integration selector, offline Carton preparation, adaptive Place sample floor, and the Pour Water test migration. The four inline findings cover a remaining batch isolation case, two asset-resolution regressions, and the related PickUp sample floor.
| for index, target_pose in enumerate(required): | ||
| target_pose = build_pose_plan_states( | ||
| torch.where( | ||
| down_success[:, None, None], target_pose.xpos, current_xpos |
There was a problem hiding this comment.
[P2] Keep rejected rows out of IK and timing
An FK pose used as a hold target still goes through IK. In a CPU probe with the Franka URDF joint limits and the supported FEP manipulability mode, solving a seed's own TCP pose returned a valid posture with 1.254 rad of joint displacement. At 0.005 rad/s and a 20 ms control period, that rejected row alone sets a lower bound of 23,512 timing intervals, exceeding the 10,000-interval cap that fails the entire batch. Please bypass IK and timing for inactive rows, or supply their joint-space holds directly, for both the approach and retract.
| source = Path(task_config_path).resolve() | ||
| task = _read(source) | ||
| integration = _read(_path(task["task_program"]["integration"], source)) | ||
| integration = _resolve_integration( |
There was a problem hiding this comment.
[P2] Read asset requirements without resolving runtime manifests
This lookup now resolves DemoSkill and nominal-motion manifests before returning the role contract. I used a deployment with valid asset requirements and a Carton nominal source, then simulated an unavailable Real2SimCarton cache. The role lookup failed on the unrelated runtime asset, whereas the previous lookup could return the same requirements directly. Please share integration selection and override handling here while deferring runtime asset resolution to deployment loading.
| ) | ||
| embodiment_path = _path(raw["embodiment"]["component"], source) | ||
| integration = _read(integration_path) | ||
| integration = _resolve_integration( |
There was a problem hiding this comment.
[P2] Preserve registry references in materialized deployments
Resolving the integration here turns a DemoSkill {asset, path} reference into a source-machine cache path. The materializer then serializes that absolute path into the generated integration. I materialized a deployment with a real DemoSkill artifact and moved its cache to a different root. Loading the export raised FileNotFoundError without consulting the destination registry. Restoring the original {asset, path} made the same deployment load. Please keep registry references in the exported integration so the destination can resolve them against its own cache.
| if options.cartesian_lift: | ||
| if motion_policy.strategy != "ik_interp": | ||
| raise ValueError("cartesian_lift requires strategy='ik_interp'.") | ||
| approach_success, approach_arm = plan_named_arm_trajectory( |
There was a problem hiding this comment.
[P3] Raise the adaptive sample floor for Cartesian PickUp
With adaptive ik_interp, sample_count=9 and the default five gripper frames leave two samples for this approach. The approach needs three keyframes for the start, pre-grasp, and grasp. Using the existing action fixture, the same request planned successfully with the ordinary path and raised ValueError with cartesian_lift=True. Please give adaptive PickUp the same waypoint-based sample floor used by Place.
Description
Task Program deployments repeat logical/native identifiers, fixed-target scaffolding and long transform matrices. This PR simplifies authoring while retaining legacy input forms, explicit physical bindings and the existing acceptance contracts. It is isolated from #1175 and based on
main.simulation_uidvalues, use fixedposetargets, accept compact position plus XYZW quaternion or explicit rotation matrix, and use canonicalskill_options. Legacy flat matrices and former aliases remain supported. Rounded calibration matrices retain their coefficients without orthogonalization.inspect_deployment.py --explain --json, without constructing simulation.No new runtime dependencies.
Review fixes
Review-fix commit:
2ef586a5209b176fe02e046e2835a51f173698fa.All eight findings against
253ae52fbare addressed:entity_id; explicit different mappings work; explicit invalid values still fail.nominal_sources.The other four failures from the previous CI job are corrected at their test boundary: the policy inspector consumes a real invalid deployment, target checks use decoded poses, the repaired stacking deployment expects its 220-sample policy, and the shared-embodiment check verifies its declared placement override. Existing rejection and ownership assertions are retained.
Validation
253ae52fb; isolating the user's model-service configuration, generating the standard VERSION resource and exposing the existing DexSim installation to the temporary wheel consumer resolved them without test or production changes.present.prepare()plus explain succeeds, and the mixed example's decoded flat16 transform constructs its production constraint successfully.black ., formatting check andgit diff --checkpass. Public API coverage: 4914/4914 exports documented.Native verification and limits
Before the main synchronization, three independent normal-entry Marvin Repeated Pick/Place processes used seed 7, one environment, disabled sensors, task-owned Default/ConstraintDynamics physics and renderer, and the original three-attempt/4800-step budgets. All succeeded on their first attempt with every one of six execution segments accepted:
Program/config hashes match across these runs. This is bounded fixed-seed verification, not a universal success-rate claim or complete attribution of prior grip-retention failures.
The cloth environment, original trajectory and cloth tests remain byte-identical to
main; trajectory SHA-256 is51f01325407ecc256c374d4d8c82a43222b90b10e69e7984daa620f1a346ab24. Cloth native simulation was not repeated in this review-fix round. Its earlier single-run final XY errors were 9.30 / 16.84 mm and remain rejected by the unchanged 15 mm rule. Other GPU/simulation CI lanes were not rerun locally.Main synchronization
Merged latest
main(b629c7158b8180934c0990b71e12e1031ddac16e) inb21c0f315fa4ad51f34ab5f3e7674bbd7f9c657c. The two PickUp/Place conflicts retain this PR's Cartesian branches and main's newsuccess=motion_result.successpropagation when splitting trajectories. No acceptance criteria were changed.Post-merge verification: 14,860 passed, 55 skipped, all 37 deployment snapshots unchanged, formatting/context checks pass, and 4914/4914 public API exports documented. A fresh seed-7 normal-entry Marvin run also accepted all six segments in 2469 steps, with the original attempt and step budgets.
All eight original review threads now have individual replies describing the fix in
2ef586a52and its validation. Threads are left for reviewer confirmation.Follow-up review fixes
Commit
6a07480c739096140d036ee64c46ba7b999d3b16addresses all five follow-up findings againstb21c0f315:sample_count=9; retain explicit fixed-grid budget validation.Fresh validation: 14,865 fast tests passed, 55 skipped; 682 focused tests passed; all 6 slow Pour Water cases passed using CI's
SDL_VIDEODRIVER=dummy. The six physical runs retain the original success/retention/return gates, position error below 20 mm and tilt above 45 degrees. Measured maximum errors were 4.040 / 17.314 / 9.581 / 16.876 / 17.241 / 9.577 mm. All 37 deployment snapshots are unchanged; API coverage 4914/4914, context 42 passed, black 26.10.1 and diff checks pass. Independent reviews additionally checked an initially ineligible or previously failed 1000 m target against the healthy-row baseline, selector-relative manifests and unselected-object protection.Because
test_pour_water_sim.pyis now directly modified, the existing CI planner selects it inslow-gpufor this PR; no selector rules, thresholds, task YAML or cloth replay were changed.Type of change
Screenshots
Not applicable. Validation records include structured native outcomes and before/after deployment snapshots.
Checklist
black .with black 26.10.1.