Remove return value from solver step()#1968
Conversation
The step() method wrote results into state_out in-place; returning it was redundant and no callers used the value. Remove the return type annotation from the base class and the return statements from the xpbd, featherstone, and mujoco implementations.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded explicit return type annotations ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/solver.py (1)
272-285:⚠️ Potential issue | 🟠 MajorDon't remove the
step()return contract without a deprecation window.This changes the public
SolverBase.step()API fromState | Noneto an implicit no-return contract. Even if in-repo callers ignore the result, external users and typed integrations can still be doingstate = solver.step(...), so this is a breaking change. Please keep the old contract for this release and deprecate the return value first, then remove it in a later release with explicit migration guidance. As per coding guidelines,newton/**/*.py:Breaking changes require a deprecation first. Do not remove or rename public API symbols without deprecating them in a prior release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/solver.py` around lines 272 - 285, Restore the original return contract by changing the step signature back to return State | None (i.e. def step(self, ... ) -> State | None) and update the docstring to mark the returned State as deprecated (include a short deprecation note and migration guidance), keep the current NotImplementedError in the base implementation so subclasses still must implement it, and add a TODO/deprecation comment that the return value will be removed in a future release so callers can be migrated before the breaking change; reference the step method on SolverBase (step) so reviewers can find and verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@newton/_src/solvers/solver.py`:
- Around line 272-285: Restore the original return contract by changing the step
signature back to return State | None (i.e. def step(self, ... ) -> State |
None) and update the docstring to mark the returned State as deprecated (include
a short deprecation note and migration guidance), keep the current
NotImplementedError in the base implementation so subclasses still must
implement it, and add a TODO/deprecation comment that the return value will be
removed in a future release so callers can be migrated before the breaking
change; reference the step method on SolverBase (step) so reviewers can find and
verify the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 120d2496-e5dd-41d2-8c18-cd591bd43a05
📒 Files selected for processing (4)
newton/_src/solvers/featherstone/solver_featherstone.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/solver.pynewton/_src/solvers/xpbd/solver_xpbd.py
💤 Files with no reviewable changes (3)
- newton/_src/solvers/xpbd/solver_xpbd.py
- newton/_src/solvers/featherstone/solver_featherstone.py
- newton/_src/solvers/mujoco/solver_mujoco.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/featherstone/solver_featherstone.py (1)
324-331:⚠️ Potential issue | 🟠 Major
step()return removal needs a deprecation path.Changing this public method from returning
state_outto returningNonewill break downstream code that doesstate = solver.step(...). If the API is being tightened, please deprecate the old behavior for a release first and add migration guidance before removing the return value outright.As per coding guidelines,
newton/**/*.py: Breaking changes require a deprecation first. Do not remove or rename public API symbols without deprecating them in a prior release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/featherstone/solver_featherstone.py` around lines 324 - 331, The public method step(self, state_in: State, state_out: State, control: Control, contacts: Contacts, dt: float) was changed to return None which breaks callers expecting a returned state; restore backward compatibility by retaining the original return of state_out while emitting a DeprecationWarning (via warnings.warn(..., DeprecationWarning)) and updating the step docstring to state the return will be removed in a future release, then schedule removal in a subsequent release; locate the method named step in class SolverFeatherstone (in solver_featherstone.py), add the warnings.warn call inside the method before returning state_out, and document the deprecation and migration guidance in the docstring.
🧹 Nitpick comments (1)
newton/_src/solvers/featherstone/solver_featherstone.py (1)
324-331: Match the override's parameter types to the base contract.The override at
solver_featherstone.py:324narrowscontrolandcontactsto non-optional types, but the implementation handlesNoneat lines 344 and 447. The base classSolverBase.step(line 272 insolver.py) declares these asControl | NoneandContacts | None, per PEP 604 conventions. Update the signature to accept the same parameter types:def step( self, state_in: State, state_out: State, - control: Control, - contacts: Contacts, + control: Control | None, + contacts: Contacts | None, dt: float, ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/featherstone/solver_featherstone.py` around lines 324 - 331, The step override in solver_featherstone.py narrows parameter types for control and contacts but the implementation accepts None; change the signature of SolverFeatherstone.step to match the base contract by typing control and contacts as optional (Control | None and Contacts | None) so it matches SolverBase.step; update only the method signature in def step(...) to use these union types (PEP 604) to resolve the mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/solvers/style3d/solver_style3d.py`:
- Line 167: The step method declares an unused parameter control which triggers
ARG002; explicitly consume it or silence the linter without changing the API:
inside the step(self, state_in: State, state_out: State, control: Control,
contacts: Contacts, dt: float) implementation, either add a statement to consume
the parameter (e.g., del control) or add a narrow noqa comment on the parameter
usage to suppress ARG002, leaving the function signature (step) unchanged.
In `@newton/_src/solvers/xpbd/solver_xpbd.py`:
- Around line 249-250: Preserve backward compatibility by temporarily keeping
SolverXPBD.step(...) returning the passed-in state_out while marking that return
as deprecated: update the method to still return state_out, add a runtime
DeprecationWarning (via warnings.warn) and update the step docstring to state
the return will be removed and callers should use the state_out argument; also
add an entry under CHANGELOG.md [Unreleased] explaining the deprecation with
migration guidance ("use the passed-in state_out instead of the return value");
apply the same deprecation pattern to the other affected occurrence(s)
referenced (the similar step implementation around the second mention) so all
public API callers are warned consistently.
---
Outside diff comments:
In `@newton/_src/solvers/featherstone/solver_featherstone.py`:
- Around line 324-331: The public method step(self, state_in: State, state_out:
State, control: Control, contacts: Contacts, dt: float) was changed to return
None which breaks callers expecting a returned state; restore backward
compatibility by retaining the original return of state_out while emitting a
DeprecationWarning (via warnings.warn(..., DeprecationWarning)) and updating the
step docstring to state the return will be removed in a future release, then
schedule removal in a subsequent release; locate the method named step in class
SolverFeatherstone (in solver_featherstone.py), add the warnings.warn call
inside the method before returning state_out, and document the deprecation and
migration guidance in the docstring.
---
Nitpick comments:
In `@newton/_src/solvers/featherstone/solver_featherstone.py`:
- Around line 324-331: The step override in solver_featherstone.py narrows
parameter types for control and contacts but the implementation accepts None;
change the signature of SolverFeatherstone.step to match the base contract by
typing control and contacts as optional (Control | None and Contacts | None) so
it matches SolverBase.step; update only the method signature in def step(...) to
use these union types (PEP 604) to resolve the mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 558ab94e-e34d-46be-8902-b7b79c9ac322
📒 Files selected for processing (8)
newton/_src/solvers/featherstone/solver_featherstone.pynewton/_src/solvers/implicit_mpm/solver_implicit_mpm.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/semi_implicit/solver_semi_implicit.pynewton/_src/solvers/solver.pynewton/_src/solvers/style3d/solver_style3d.pynewton/_src/solvers/vbd/solver_vbd.pynewton/_src/solvers/xpbd/solver_xpbd.py
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/solvers/solver.py
- newton/_src/solvers/mujoco/solver_mujoco.py
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looks good — all solver step() and notify_model_changed() signatures consistently annotated with -> None, and the three stale return state_out statements removed.
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looks good — all solver step() and notify_model_changed() signatures consistently annotated with -> None, and the three stale return state_out statements removed.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
The step() method on all solvers writes results into state_out in-place. Returning state_out was redundant — no callers use the return value.
The -> State | None return type annotation on the base class was likely introduced incorrectly in #1749. This PR removes it, along with the return state_out statements in the xpbd, featherstone, and mujoco implementations to make all solvers consistent. The vbd, style3d, semi_implicit, and implicit_mpm solvers already had no return statement.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Internal cleanup with no behavior change. Full test suite passes:
Summary by CodeRabbit