Report Kamino contact buffer overflow - #3791
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughKamino centralizes contact-capacity reservation through a new ChangesContact overflow handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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/_src/solvers/kamino/_src/geometry/primitive/pipeline.py`:
- Around line 195-202: Reset the overflow warning flag before every kernel
launch so each execution can emit one diagnostic: clear
self._contact_overflow_warning_emitted before primitive_narrowphase in
newton/_src/solvers/kamino/_src/geometry/primitive/pipeline.py lines 195-202,
and clear contacts_out._data.contact_overflow_warning_emitted after
contacts_out.clear() and before wp.launch() in
newton/_src/solvers/kamino/_src/geometry/contacts.py lines 1401-1401. Add
reuse-based tests in newton/_src/solvers/kamino/tests/test_geometry_primitive.py
lines 1099-1112 and newton/_src/solvers/kamino/tests/test_geometry_contacts.py
lines 1395-1395, asserting the flag is 0 after a non-overflow run and returns to
1 after a subsequent overflow run.
🪄 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: cacea4d6-1748-4ae6-a702-1f26c259ab82
📒 Files selected for processing (5)
newton/_src/solvers/kamino/_src/geometry/contacts.pynewton/_src/solvers/kamino/_src/geometry/primitive/narrowphase.pynewton/_src/solvers/kamino/_src/geometry/primitive/pipeline.pynewton/_src/solvers/kamino/tests/test_geometry_contacts.pynewton/_src/solvers/kamino/tests/test_geometry_primitive.py
|
Open question is how often the warning should emit. @Guirec-Maloisel , @chschuma-disney . We have it currently configured to warn only once. Alternative would be to reset and warn every step. |
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.
Regarding the open question (warning frequency): not really sure, but I think I'm leaning towards having this warning more frequently (once per time step with culled contacts), because by construction it can't really be a false positive. So I'd say it's worse if it gets potentially overlooked (because hidden in the middle of many lines about modules being loaded etc; with wp.printf() we can't send this through the proper warning channel with colored text); than to annoy some users (for which the fix should be relatively straightforward).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/solvers/kamino/_src/geometry/contacts.py (1)
422-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the return packing and the reserved count contract.
reserve_contact_capacityis shared by three modules, and it returns a packedwp.vec3iwhose components are not self-describing. Callers must also know that component 1 is the retained count, which can be smaller thannum_contacts. Both current callers passnum_contacts=1and ignore component 1, so partial reservations stay latent. Add a Google-styleArgs:/Returns:block that names each component and states the caller obligation.♻️ Proposed docstring
- """Reserve contact capacity and return the model/world indices and number reserved.""" + """Reserve contact capacity atomically in the per-world and model contact counters. + + Args: + model_max_contacts: Contact capacity of the model buffer. + world_max_contacts: Contact capacity of world ``wid``. + wid: World index of the contacts to reserve. + num_contacts: Number of contacts requested. + contact_model_num: Model-level active contact counter, shape ``(1,)``. + contact_world_num: Per-world active contact counters, shape ``(num_worlds,)``. + contact_overflow_warning_emitted: One-time overflow warning flag, shape ``(1,)``. + + Returns: + ``(model_index, num_reserved, world_index)``. ``model_index`` is ``-1`` when no + capacity was reserved. ``num_reserved`` can be smaller than ``num_contacts``, + so callers that request more than one contact must write only ``num_reserved`` + contacts starting at ``model_index``/``world_index``. + """🤖 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/geometry/contacts.py` around lines 422 - 432, The reserve_contact_capacity function lacks documentation explaining its packed wp.vec3i return value and the contract that component 1 represents the retained count, which may be smaller than num_contacts. Add a Google-style docstring with Args and Returns sections that names each component of the returned wp.vec3i, explicitly states that the retained count can be smaller than the requested num_contacts parameter, and clarifies the caller obligation to handle and check for partial reservations instead of assuming full allocation.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.
Nitpick comments:
In `@newton/_src/solvers/kamino/_src/geometry/contacts.py`:
- Around line 422-432: The reserve_contact_capacity function lacks documentation
explaining its packed wp.vec3i return value and the contract that component 1
represents the retained count, which may be smaller than num_contacts. Add a
Google-style docstring with Args and Returns sections that names each component
of the returned wp.vec3i, explicitly states that the retained count can be
smaller than the requested num_contacts parameter, and clarifies the caller
obligation to handle and check for partial reservations instead of assuming full
allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e33b4419-8686-4b79-8514-07efd870efdf
📒 Files selected for processing (7)
newton/_src/solvers/kamino/_src/geometry/contacts.pynewton/_src/solvers/kamino/_src/geometry/primitive/narrowphase.pynewton/_src/solvers/kamino/_src/geometry/primitive/pipeline.pynewton/_src/solvers/kamino/_src/geometry/unified.pynewton/_src/solvers/kamino/tests/test_geometry_contacts.pynewton/_src/solvers/kamino/tests/test_geometry_primitive.pynewton/_src/solvers/kamino/tests/test_solvers_dvi.py
🚧 Files skipped from review as they are similar to previous changes (4)
- newton/_src/solvers/kamino/tests/test_geometry_contacts.py
- newton/_src/solvers/kamino/tests/test_geometry_primitive.py
- newton/_src/solvers/kamino/_src/geometry/primitive/pipeline.py
- newton/_src/solvers/kamino/_src/geometry/primitive/narrowphase.py
Sounds good. I updated it to reset the flag every simulation step. If it becomes too annoying we can always make it less verbose again. |
Guirec-Maloisel
left a comment
Guirec-Maloisel
left a comment
There was a problem hiding this comment.
Thanks for the updates, looks good to me now! I think I can live with the warnings being slightly less specific.
Leaving approval to @chschuma-disney in case he has a different opinion on the warning frequency.
2 participants
Description
Add graph-capture-friendly, one-time
wp.printfwarnings when Kamino truncates contacts because either per-world or model contact capacity is exhausted. Apply the diagnostic to primitive narrow-phase contact generation and Newton-to-Kamino contact conversion, with regression coverage for both paths.Implements disneyresearch/newton#115.
A user-facing overflow API is out of scope: Newton does not yet have an established API for this diagnostic. This PR therefore limits behavior to a single warning, following the precedent introduced for DVI in #3613.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Summary by CodeRabbit
Bug Fixes
Tests