Add move_through_joint_positions and get_3d_models to Arm (BREAKING) - #1250
Open
Nick Hehr (HipsterBrown) wants to merge 7 commits into
Open
Add move_through_joint_positions and get_3d_models to Arm (BREAKING)#1250Nick Hehr (HipsterBrown) wants to merge 7 commits into
Nick Hehr (HipsterBrown) wants to merge 7 commits into
Conversation
Nick Hehr (HipsterBrown)
requested review from
martha-johnston and
Naveed Jooma (njooma)
July 29, 2026 15:40
Contributor
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
Member
|
👋 Thanks for requesting a review from the team! We aim to review PRs within one business day. If this is urgent |
…on all Arm subclasses Adds both methods to MockArm, ExampleArm, MyArm, MyCoolArm, and MyModularArm (notebook) while the Arm ABC remains untouched, so the test suite stays green. This is a prerequisite for a later change that declares both methods as abstract on the Arm ABC.
FakeArmClient in tests/test_robot.py is a sixth Arm subclass that the original Task 1 file list missed. Without these methods it fails to instantiate once move_through_joint_positions and get_3d_models become abstract on the Arm ABC.
my_cool_arm.py and complex_module's my_arm.py accepted MoveOptions in move_through_joint_positions but silently ignored it, with the only comment pointing at cancellation instead. A module author copying these examples would ship a driver that accepts speed/accel limits and drops them on the floor. Also fixes FakeArmClient in test_robot.py to pass options by keyword, matching extra/timeout, so a future keyword-only change on ArmClient doesn't break it at a distance.
…, and client Declares the method as abstract on the Arm ABC and implements it in ArmRPCService and ArmClient in the same change, since ArmClient is a seventh Arm subclass (beyond the six covered by the prior mock/example commit) and would otherwise fail to instantiate once the ABC method is abstract. The service guards on MoveOptions field presence so "no limit requested" (None) is never conflated with a zeroed MoveOptions, which a driver could misread as "do not move".
…ence Address code review on move_through_joint_positions: - ArmRPCService now calls the resource's move_through_joint_positions with options=options (keyword) instead of positionally, so drivers that reasonably declare it keyword-only don't blow up at runtime. - Extend the ABC's Note to cover the same 0.0-vs-unset ambiguity one level deeper: every scalar on MoveOptions has its own explicit presence, so reading e.g. max_vel_degs_per_sec without a HasField check silently reads "unset" as "stop". Also document the proto's precedence rule (per-joint limits override the scalar ceiling) and that an empty positions list must be handled by implementations. - Update the two example drivers' comments to point at HasField instead of naming the scalar directly, so they don't teach the wrong pattern. - Add the MoveOptions import hint to the Arm class docstring, and move the ABC's move_through_joint_positions to sit after move_to_joint_positions, matching the order already used in client.py, service.py, and MockArm. - Cover the keyword form (options=..., timeout=...) in TestClient, moving the new timeout assertion onto the test that runs first so the timeout-is-None invariant TestClient.test_stop relies on still holds.
Address code review on get_3d_models:
- ArmClient.get_3d_models now copies the protobuf map response into a
plain dict before returning it. Protobuf map containers create and
insert a default-constructed value on __getitem__ for an absent key
instead of raising KeyError, silently violating the Mapping[str, Mesh]
contract and corrupting subsequent len()/iteration on the caller's
reference. get_kinematics has the same wart on its third element but
is left alone as pre-existing and out of scope.
- Cross-reference the get_3d_models and get_kinematics docstrings so
readers of either know the two are keyed differently (by model name
vs. by URDF filepath) and aren't the same data.
- Reword "an arm with no models returns an empty mapping" as an
implementer obligation ("must return an empty mapping, not None"),
matching the Note: style used in move_through_joint_positions.
- Unify the Arm class docstring's import guidance to the package
re-export form for all four types (Pose, JointPositions, MoveOptions,
Mesh), since all four are in __all__.
- Soften docs/examples/my_cool_arm.py's comment from "keyed by link
name" to "keyed by name" to match the ABC and avoid over-claiming
beyond what the proto documents.
- Move ArmRPCService.Get3DModels to sit between GetKinematics and
GetGeometries, matching ArmClient's method ordering.
- Tighten TestClient.test_get_3d_models to assert equality against the
live Mapping (not a dict() copy, which would pass either way) and
add a KeyError assertion plus a timeout assertion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015RbNNgUjePEdPuUGAdU3qb
Nick Hehr (HipsterBrown)
force-pushed
the
arm-trajectory-3d-models
branch
from
July 29, 2026 19:34
f4a2912 to
adcc687
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
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.
Armnow declares two new@abc.abstractmethods. Every existing third-party arm module will fail at startup until it implements both:This is not a deprecation or a runtime warning — the module does not start. The release carrying this needs an explicit upgrade note, since the
TypeErrorgives module authors no hint about what to implement.Abstract (rather than concrete-raising-
MethodNotImplementedError) was a deliberate choice for consistency with the other sevenArmmethods.Summary
Brings the Python SDK's
Armto RPC parity with the Go SDK (rdk) by implementing the two arm RPCs the SDK did not expose:move_through_joint_positions(positions, options=None, ...)— executes a multi-waypoint joint trajectory under optional kinematic ceilings (MoveOptions). Previously the only way to do this from Python was amove_to_joint_positionsloop with no velocity or acceleration control.get_3d_models() -> Mapping[str, Mesh]— retrieves the arm's 3D meshes.Both are implemented across all three layers (ABC,
ArmRPCService,ArmClient), plus all seven in-repoArmsubclasses.MoveOptionsandMeshare now re-exported fromviam.components.arm.No proto regeneration — the generated stubs and types already existed.
src/viam/gen/**andsrc/viam/proto/**are untouched.Coverage before/after
Notes for reviewers
MoveOptionspresence is two levels deep. Theoptionsfield has explicit presence, so the service branches onrequest.HasField("options")— without it, a caller who omitted options would hand the driver a zeroedMoveOptions, andmax_vel_degs_per_sec == 0.0could be misread as "do not move" rather than "no limit requested". The scalars insideMoveOptionshave presence too, so the ABC docstring directs implementers to checkoptions.HasField(...)per field. It also documents the proto's precedence rule (max_vel_degs_per_secis ignored whenmax_vel_degs_per_sec_jointsis set), which nothing else in the SDK surfaced.get_3d_modelsreturnsdict(response.models), not the raw container. Protobuf map fields mimic C++ semantics —__getitem__on a missing key default-constructs and inserts rather than raisingKeyError. Returning the live container would meanmodels["gripper_link"]on an absent link yields an emptyMeshand silently grows the caller's mapping, breaking the declaredMapping[str, Mesh]contract. A test pins theKeyError.No client-side joint-limit validation, unlike Go.
rdk's client callsKinematics()and runscheckDesiredJointPositionsbefore everyMoveThroughJointPositions. Python has no equivalent ofreferenceframe.Model—get_kinematics()returns raw URDF/SVA bytes with no parser — so there is nothing to validate against. The asymmetry is documented in the ABC docstring'sNote:: the Go client refuses out-of-limit waypoints, the Python client forwards them to the driver. Adding a parser is its own project.MoveThroughJointPositionsStreameddeliberately left unimplemented — it exists in the generated stubs but is implemented in neitherrdknor here.Known follow-up (not in this PR)
get_kinematicsreturnsresponse.meshes_by_urdf_filepath— the same live protobuf container, with the same missing-key auto-insert bug described above. Pre-existing, so it was left out of scope here, but it warrants a separate fix.Test Plan
make test— 876 passed, 0 failed (includestests/test_examples.py, which launches theserverandcomplex_moduleexample modules)make typecheck— 0 errors (1 pre-existing warning insrc/viam/module/types.py)make lint/make format— clean, 301 filesgit diff --stat -- src/viam/gen src/viam/proto— emptyArm.__abstractmethods__contains both new methods;ArmClient.__abstractmethods__is emptyChannelFor: waypoints andMoveOptionsround-trip intact; omitted options arrive asNonerather than zeroed;get_3d_modelsreturns a realdictthat raisesKeyErrorwithout insertingArmsubclasses instantiate — verified in subprocesses for the example/docs arms, which no lint, typecheck, or test coversdocs/examples/example.ipynb— valid JSON, cell sources still list-of-lines, embeddedMyModularArmparses10 new tests across
TestArm,TestService, andTestClient.🤖 Generated with Claude Code
https://claude.ai/code/session_015RbNNgUjePEdPuUGAdU3qb