Review draft for upstream #2449

[adenzler-nvidia]

Scratch pad for iterating on review of upstream newton#2449 — 'Move modular design of newton-actuators to Newton repo' by @jvonmuralt.

Do not merge. Comments here will be extracted and posted upstream atomically on /review-on-fork ship.

• Fenced text inside blocks is what will ship upstream.
• Free-form prose is working conversation and stays on the fork only.

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

AGENTS.md: "Sphinx cross-refs… Prefer public API paths; never use newton._src." These new docstrings reference :class:~newton._src.actuators.controllers.base.Controller`` etc. The public paths are newton.actuators.Controller / `newton.actuators.Clamping` / `newton.actuators.Delay`. Can we route through those instead? (Same check worth running over all the new files — I spot-checked the actuator module itself and it already uses `newton.actuators.*`, so it's just this pair of selection docstrings I noticed.)

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

resolve_arguments() returns only network_path, but the __init__ signature has five more user-tunable params (input_order, input_idx, pos_scale, vel_scale, torque_scale). Because the builder does

resolved_ctrl = controller_class.resolve_arguments(controller_kwargs)
...
controller = entry.controller_class(**ctrl_arrays, **entry.controller_shared_kwargs)

any of those kwargs passed through ModelBuilder.add_actuator(ControllerNetMLP, ...) (or via USD) will be silently dropped and the controller will use the defaults — including pos_scale=vel_scale=torque_scale=1.0, which is often not what a trained network needs.

Two options I can see:

1. Include them in resolve_arguments() and mark input_idx/input_order as SHARED_PARAMS (they don't vary per-DOF anyway).
2. Document explicitly that programmatic construction is required and raise if these kwargs are passed through add_actuator.

Either way, silent dropping of scale factors feels like a footgun. Same comment applies in spirit to ControllerNetLSTM.resolve_arguments, though that one has fewer tunables.

Related: the comment in usd_parser.py:67-68 ("input_order / input_idx are intentionally left out of the schema; they should be set programmatically") acknowledges the issue for USD but the programmatic add_actuator path has the same hole.

[adenzler-nvidia]

please double check with the intended usage, maybe the function just needs a rename?

[adenzler-nvidia]

Double-checked. Controller.resolve_arguments (base.py) docstring says: "Resolve user-provided arguments with defaults. Returns: Complete arguments with defaults filled in." The builder relies on that contract (builder.py:1721-1724): it splits the full resolved dict into SHARED_PARAMS (scalar shared across DOFs) vs per-DOF array params, and calls controller_class(**ctrl_arrays, **controller_shared_kwargs). ControllerPD.resolve_arguments returns all three __init__ kwargs (kp, kd, constant_force) — consistent with the contract.

ControllerNetMLP.resolve_arguments returns only {"network_path": ...} while __init__ takes 7 kwargs. So it's genuinely incomplete per the documented contract, not a naming issue — the name already fits.

Sticking with the original ask. The ship block still applies.

[adenzler-nvidia]

Confirmation: the verification above stands. Re-posting identical content as v2 so the ship-gate sees a fresh Claude-authored ship block after your reply.

📝 Review comment (will ship upstream) — v2

resolve_arguments() returns only network_path, but the __init__ signature has five more user-tunable params (input_order, input_idx, pos_scale, vel_scale, torque_scale). Because the builder does

resolved_ctrl = controller_class.resolve_arguments(controller_kwargs)
...
controller = entry.controller_class(**ctrl_arrays, **entry.controller_shared_kwargs)

any of those kwargs passed through ModelBuilder.add_actuator(ControllerNetMLP, ...) (or via USD) will be silently dropped and the controller will use the defaults — including pos_scale=vel_scale=torque_scale=1.0, which is often not what a trained network needs.

Two options I can see:

1. Include them in resolve_arguments() and mark input_idx/input_order as SHARED_PARAMS (they don't vary per-DOF anyway).
2. Document explicitly that programmatic construction is required and raise if these kwargs are passed through add_actuator.

Either way, silent dropping of scale factors feels like a footgun. Same comment applies in spirit to ControllerNetLSTM.resolve_arguments, though that one has fewer tunables.

Related: the comment in usd_parser.py:67-68 ("input_order / input_idx are intentionally left out of the schema; they should be set programmatically") acknowledges the issue for USD but the programmatic add_actuator path has the same hole.

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

Actuator.SHARED_PARAMS is an instance property, but the component-level Controller.SHARED_PARAMS / Clamping.SHARED_PARAMS are ClassVar[set[str]]. The builder relies on reading it as a classvar (getattr(controller_class, "SHARED_PARAMS", set())). Reading Actuator.SHARED_PARAMS on the class returns the property object, not a set — so this method would silently break the same grouping logic if someone ever reached for it as a group key.

grep shows nothing else in the repo actually calls Actuator.SHARED_PARAMS. Is this still needed, or can it be dropped? (Noting @​eric-heiden raised a related question in the existing thread — this is asking the same thing from the "consistency with the rest of the system" angle.)

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

Two small things on masked reset:

a. The loop launches _delay_masked_reset_kernel three times (one per buffer), and each launch zeros num_pushes[i] where mask is true. That's redundant — the kernel takes num_pushes as output. Either pass num_pushes on only one launch, or have one kernel that zeros all three buffers and num_pushes once.

b. Related, Delay.__init__ accepts max_delay: int with no validation. ModelBuilder.add_actuator's delay path enforces delay >= 1, but Delay(max_delay=0) constructed directly would later divide by zero in update_state's (write_idx + 1) % self.buf_depth. The builder-side validation in resolve_arguments should probably move (or be mirrored) into Delay.__init__ so direct construction (the exact use case the concept doc advertises) doesn't have that trap.

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

The example signature here is stale vs. Controller.compute in base.py:

def compute(self, positions, velocities, target_pos, target_vel,
            feedforward, input_indices, target_indices, forces,
            state, dt, device=None):

The real signature uses pos_indices, vel_indices, target_pos_indices, target_vel_indices (four index arrays, not two). Anyone following this example to write a custom controller will get a TypeError on kernel launch. Could we update the example to match the 4-index layout?

[adenzler-nvidia]

please double-check for accuracy here.

[adenzler-nvidia]

Verified. Controller.compute in newton/_src/actuators/controllers/base.py:76-90 actually signs as:

def compute(self, positions, velocities, target_pos, target_vel,
            feedforward, pos_indices, vel_indices,
            target_pos_indices, target_vel_indices,
            forces, state, dt, device=None):

The rst example at line 219 still uses the old 2-index shape (input_indices, target_indices). Stale, as originally flagged. Ship block stands.

[adenzler-nvidia]

Confirmation: the verification above stands. Re-posting identical content as v2.

📝 Review comment (will ship upstream) — v2

The example signature here is stale vs. Controller.compute in base.py:

def compute(self, positions, velocities, target_pos, target_vel,
            feedforward, input_indices, target_indices, forces,
            state, dt, device=None):

The real signature uses pos_indices, vel_indices, target_pos_indices, target_vel_indices (four index arrays, not two). Anyone following this example to write a custom controller will get a TypeError on kernel launch. Could we update the example to match the 4-index layout?

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

values = [args[key] for args in args_list]
if all(isinstance(v, int) for v in values):
    result[key] = wp.array(values, dtype=wp.int32, device=device)
else:
    result[key] = wp.array(values, dtype=wp.float32, device=device, requires_grad=requires_grad)

This is a nice simplification vs. the previous scalar-only stacking, but the else branch will do surprising things if values is a list of wp.array or numpy arrays: wp.array with a list of 0-d / ragged items tends to silently succeed with unintended shapes. Since add_actuator's **controller_kwargs accepts anything, a user passing e.g. kp=wp.array([1.0, 2.0]) (maybe thinking "multi-DOF at once") currently doesn't get a useful error.

Worth adding a isinstance(v, (int, float)) check and raising a clear TypeError("add_actuator expects scalar per-DOF params; got {type}")?

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

The test file is thorough on the "happy path" (PD/PID/DC-motor/position-based/MLP/LSTM compute + full pipeline integration + selection API), which is great. A few gaps that match promises the code or docs make:

1. State.reset(mask=...) — Delay.State.reset, ControllerPID.State.reset, ControllerNetMLP.State.reset, ControllerNetLSTM.State.reset, and Actuator.State.reset are all part of the public API but not exercised by a single test. The masked path is the most interesting (e.g. resetting one env in a vectorized batch) and is the exact kind of logic that silently does the wrong thing.
2. CUDA graph capture — Actuator.is_graphable() is documented but there's no wp.ScopedCapture/wp.capture_launch test confirming PD+clamping actually graph-captures end-to-end. Given that this is explicitly called out as a design goal in the concept doc, a smoke test would be valuable.
3. requires_grad=True — the Actuator.__init__(requires_grad=...) path allocates gradient-enabled scratch buffers, but no test validates that a differentiable chain actually produces non-None grads after backward().

At minimum, a reset() test for Delay.State + ControllerPID.State with a mask feels low-cost and catches the kind of regression that would otherwise silently corrupt a training run.

[adenzler-nvidia]

please reword to have 3 as a follow up

[adenzler-nvidia]

Good call — framing requires_grad as a follow-up since differentiable-actuator testing is a whole design thread of its own. Revised:

📝 Review comment (will ship upstream) — v2

The test file is thorough on the "happy path" (PD/PID/DC-motor/position-based/MLP/LSTM compute + full pipeline integration + selection API), which is great. Two gaps that match promises the code or docs make:

1. State.reset(mask=...) — Delay.State.reset, ControllerPID.State.reset, ControllerNetMLP.State.reset, ControllerNetLSTM.State.reset, and Actuator.State.reset are all part of the public API but not exercised by a single test. The masked path is the most interesting (e.g. resetting one env in a vectorized batch) and is the exact kind of logic that silently does the wrong thing.
2. CUDA graph capture — Actuator.is_graphable() is documented but there's no wp.ScopedCapture/wp.capture_launch test confirming PD+clamping actually graph-captures end-to-end. Given that this is explicitly called out as a design goal in the concept doc, a smoke test would be valuable.

At minimum, a reset() test for Delay.State + ControllerPID.State with a mask feels low-cost and catches the kind of regression that would otherwise silently corrupt a training run.

Follow-up for a later PR (not blocking this one): a requires_grad=True end-to-end test validating that a differentiable chain produces non-None grads after backward(). The gradient-enabled scratch-buffer path in Actuator.__init__(requires_grad=...) has no coverage today, but testing it well requires thinking through what a differentiable-actuator API contract should look like — bigger than this PR.

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

The dataclass keeps target_paths: list[str] "for future multi-target support" (comment in # Currently only a single target is supported.) and then parse_actuator_prim raises if len != 1. Meanwhile import_usd.py:2769 also re-validates len == 1. Three questions:

• Should this just be target_path: str until multi-target is actually a thing? The list shape leaks into callers (see import_usd.py) and is easy to misuse.
• If we keep it as list for forward-compat, is it worth making SCHEMA_REGISTRY mutable/extensible via a public hook? Right now adding a custom controller schema requires monkey-patching a module-level dict; a register_schema() function would be a cleaner extension point.
• SCHEMA_REGISTRY is documented as "Temporary registry until the actual USD schema is merged." Is there a tracking issue for that followup we could link in the comment? (Several TODOs point at it but none link to an issue.)

[adenzler-nvidia]

please drop the tracking issue comment.

[adenzler-nvidia]

Dropped the tracking-issue bullet. Revised:

📝 Review comment (will ship upstream) — v2

The dataclass keeps target_paths: list[str] "for future multi-target support" (comment in # Currently only a single target is supported.) and then parse_actuator_prim raises if len != 1. Meanwhile import_usd.py:2769 also re-validates len == 1. Two questions:

• Should this just be target_path: str until multi-target is actually a thing? The list shape leaks into callers (see import_usd.py) and is easy to misuse.
• If we keep it as list for forward-compat, is it worth making SCHEMA_REGISTRY mutable/extensible via a public hook? Right now adding a custom controller schema requires monkey-patching a module-level dict; a register_schema() function would be a cleaner extension point.

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

Per AGENTS.md, user-facing changes (especially Changed/Removed) require a CHANGELOG entry with migration guidance. This PR (a) deletes the newton-actuators dependency, (b) replaces the old ModelBuilder.add_actuator(actuator_class=..., input_indices=..., output_indices=...) signature with a very different add_actuator(controller_class, index, clamping=[...], delay=..., pos_index=..., **ctrl_kwargs) API, and (c) changes ArticulationView.get_actuator_parameter/set_actuator_parameter to require a component argument.

All three are breaking for any existing user. Could we add entries under Changed/Removed with explicit before/after snippets? e.g.

- Remove `ActuatorPD`/`ActuatorPID` etc. in favor of composed
  `ControllerPD` + `ClampingMaxForce` etc. Migrate:
    builder.add_actuator(ActuatorPD, input_indices=[i], kp=..., kd=..., max_force=...)
  →
    builder.add_actuator(ControllerPD, index=i, kp=..., kd=...,
                         clamping=[(ClampingMaxForce, {"max_force": ...})])

[adenzler-nvidia]

📝 Review comment (will ship upstream) — v1

AGENTS.md asks us to run docs/generate_api.py after adding new public-API symbols, and add the new module to the API Reference toctree. I don't see api/newton_actuators in docs/index.rst's API Reference toctree (further down in this file) or a corresponding docs/api/newton_actuators.rst. This entry adds the concept page but the generated API page is missing — Sphinx cross-refs throughout this PR (:mod:newton.actuators, `:class:`ControllerPD, :meth:Actuator.state``, etc.) won't resolve and the new concept page at docs/concepts/actuators.rst will have broken links. Can we add the generated API page and toctree entry before this merges?

[adenzler-nvidia]

please reword, make it less verbose. something like don't forget to add to the toctree, and direct the author to the right place

[adenzler-nvidia]

Shorter version:

📝 Review comment (will ship upstream) — v2

Don't forget to add api/newton_actuators to the API Reference toctree (further down in this file) and run docs/generate_api.py to generate the page. Per AGENTS.md — and without it, the :mod:newton.actuators / `:class:`ControllerPD / :meth:Actuator.state`` cross-refs in the new concept doc won't resolve.

[adenzler-nvidia]

all of these should use test-code, right?

[adenzler-nvidia]

Missed your question on my first pass — my grouping only looked at threads I started. Fixing that in the skill now (see below).

On the substance: yes, confirmed. docs/concepts/articulations.rst uses .. testcode:: 13 times and docs/concepts/collisions.rst uses it 6 times (vs. only 2 .. code-block:: python in collisions, both for intentionally-stub code). The new actuators.rst has 5 .. code-block:: python blocks (lines 82, 104, 134, 146, 206) — none of them in testcode — so the Python examples here are not executed by the doctest runner and can silently rot.

One nuance worth flagging: the custom-controller example at line 206 has a stub body (# Launch a Warp kernel... ...) and won't run cleanly as testcode. Either give it a minimal concrete toy body, or leave that single one as .. code-block:: with a brief comment explaining why. The other four should definitely be testcode.

📝 Review comment (will ship upstream) — v1

Per Newton docs convention (articulations.rst, collisions.rst), runnable Python examples in concept pages use .. testcode:: so they're exercised by the doctest builder and can't silently rot. This file uses .. code-block:: python for every Python example (lines 82, 104, 134, 146, 206) — none are executed.

Could we convert the first four (builder-level usage, manual construction, USD loading, selection API) to .. testcode:: with appropriate named groups so setup state carries across? The custom-controller example at line 206 has a stub body (...) that won't execute cleanly — either give it a minimal concrete toy body so testcode works, or keep that one as .. code-block:: with a note explaining why.