Cleanup warnings #1144
Cleanup warnings #1144
Conversation
📝 WalkthroughWalkthroughAdded an optional verbose parameter to ModelBuilder and Style3D finalize methods, gated several runtime warnings behind the verbosity flag, set Warp quiet mode in examples/tests, and updated tests to pass verbose where needed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
5907-5929: Add documentation for the newverboseparameter.The
verboseparameter is added to the method signature but is not documented in the docstring's Args section.Args: device: The simulation device to use (e.g., 'cpu', 'cuda'). If None, uses the current Warp device. requires_grad: If True, enables gradient computation for the model (for differentiable simulation). + verbose: If True, emit warnings for inertia corrections and other diagnostic messages. + Defaults to `wp.config.verbose`. Returns: Model: A fully constructed Model object containing all simulation data on the specified device.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/sim/builder.py(2 hunks)newton/_src/sim/graph_coloring.py(1 hunks)newton/_src/solvers/vbd/solver_vbd.py(1 hunks)newton/examples/__init__.py(1 hunks)newton/tests/test_inertia_validation.py(2 hunks)newton/tests/thirdparty/unittest_parallel.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/examples/__init__.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/examples/__init__.py
📚 Learning: 2025-07-23T14:36:42.182Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 461
File: asv/benchmarks/envs/example_humanoid.py:40-41
Timestamp: 2025-07-23T14:36:42.182Z
Learning: In Warp benchmarks, explicit wp.init() calls are not needed in most circumstances since the first Warp API call that requires initialization will automatically call wp.init(). Explicit wp.init() in setup() methods is helpful when the ASV benchmark is measuring a Warp API call, as wp.init() has non-trivial overhead that should be excluded from the benchmark timing.
Applied to files:
newton/tests/thirdparty/unittest_parallel.py
🧬 Code graph analysis (2)
newton/tests/test_inertia_validation.py (1)
newton/_src/sim/builder.py (1)
finalize(4907-5299)
newton/_src/solvers/vbd/solver_vbd.py (1)
newton/_src/solvers/solver.py (1)
device(169-176)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (8)
newton/tests/thirdparty/unittest_parallel.py (2)
177-178: LGTM!Setting
wp.config.quiet = Truehere appropriately suppresses Warp logs in the main test runner process.
577-578: LGTM!Setting
wp.config.quiet = Truein each test worker process ensures consistent log suppression across parallel test execution.newton/examples/__init__.py (1)
25-26: LGTM!Setting
wp.config.quiet = Trueat the module level appropriately suppresses Warp logs for all examples.newton/_src/sim/graph_coloring.py (1)
249-255: LGTM!The warning is now appropriately gated behind the
verboseflag, allowing users to suppress informational warnings about color balancing early termination when running in quiet mode.newton/_src/solvers/vbd/solver_vbd.py (1)
2333-2334: LGTM!The deprecation warning is now appropriately gated behind the
verboseflag, allowing users to suppress this informational message when running in quiet mode. The functional behavior remains unchanged—tiled solve is still disabled on CPU at line 2336.newton/tests/test_inertia_validation.py (2)
178-178: LGTM!Passing
verbose=Trueis necessary here to ensure the inertia validation warning is emitted during the fast validation path, allowing the test to verify the warning behavior.
250-250: LGTM!Passing
verbose=Trueis necessary here to ensure the inertia validation warning is emitted when testing default validation behavior, allowing the test to verify that critical issues like negative mass are caught and reported.newton/_src/sim/builder.py (1)
5133-5138: LGTM!The verbose gating is correctly applied. Warnings for inertia corrections are now only emitted when
verbose=True, which aligns with the PR objective of cleaning up warnings while still allowing users to opt-in to diagnostic output.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
4907-4921: Finalize’s newverboseparameter: API shape looks fine; consider minor polishExtending
finalizewithverbose: bool = wp.config.verboseis backward compatible and nicely documented. Two optional tweaks you might consider:
- The default is captured at import time; if
wp.config.verboseis toggled later, calls likebuilder.finalize()will not reflect that change. A more dynamic pattern would beverbose: bool | None = Noneandif verbose is None: verbose = wp.config.verboseinside the method, though this is consistent withcollapse_fixed_jointstoday.Style3D’s builder override (builder_style3d.ModelBuilder.finalize(self, device=None, requires_grad=False)) does not acceptverboseand therefore can’t be called asfinalize(verbose=...). To keep the subclass LSP‑compatible withModelBuilder, you may want to addverbose: bool = wp.config.verbosethere as well and pass it through tosuper().finalize(...).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/builder.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (2)
newton/_src/geometry/types.py (2)
finalize(102-109)finalize(251-268)newton/_src/sim/style3d/builder_style3d.py (1)
finalize(675-698)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (1)
newton/_src/sim/builder.py (1)
5134-5139: Verbose‑gated inertia correction warning behaves as intendedConditioning the inertia‑correction warning on
num_corrections > 0 and verbosepreserves diagnostics when needed while respecting quiet mode; the fast path still applies corrections regardless of verbosity.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/examples/__init__.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/examples/__init__.py
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/examples/__init__.py
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (1)
newton/examples/__init__.py (1)
383-384: Unable to complete verification due to repository access failure. Manual verification or code context is needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/examples/__init__.py (1)
373-375: Globalwp.config.quietmutation ininit()is still fragile and surprising
init()now unconditionally does:args._original_warp_quiet = wp.config.quiet wp.config.quiet = Trueand relies on
_quiet_warp_context(args)inrun()to restore the original value. This has a few issues:
- Any caller that uses
newton.examples.init()but never callsrun(example, args)will leavewp.config.quietpermanently set toTruefor the rest of the process.- If
run()is called with a differentargsobject than the one returned frominit(), the original state is also not restored.- It still conflicts with the earlier review feedback that
newton.examplesshouldn’t change global Warp config in general, since this is a library-style helper that can be imported and used outside the CLI path.To avoid surprising global side effects and fragile coupling between
init()andrun(), consider one of the following:
- Remove the
wp.config.quietmutation frominit()and have_quiet_warp_contextfully manage both setting and restoringwp.config.quietaround whatever region you want muted (perhaps only underargs.test, or only from themain()CLI entrypoint), or- Gate this behind an explicit, opt-in flag (e.g.,
--quiet/args.quiet) so that library-style usage ofnewton.examples.init()doesn’t silently alter global Warp logging.This would better align with the prior concern about not letting
newton.exampleschange Warp global state.
🧹 Nitpick comments (2)
newton/examples/__init__.py (2)
18-34: Tight coupling between_quiet_warp_contextandinit()’s side effect
_quiet_warp_context(args)only restoreswp.config.quietbased onargs._original_warp_quietand never sets it, so it implicitly relies oninit()having already set bothargs._original_warp_quietandwp.config.quiet. That hidden coupling makes the lifecycle harder to reason about (e.g., ifrun()is ever called withargsthat didn’t come frominit(), this context manager becomes a no-op).Consider making
_quiet_warp_contextself-contained (capture currentwp.config.quiet, set it to the desired value on__enter__, always restore on__exit__) and/or documenting that it assumesinit()has run.
184-224:run()logic looks sound; Ruff TRY003 on NaN checks is stylistic onlyThe refactored
run()flow (GUI registration, main loop withScopedTimers, viewer close, and optionalargs.testvalidations) is straightforward and functionally coherent. The generic NaN checks viafind_nan_membersforstate_0,state_1,model,control, andcontactsgated behindargs.testare a nice addition for test-mode robustness.Ruff’s TRY003 warnings on the
ValueErrormessages at lines 208/212/216/220/224 are style-only; given these errors are specific diagnostics for tests, keeping the explicit messages is reasonable. If you prefer to satisfy TRY003, you could introduce a small helper likeraise_nan_error(label, nan_members)instead of inlining similarValueErrorconstructions five times.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/examples/__init__.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/examples/__init__.py
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/examples/__init__.py
🧬 Code graph analysis (1)
newton/examples/__init__.py (1)
newton/tests/unittest_utils.py (1)
find_nan_members(256-265)
🪛 Ruff (0.14.7)
newton/examples/__init__.py
208-208: Avoid specifying long messages outside the exception class
(TRY003)
212-212: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Avoid specifying long messages outside the exception class
(TRY003)
220-220: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
5325-5357: Finalize verbosity flag is wired correctly; consider extending it to detailed validation if desiredThe new
verbose: bool | None = Noneparameter, defaulting fromwp.config.verbosewhenNone, is backward‑compatible and clearly documented. Using it to gate the inertia‑correction warning (if num_corrections > 0 and verbose:) is a straightforward way to quiet the common warning noise while still allowing users/tests to opt in viafinalize(verbose=True).If you eventually want
finalize(verbose=False)to silence all inertia‑related diagnostics, you might consider threading this flag into the detailed path as well (e.g., ifverify_and_correct_inertiacan accept or respect a verbosity setting). Not required for correctness; the current behavior is consistent with the attribute docs and the PR goals.Also applies to: 5677-5682
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/sim/builder.py(4 hunks)newton/_src/sim/style3d/builder_style3d.py(2 hunks)newton/examples/__init__.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/sim/style3d/builder_style3d.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/examples/__init__.py
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/sim/style3d/builder_style3d.py (1)
finalize(672-702)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (2)
newton/examples/__init__.py (2)
27-34: LGTM! Exception-safe context manager.The context manager correctly implements try-finally to ensure
wp.config.quietis always restored, even if the wrapped code raises an exception. This addresses the previous concern about exception safety.
523-524: LGTM! Appropriately scoped quiet mode.The context manager usage is correctly limited to command-line execution only, which addresses the previous concern about not affecting scenarios where
newton.examplesis imported as a module.
| # Run the target example module | ||
| runpy.run_module(target_module, run_name="__main__") | ||
| with _quiet_warp_context(): | ||
| runpy.run_module(target_module, run_name="__main__") |
There was a problem hiding this comment.
Is this really necessary? The compile spam mentioned in #1016 shouldn't be happening anymore since we now cache kernels even if they are marked module="unique".
There was a problem hiding this comment.
I do see these prints on the main branch when I run examples
Module repeat_array_kernel_edde1597 93dc5c6 load on device 'cuda:0' took 0.17 ms
|
what's the status of this PR, are we planning on merging this or is this abandoned? Any actions/decisions we need to discuss in the group? |
I'm not sure if we really want to suppress the compilation print-outs like this, they are just masking potential kernel caching bugs. |
|
I think the initial reason to want this was because the spam prevents us from quickly root-causing CI issues. Here's my opinion:
Does that make sense? |
I'm also in favor of a |
|
Let's table this feature for now until there is a need for this. |
3 participants
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Improvements
Tests / Examples
✏️ Tip: You can customize this high-level summary in your review settings.