Move Kamino tests to Newton test folder - #3807
Conversation
chschuma-disney
added
the
testing
chschuma-disney
added
the
kamino
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughKamino records worlds without assigned base bodies and warns when supplied base inputs have no effect. Shared Kamino test utilities move to ChangesKamino solver integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
newton/_src/solvers/kamino/_src/core/builder.py (2)
912-923: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winVerify
follower_idx = joint.bid_Fdoes not reference an unboundjoint.
follower_idx = joint.bid_Fexecutes insideif world.has_base_joint:. In this branch,jointis never assigned. The only assignment ofjointinfinalize()happens in theelifbranch's loopfor jt_idx, joint in enumerate(self._joints[w]):, several lines below.Python treats
jointas a local variable offinalize()because it is assigned somewhere in the function body. If the first world processed by the outer loop hasworld.has_base_joint == True,jointhas not been assigned yet on that call, and this line raisesUnboundLocalError.This line looks like it should read
base_joint.bid_F(the local variable defined at line 913), notjoint.bid_F.🐛 Proposed fix
- follower_idx = joint.bid_F # Note: index among world bodies + follower_idx = base_joint.bid_F # Note: index among world bodies🤖 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 `@newton/_src/solvers/kamino/_src/core/builder.py` around lines 912 - 923, In the world base-joint handling within finalize(), update follower_idx to read bid_F from the already validated base_joint variable rather than the unbound joint variable. Preserve the existing base-body compatibility check and set-base-body behavior.
924-939: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the fallback condition: it marks worlds with a valid base body as having none.
Trace the case where the unary-joint loop finds a
FREEunary joint.world.set_base_body(joint.bid_F)runs at line 932, soworld.has_base_bodybecomesTruebefore the loop ends.At line 936, the condition is
not world.has_base_body and not has_unary_joint and world.num_bodies > 0. Becauseworld.has_base_bodyis nowTrue,not world.has_base_bodyisFalse. The whole condition isFalse. Execution falls to theelsebranch and setshas_world_without_base_body = True, even though the world has a valid base body and base joint.Compare this with the equivalent logic in
newton/_src/solvers/kamino/_src/core/conversions.py(lines 1508-1512). That version skips the fallback entirely whenbase_body_idx_np[wid] != -1(checked earlier at line 1495), so it never marks a world with a valid base body as missing one.Because of this bug, every
ModelBuilderKamino-built floating-base model whose base is auto-detected through a unary free joint reportshas_world_without_base_body = True. Callers ofSolverKamino.reset()andForwardKinematicsSolver.solve_fk()then get spurious "no base body assigned" warnings whenever they pass a base pose or velocity, even for correctly configured floating-base models.None of the added tests catch this:
test_kamino_core_model.py::test_05_model_conversions_base_assignment_non_floating_rootexercisesModelKamino.from_newton()(the conversions.py path), notModelBuilderKamino.finalize()directly.🐛 Proposed fix
- if not world.has_base_body and not has_unary_joint and world.num_bodies > 0: - world.set_base_body(0) - else: - has_world_without_base_body = True + if world.has_base_body: + pass # Base body/joint already assigned via a free unary joint + elif not has_unary_joint and world.num_bodies > 0: + world.set_base_body(0) + else: + has_world_without_base_body = TrueConsider also adding a direct unit test that calls
ModelBuilderKamino.finalize()on a world with a unary free joint and assertshas_world_without_base_bodyisFalse, since the current tests only cover this behavior throughModelKamino.from_newton().🤖 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 `@newton/_src/solvers/kamino/_src/core/builder.py` around lines 924 - 939, Fix the fallback and missing-base flag logic in ModelBuilderKamino.finalize around the unary-joint detection: after a FREE unary joint assigns a base body, do not enter the missing-base path. Only assign body 0 when no unary joint was found and no base body exists, and set has_world_without_base_body only when the world still lacks a base body after all valid assignments. Add a direct test covering finalize with a unary free joint and asserting the flag remains false.
🧹 Nitpick comments (3)
newton/tests/kamino/__init__.py (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported
setup_testsAPI.Add a Google-style docstring with
Args:entries forverbose,device, andclear_cache. Add an explicit-> Nonereturn annotation.Suggested update
-def setup_tests(verbose: bool = False, device: wp.DeviceLike | str | None = None, clear_cache: bool = False): +def setup_tests( + verbose: bool = False, + device: wp.DeviceLike | str | None = None, + clear_cache: bool = False, +) -> None: + """Configure the shared Kamino test runtime. + + Args: + verbose: Enable verbose test output. + device: Warp device to use for tests. + clear_cache: Clear Warp kernel and LTO caches before testing. + """🤖 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 `@newton/tests/kamino/__init__.py` at line 46, Update the exported setup_tests function signature to explicitly return None and add a Google-style docstring documenting verbose, device, and clear_cache under Args:, including each parameter’s purpose and expected value.Source: Coding guidelines
newton/tests/kamino/utils/diff_check.py (1)
21-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Google-style docstrings.
Convert both docstrings from NumPy-style
ParametersandReturnssections to Google-style sections. Keep type information in the annotations. Format argument entries asname: description.As per coding guidelines, “Use Google-style docstrings, keep types in annotations rather than docstrings, and format
Args:entries asname: description.”Also applies to: 71-105
🤖 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 `@newton/tests/kamino/utils/diff_check.py` around lines 21 - 42, Convert the docstrings for central_finite_differences and the additionally referenced function around the later diff to Google style: replace NumPy-style Parameters and Returns sections with Args and Returns, format arguments as name: description, and remove redundant type declarations from docstrings while preserving the existing annotations and behavior.Source: Coding guidelines
newton/tests/kamino/test_kamino_gimbal.py (1)
101-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style
Args:sections to the helper docstrings.
_build_fixture,_make_solver, and_runaccept configuration inputs that control model construction and solver behavior. Document each parameter, includingfixture_kwargs, in anArgs:section.🤖 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 `@newton/tests/kamino/test_kamino_gimbal.py` around lines 101 - 186, Add Google-style Args sections to the docstrings of _build_fixture, _make_solver, and _run, documenting every parameter each helper accepts, including _run’s fixture_kwargs and keyword-only configuration inputs. Keep the existing summaries and accurately describe how each argument controls fixture construction, solver selection, or rollout behavior.Source: Coding guidelines
🤖 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 `@newton/examples/kamino/example_kamino_robot_anymal_d.py`:
- Around line 88-90: Add an imperative changelog entry under the [Unreleased]
Fixed section describing the example_kamino_robot_anymal_d behavior change:
default use_kamino_contacts=True and passing None to SolverKamino.step() when
Kamino handles collision detection. Reuse an existing entry only if it clearly
covers this behavior.
In `@newton/tests/kamino/test_kamino_kinematics_resets.py`:
- Around line 583-611: Remove the leftover print(logs.output) statement from
test_06_reset_warns_without_base_body_when_base_provided; retain the existing
assertLogs capture and assertion that verifies the warning content.
---
Outside diff comments:
In `@newton/_src/solvers/kamino/_src/core/builder.py`:
- Around line 912-923: In the world base-joint handling within finalize(),
update follower_idx to read bid_F from the already validated base_joint variable
rather than the unbound joint variable. Preserve the existing base-body
compatibility check and set-base-body behavior.
- Around line 924-939: Fix the fallback and missing-base flag logic in
ModelBuilderKamino.finalize around the unary-joint detection: after a FREE unary
joint assigns a base body, do not enter the missing-base path. Only assign body
0 when no unary joint was found and no base body exists, and set
has_world_without_base_body only when the world still lacks a base body after
all valid assignments. Add a direct test covering finalize with a unary free
joint and asserting the flag remains false.
---
Nitpick comments:
In `@newton/tests/kamino/__init__.py`:
- Line 46: Update the exported setup_tests function signature to explicitly
return None and add a Google-style docstring documenting verbose, device, and
clear_cache under Args:, including each parameter’s purpose and expected value.
In `@newton/tests/kamino/test_kamino_gimbal.py`:
- Around line 101-186: Add Google-style Args sections to the docstrings of
_build_fixture, _make_solver, and _run, documenting every parameter each helper
accepts, including _run’s fixture_kwargs and keyword-only configuration inputs.
Keep the existing summaries and accurately describe how each argument controls
fixture construction, solver selection, or rollout behavior.
In `@newton/tests/kamino/utils/diff_check.py`:
- Around line 21-42: Convert the docstrings for central_finite_differences and
the additionally referenced function around the later diff to Google style:
replace NumPy-style Parameters and Returns sections with Args and Returns,
format arguments as name: description, and remove redundant type declarations
from docstrings while preserving the existing annotations and behavior.
🪄 Autofix
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e579e4c-5bd2-4adf-a8c3-719f23e0c03d
📒 Files selected for processing (68)
newton/_src/solvers/kamino/_src/core/builder.pynewton/_src/solvers/kamino/_src/core/conversions.pynewton/_src/solvers/kamino/_src/core/model.pynewton/_src/solvers/kamino/_src/solver_kamino_impl.pynewton/_src/solvers/kamino/_src/solvers/fk/solver.pynewton/_src/solvers/kamino/tests/test_utils_io_usd.pynewton/_src/solvers/kamino/tests/test_utils_random.pynewton/examples/kamino/example_kamino_robot_anymal_d.pynewton/tests/kamino/__init__.pynewton/tests/kamino/test_kamino_core_geometry.pynewton/tests/kamino/test_kamino_core_joints.pynewton/tests/kamino/test_kamino_core_materials.pynewton/tests/kamino/test_kamino_core_model.pynewton/tests/kamino/test_kamino_core_shapes.pynewton/tests/kamino/test_kamino_core_types.pynewton/tests/kamino/test_kamino_core_world.pynewton/tests/kamino/test_kamino_dynamics_delassus.pynewton/tests/kamino/test_kamino_dynamics_dual.pynewton/tests/kamino/test_kamino_dynamics_wrenches.pynewton/tests/kamino/test_kamino_geometry_aggregation.pynewton/tests/kamino/test_kamino_geometry_contacts.pynewton/tests/kamino/test_kamino_geometry_detector.pynewton/tests/kamino/test_kamino_geometry_keying.pynewton/tests/kamino/test_kamino_geometry_margin_gap.pynewton/tests/kamino/test_kamino_geometry_mesh_heightfield.pynewton/tests/kamino/test_kamino_geometry_primitive.pynewton/tests/kamino/test_kamino_geometry_unified.pynewton/tests/kamino/test_kamino_gimbal.pynewton/tests/kamino/test_kamino_kinematics_constraints.pynewton/tests/kamino/test_kamino_kinematics_jacobians.pynewton/tests/kamino/test_kamino_kinematics_joints.pynewton/tests/kamino/test_kamino_kinematics_limits.pynewton/tests/kamino/test_kamino_kinematics_resets.pynewton/tests/kamino/test_kamino_linalg_conjugate_fused.pynewton/tests/kamino/test_kamino_linalg_core.pynewton/tests/kamino/test_kamino_linalg_factorize_llt_blocked.pynewton/tests/kamino/test_kamino_linalg_factorize_llt_sequential.pynewton/tests/kamino/test_kamino_linalg_solve_cg.pynewton/tests/kamino/test_kamino_linalg_solver_llt_blocked.pynewton/tests/kamino/test_kamino_linalg_solver_llt_blocked_rcm.pynewton/tests/kamino/test_kamino_linalg_solver_llt_sequential.pynewton/tests/kamino/test_kamino_linalg_sparse.pynewton/tests/kamino/test_kamino_solver_kamino.pynewton/tests/kamino/test_kamino_solver_kamino_joint_frames.pynewton/tests/kamino/test_kamino_solver_kamino_notify.pynewton/tests/kamino/test_kamino_solvers_dvi.pynewton/tests/kamino/test_kamino_solvers_forward_kinematics.pynewton/tests/kamino/test_kamino_solvers_metrics.pynewton/tests/kamino/test_kamino_solvers_padmm.pynewton/tests/kamino/test_kamino_utils_world_equivalence.pynewton/tests/kamino/utils/__init__.pynewton/tests/kamino/utils/checks.pynewton/tests/kamino/utils/diff_check.pynewton/tests/kamino/utils/extract.pynewton/tests/kamino/utils/joints.pynewton/tests/kamino/utils/make.pynewton/tests/kamino/utils/print.pynewton/tests/kamino/utils/rand.pynewton/tests/kamino/utils/sampling.pynewton/tests/test_body_velocity.pynewton/tests/test_control_force.pynewton/tests/test_joint_controllers.pynewton/tests/test_physics_verification.pynewton/tests/test_rigid_contact.pynewton/tests/test_rigid_friction_ramp.pynewton/tests/test_solver_kamino_dvi.pynewton/tests/test_solver_kamino_dvi_cuda.pynewton/tests/test_up_axis.py
💤 Files with no reviewable changes (2)
- newton/tests/test_solver_kamino_dvi_cuda.py
- newton/tests/test_solver_kamino_dvi.py
22f9e5d to
cf87212
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
| # Host-side Metadata | ||
| ### | ||
|
|
||
| has_world_without_base_body: bool = False |
There was a problem hiding this comment.
I'm wondering if we should use an underscore prefix here (_has_world_without_base_body) to indicate that it's an internal flag? Or do we expect users to read this somewhere?
There was a problem hiding this comment.
I'd argue it's more consistent without the underscore. The attribute is not internal to the class, in fact it's only accessed from outside the class. And the whole Kamino model class is never exposed to the user, so there is no expectation that this will be read (by a regular user).
| ) and self._model.info.has_world_without_base_body: | ||
| msg.warning( | ||
| "Some worlds have no base body assigned (empty world or non-free articulation root). " | ||
| "Base resets and FK base handling will have no effect for those worlds." |
There was a problem hiding this comment.
Suggested small reformulations:
"Some worlds have no base body assigned (possibly due to articulation roots not being free joints, e.g. fixed-base systems)."
"Base pose/velocity resets will have no effect for those worlds."
I'd also suggest moving this a bit higher after other validations (length checks etc) since it only reads the config.
There was a problem hiding this comment.
Looking back above I see that the point was having the same warning for FK and the general reset, so up to you. But it could still make sense to have the messages similar, but more explicit about whether it's the resets (that could also be a pure base reset without FK), or the FK solver (which a user could also call in isolation).
There was a problem hiding this comment.
I'll update these, let me know if you think these are good now.
| return builder.finalize(device="cpu"), d6 | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
There was a problem hiding this comment.
I didn't fully get what this commit is doing, it seems that the code is moved within the same file rather than removed. Could you clarify what the removed unnecessary computations are then?
There was a problem hiding this comment.
The differences are somewhat hidden by moving around the code. Basically, test_limits used to also run a simulation for MJWarp, but never actually compared anything between Kamino and MJWarp. I removed the MJWarp simulation and moved the test to TestGimbal, so it also runs on CPU and the split in tests is clearer. I moved the existing helper code to clean up the file structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@newton/tests/kamino/test_kamino_gimbal.py`:
- Around line 236-243: Add concise triple-double-quoted imperative docstrings to
the lifecycle methods TestGimbal.setUp, TestGimbal.tearDown, and
TestGimbalMJWarp.setUp, preserving their existing setup and teardown behavior.
🪄 Autofix
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8abad1a7-3f3c-4038-8bbe-5d9b9de0ec51
📒 Files selected for processing (5)
newton/examples/kamino/example_kamino_robot_anymal_d.pynewton/tests/kamino/test_kamino_geometry_unified.pynewton/tests/kamino/test_kamino_gimbal.pynewton/tests/kamino/test_kamino_solvers_forward_kinematics.pynewton/tests/test_physics_verification.py
🚧 Files skipped from review as they are similar to previous changes (4)
- newton/examples/kamino/example_kamino_robot_anymal_d.py
- newton/tests/kamino/test_kamino_geometry_unified.py
- newton/tests/test_physics_verification.py
- newton/tests/kamino/test_kamino_solvers_forward_kinematics.py
2 participants
Description
This moves most of the previously internal Kamino tests to the Newton test folder, so that they will now run during the CI pipeline.
Test files that only test functionality that is not user-facing have been left as internal Kamino tests.
The changes in this PR are mostly just a pure migration, without changes to the test setups. There seem to be no issues with integration with the Newton test scaffolding, so a basic migration seemed appropriate to unblock developers that might have changes to the test lined up or planned. Future revisions to the tests will look into adapting more of Newton's test structure (e.g., testing across devices).
Some of the changes besides pure migration:
test_solver_kamino_dvi.pyandtest_solver_kamino_dvi_cuda.pyhave been removed, since these tests are now directly called.I tried keeping these changes clean in the commits, in case someone wants to have a look at changes besides the migration. The overall goal of most of these has been to avoid emitting output during the tests, so that we could transition to using
NewtonTestCasein the future.Performance
Performance-wise, the Kamino tests currently take ~5 minutes on my machine.
Claude also gathered the statistics of the latest 100 merges on
mainas a reference:Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
This should run all new tests added to the test folder.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests