Add more ASV benchmarks#361
Conversation
3176a96 to
2284357
Compare
|
Thanks, this is a good step toward resolving #345 , and we can update these benchmarks with the specs from that issue. I'm still confirming env counts to test cloning and perf with with @kellyguo11 . |
bdac643 to
9e18b9a
Compare
|
For the record, these are how long it took to run each benchmark: |
4cac7e1 to
9ccccb0
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes introduce a comprehensive overhaul of the benchmarking suite for the Newton project. New, modular ASV benchmarks are added for various simulation examples, replacing the previous cartpole benchmark. Benchmark configuration is updated for deterministic environment setup and longer timeouts. Documentation is expanded to include detailed requirements, benchmarking instructions, and command-line argument style guidance. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ASV
participant BenchmarkScript
participant ExampleScript
participant WarpLib
User->>ASV: Run benchmarks
ASV->>BenchmarkScript: Invoke benchmark class/method
BenchmarkScript->>WarpLib: (Optional) Initialize Warp, clear caches
alt Subprocess-based benchmark
BenchmarkScript->>ExampleScript: Run as subprocess with CLI args
ExampleScript->>WarpLib: Initialize and run simulation
ExampleScript-->>BenchmarkScript: Output/results
else In-process benchmark
BenchmarkScript->>WarpLib: Create Example instance, step simulation
BenchmarkScript->>WarpLib: Synchronize device
end
BenchmarkScript-->>ASV: Timing/memory results
ASV-->>User: Benchmark report
sequenceDiagram
participant Developer
participant Docs
Developer->>Docs: Read requirements and benchmarking guide
Docs-->>Developer: Provides Python/NVIDIA/Warp requirements, ASV usage, CLI style guide
📜 Recent review detailsConfiguration used: .coderabbit.yml 📥 CommitsReviewing files that changed from the base of the PR and between 9ccccb0cc472ad29fcb9199f102c80739d894ae8 and a99f18c. ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
asv/benchmarks/examples/example_anymal.py (1)
54-66: Note naming inconsistency with other benchmark files.The class is named
PretrainedSimulatewhile similar classes in other example files are namedMuJoCoSolverSimulate(e.g., inexample_humanoid.pyandexample_cartpole.py). Consider using consistent naming across all example benchmarks for better discoverability.-class PretrainedSimulate: +class MuJoCoSolverSimulate:asv/benchmarks/examples/example_cartpole.py (2)
25-34: Fix unused variable in ModelMemory class.The variable
_exampleis created but never used, which may not accurately represent the memory usage pattern intended for benchmarking.Consider either using the example instance or removing the underscore prefix if it's intentionally unused:
def peakmem_initialize_model_cpu(self, num_envs): with wp.ScopedDevice("cpu"): - _example = Example(stage_path=None, num_envs=num_envs) + example = Example(stage_path=None, num_envs=num_envs)
36-47: Fix unused variable in InitializeModel class.Similar to ModelMemory, the variable
_exampleis created but never used.Apply the same fix as in ModelMemory:
def time_initialize_model(self, num_envs): with wp.ScopedDevice("cpu"): - _example = Example(stage_path=None, num_envs=num_envs) + example = Example(stage_path=None, num_envs=num_envs)asv/benchmarks/examples/example_quadruped.py (3)
25-34: Fix unused variable in ModelMemory class.The variable
_exampleis created but never used, consistent with the same issue in other benchmark files.def peakmem_initialize_model_cpu(self, num_envs): with wp.ScopedDevice("cpu"): - _example = Example(stage_path=None, num_envs=num_envs) + example = Example(stage_path=None, num_envs=num_envs)
36-47: Fix unused variable in InitializeModel class.Same unused variable issue as other benchmark files.
def time_initialize_model(self, num_envs): with wp.ScopedDevice("cpu"): - _example = Example(stage_path=None, num_envs=num_envs) + example = Example(stage_path=None, num_envs=num_envs)
49-77: Consider adding --no-use-cuda-graph flag for consistency.The quadruped ExampleLoad benchmark doesn't include the
--no-use-cuda-graphflag, which is used in the cartpole and selection_cartpole benchmarks. This inconsistency might affect benchmark comparison accuracy.Consider adding the flag for consistency:
command = [ sys.executable, "-m", "newton.examples.example_quadruped", "--stage-path", "None", "--num-frames", "1", + "--no-use-cuda-graph", ]
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 126ddc3 and 9ccccb0cc472ad29fcb9199f102c80739d894ae8.
⛔ Files ignored due to path filters (4)
docs/generated/core/newton.Mesh.rstis excluded by!**/generated/**docs/generated/core/newton.Model.rstis excluded by!**/generated/**docs/generated/core/newton.ModelBuilder.rstis excluded by!**/generated/**docs/generated/solvers/newton.solvers.MuJoCoSolver.rstis excluded by!**/generated/**
📒 Files selected for processing (9)
asv.conf.json(1 hunks)asv/benchmarks/cartpole.py(0 hunks)asv/benchmarks/examples/example_anymal.py(1 hunks)asv/benchmarks/examples/example_cartpole.py(1 hunks)asv/benchmarks/examples/example_humanoid.py(1 hunks)asv/benchmarks/examples/example_quadruped.py(1 hunks)asv/benchmarks/examples/example_selection_cartpole.py(1 hunks)docs/development-guide.rst(2 hunks)docs/guide/quickstart.rst(1 hunks)
💤 Files with no reviewable changes (1)
- asv/benchmarks/cartpole.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
asv/benchmarks/examples/example_anymal.py (3)
asv/benchmarks/examples/example_cartpole.py (7)
ExampleLoad(49-77)setup(28-29)setup(41-42)setup(55-57)setup(84-86)time_load(60-77)time_simulate(89-92)asv/benchmarks/examples/example_humanoid.py (5)
ExampleLoad(25-52)setup(31-33)setup(59-61)time_load(36-52)time_simulate(64-67)asv/benchmarks/examples/example_selection_cartpole.py (5)
ExampleLoad(25-53)setup(31-33)setup(60-62)time_load(36-53)time_simulate(65-68)
asv/benchmarks/examples/example_selection_cartpole.py (1)
asv/benchmarks/examples/example_cartpole.py (8)
ExampleLoad(49-77)setup(28-29)setup(41-42)setup(55-57)setup(84-86)time_load(60-77)MuJoCoSolverSimulate(80-92)time_simulate(89-92)
asv/benchmarks/examples/example_humanoid.py (3)
asv/benchmarks/examples/example_anymal.py (5)
ExampleLoad(25-51)setup(31-33)setup(58-60)time_load(36-51)time_simulate(63-66)asv/benchmarks/examples/example_cartpole.py (8)
ExampleLoad(49-77)setup(28-29)setup(41-42)setup(55-57)setup(84-86)time_load(60-77)MuJoCoSolverSimulate(80-92)time_simulate(89-92)asv/benchmarks/examples/example_selection_cartpole.py (6)
ExampleLoad(25-53)setup(31-33)setup(60-62)time_load(36-53)MuJoCoSolverSimulate(56-68)time_simulate(65-68)
asv/benchmarks/examples/example_cartpole.py (3)
asv/benchmarks/examples/example_anymal.py (5)
setup(31-33)setup(58-60)ExampleLoad(25-51)time_load(36-51)time_simulate(63-66)asv/benchmarks/examples/example_humanoid.py (6)
setup(31-33)setup(59-61)ExampleLoad(25-52)time_load(36-52)MuJoCoSolverSimulate(55-67)time_simulate(64-67)asv/benchmarks/examples/example_selection_cartpole.py (6)
setup(31-33)setup(60-62)ExampleLoad(25-53)time_load(36-53)MuJoCoSolverSimulate(56-68)time_simulate(65-68)
🔇 Additional comments (13)
docs/guide/quickstart.rst (1)
4-17: Well-structured requirements section with clear GPU driver guidance.The new requirements section provides essential information for users, particularly the GPU driver version requirements that align with the new GPU-dependent benchmarks being added.
asv.conf.json (2)
15-15: Appropriate timeout increase for GPU benchmarks.The increase from 120 to 360 seconds is reasonable given that GPU benchmarks typically require more time for setup, compilation, and execution.
19-24: Good dependency pinning for benchmark reproducibility.Pinning exact versions and commit hashes ensures deterministic benchmark environments, which is essential for reliable performance measurements across different runs and environments.
docs/development-guide.rst (2)
285-285: Clear style guideline for command-line arguments.The kebab-case requirement for CLI arguments improves consistency and follows common conventions.
298-356: Comprehensive and well-structured benchmarking documentation.The airspeed velocity section provides thorough guidance including:
- Installation and setup instructions
- Command examples with proper flags
- Benchmark development best practices
- Performance tuning guidance
This documentation will be valuable for developers working with the new benchmark suite.
asv/benchmarks/examples/example_anymal.py (1)
25-52: Consistent ExampleLoad implementation with proper environment setup.The benchmark follows the established pattern with appropriate cache clearing and subprocess execution. The CUDA device check ensures the benchmark only runs in suitable environments.
asv/benchmarks/examples/example_humanoid.py (2)
25-53: Well-implemented ExampleLoad benchmark with proper setup.The implementation correctly follows the established pattern with cache clearing, subprocess execution, and appropriate CUDA device/driver checks.
55-67: Consistent MuJoCoSolverSimulate implementation.The simulation benchmark follows the established pattern with proper setup, stepping loop, and device synchronization. The repeat count of 5 appears reasonable for this example's performance characteristics.
asv/benchmarks/examples/example_selection_cartpole.py (2)
25-54: LGTM: ExampleLoad benchmark implementation is correct.The benchmark properly clears caches, uses appropriate subprocess arguments, and includes proper CUDA device checks. The implementation follows the established pattern from other example benchmarks.
56-68: LGTM: MuJoCoSolverSimulate benchmark implementation is correct.The simulation benchmark correctly initializes the example with appropriate parameters (16 environments, CUDA graph enabled), includes proper device and driver version checks, and synchronizes the device after simulation steps.
asv/benchmarks/examples/example_cartpole.py (2)
49-78: LGTM: ExampleLoad benchmark implementation is correct.The benchmark properly includes the
--no-use-cuda-graphflag and follows the established pattern consistently.
80-92: LGTM: MuJoCoSolverSimulate benchmark implementation is correct.The simulation benchmark uses appropriate parameters (8 environments, 200 frames) and includes proper device and driver version checks.
asv/benchmarks/examples/example_quadruped.py (1)
79-91: No driver version check needed for XPBD benchmark
The XPBDSolverSimulate example dynamically falls back when CUDA Graph/mempools aren’t supported (viawp.is_mempool_enabled) and only requires a GPU, so addingdriver_version < (12, 3)is unnecessary. The existing@skip_benchmark_if(wp.get_cuda_device_count() == 0)in asv/benchmarks/examples/example_quadruped.py (lines 79–91) is correct.
Likely an incorrect or invalid review comment.
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
Signed-off-by: Eric Shi <ershi@nvidia.com>
9ccccb0 to
a99f18c
Compare
|
|
||
| def setup(self): | ||
| self.num_frames = 50 | ||
| self.example = Example(stage_path=None, headless=True) |
There was a problem hiding this comment.
How do we know this is initialized with device=cuda?
There was a problem hiding this comment.
The @skip_benchmark_if should take care of it. The setup should only run if there's a benchmark to run.
Signed-off-by: Eric Shi <ershi@nvidia.com>
# Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> Added in a detail for selecting the Rendering Mode from the CLI argument. <!-- As a practice, it is recommended to open an issue to have discussions on the proposed pull request. This makes it easier for the community to keep track of what is being developed or added, and if a given feature is demanded by more than one party. --> ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [ ] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
Signed-off-by: Eric Shi <ershi@nvidia.com>
Description
Something that will be a problem until Newton/Warp/MJWarp developement reaches a more stable state is how to choose the version of the other dependencies to install when benchmarking. I intentionally fixed the MJWarp Git hash for now.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
pre-commit run -aSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores