Add tiled camera scene to model build benchmark - #3812
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe benchmark setup adds tiled-camera model initialization benchmarks. The shared implementation measures replication, initialization, and finalization for replicated scenes. KPI and fast variants use different world counts and are registered in the benchmark CLI. ChangesTiled-camera model initialization benchmarks
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TiledCameraBenchmark
participant SensorTiledCamera
participant Newton
participant CUDA
TiledCameraBenchmark->>SensorTiledCamera: Build and finalize tiled-camera scenes
TiledCameraBenchmark->>Newton: Replicate and initialize model
TiledCameraBenchmark->>Newton: Finalize model
Newton->>CUDA: Execute timed operations
CUDA-->>TiledCameraBenchmark: Return synchronized timings
TiledCameraBenchmark->>Newton: Release replicated builder and scene state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 `@asv/benchmarks/setup/bench_model.py`:
- Around line 123-125: Update the warmup model construction around
newton.ModelBuilder and the warmup replication call to use the same
_replicate(1) path as the timed benchmark variations, ensuring the global ground
plane is included before finalize().
🪄 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: 88c51276-7888-4cc0-879b-e2748714f4bf
📒 Files selected for processing (1)
asv/benchmarks/setup/bench_model.py
| warmup = newton.ModelBuilder() | ||
| warmup.replicate(self.world, 1) | ||
| warmup.finalize() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'def finalize\(|add_ground_plane|shape.*plane|collision' newton/_src/sim/builder.py
rg -n -C 6 'warmup|_replicate|add_ground_plane|time_initialize_model|time_finalize_model' \
asv/benchmarks/setup/bench_model.pyRepository: newton-physics/newton
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the ASV benchmark file around the relevant methods.
rg -n -C 12 'class TiledCamera|_replicate|warmup|add_ground_plane|def run|TWO|FAST' asv/benchmarks/setup/bench_model.py
# Look at ModelBuilder.finalize implementation and ground-plane shape model fields without outputting the whole file.
rg -n -C 16 'def finalize\(|def add_ground_plane|_add_body|shape_.*plane|plane.*shape|ground_plane|contact' newton/_src/sim/builder.py | sed -n '1,260p'Repository: newton-physics/newton
Length of output: 15778
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact finalize and add_ground_plane implementations.
rg -n 'def finalize\(|def add_ground_plane\(' newton/_src/sim/builder.py
finalize_line=$(rg -n 'def finalize\(' newton/_src/sim/builder.py | head -n1 | cut -d: -f1)
ground_line=$(rg -n 'def add_ground_plane\(' newton/_src/sim/builder.py | head -n1 | cut -d: -f1)
if [ ${finalize_line:-0} -gt 0 ]; then
start=$((finalize_line - 20))
[ "$start" -lt 1 ] && start=1
end=$((finalize_line + 120))
sed -n "${start},${end}p" newton/_src/sim/builder.py | cat -n | sed 's/^/builder.py:/'
fi
if [ ${ground_line:-0} -gt 0 ]; then
start=$((ground_line - 10))
[ "$start" -lt 1 ] && start=1
end=$((ground_line + 60))
sed -n "${start},${end}p" newton/_src/sim/builder.py | cat -n | sed 's/^/builder.py:/'
fi
# Check whether finalize creates/adds plane-specific arrays/arrays backed by model attributes.
rg -n 'ground|plane.*array|array\(|compute_ground|plane_collision|plane.*body|shape.*plane' newton/_src/sim/builder.py newton/_src -g '*.py' | head -n 200Repository: newton-physics/newton
Length of output: 36353
Warm up the complete benchmark model.
The warmup model omits the global ground plane that both timed variations add before finalization. Build the warmup through _replicate(1) so it matches the model configuration used in the timed runs.
🤖 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 `@asv/benchmarks/setup/bench_model.py` around lines 123 - 125, Update the
warmup model construction around newton.ModelBuilder and the warmup replication
call to use the same _replicate(1) path as the timed benchmark variations,
ensuring the global ground plane is included before finalize().
StafaH
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
jcarius-nv
left a comment
jcarius-nv
left a comment
There was a problem hiding this comment.
Overall, this looks good. The Fast/KPI split is appropriate, and the new Fast cases appear suitable for PR coverage. Thanks, Mustafa, for the contribution and for expanding the model-building regression coverage!
| warmup = newton.ModelBuilder() | ||
| warmup.replicate(self.world, 1) | ||
| warmup.finalize() | ||
| self.replicated = self._replicate(world_count) |
There was a problem hiding this comment.
Could we rename this to self.replicated_builder? The value is a mutable ModelBuilder, rather than a completed replicated model, and the more explicit name would make the distinction clear.
|
|
||
|
|
||
| class _InitializeModelTiledCamera: | ||
| """Replicate + finalize scenes with tiled camera and collision handling enabled.""" |
There was a problem hiding this comment.
Could we clarify this wording? The benchmark reuses scenes from the tiled-camera benchmark, but it does not instantiate or measure a SensorTiledCamera; collision handling remains enabled. If the intent is model-building coverage for tiled-camera scenes, the docstring and PR description should say that explicitly.
There was a problem hiding this comment.
Done. I added initialization of sensor tiled camera, so that we can measure that as well.
| del self.world | ||
|
|
||
|
|
||
| class KpiInitializeModelTiledCamera(_InitializeModelTiledCamera): |
There was a problem hiding this comment.
Was KpiInitializeModelTiledCamera exercised by the full nightly ASV suite? The PR workflow only selects Fast*.time_*, so it cannot validate that the 4,096-world variants complete within their runtime and memory budget.
There was a problem hiding this comment.
No, it does not get picked up, I just followed the convention that the other tests in this file go though, leaving this Kpi for manual runs, and a Fast variant for the CI.
2 participants
Description
Add two additional scenes for model initialization benchmarks that include a tiled camera sensor and collision handling.
Checklist
changelog fragment instructions
Summary by CodeRabbit