OMPE-90534: Add json nsys trace definitions#5397
OMPE-90534: Add json nsys trace definitions#5397jmart-nv wants to merge 1 commit intoisaac-sim:developfrom
Conversation
- Added new nsys_trace.json (moved from omniperf benchmark) to decorate functions of interest in isaac traces without requiring decorators in the codebase. - Added a unit test to ensure the json stays in sync.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds an nsys_trace.json configuration file for Nsight Systems profiling and a pytest-based validation suite to ensure the JSON stays synchronized with the actual codebase. The implementation is well-structured with thoughtful test design that catches stale references while warning about potential coverage gaps.
Architecture Impact
Self-contained. This adds tooling in scripts/benchmarks/ with no impact on core IsaacLab runtime code. The JSON is consumed externally by nsys --python-functions-trace and the tests validate it at CI time. No existing code paths are modified.
Implementation Verdict
Minor fixes needed
Test Coverage
The test design is solid — test_function_resolves validates every JSON entry can be resolved, and test_warn_unreferenced_methods_on_covered_classes provides coverage gap detection. However, there are issues with warning suppression and one parsing edge case that could cause silent failures.
CI Status
No CI checks available yet — manual verification of test execution recommended.
Findings
🟡 Warning: scripts/benchmarks/test/test_nsys_trace.py:140-145 — Warnings emitted via warnings.warn() will be suppressed by pytest's default filters
The test_warn_unreferenced_methods_on_covered_classes test emits warnings but doesn't configure pytest to surface them. By default, pytest may filter these UserWarning instances. The test will pass silently even when unreferenced methods exist, defeating its purpose.
Consider either:
- Using
pytest.warns()context manager to capture and assert on warnings - Adding
-W defaultto pytest invocation in CI - Converting to
pytest.skip()with a message, or writing to a report file
# Alternative: collect and report at end
if unreferenced:
pytest.fail(
f"{module_name}.{class_name} has public methods not listed: {sorted(unreferenced)}",
pytrace=False
)🟡 Warning: scripts/benchmarks/test/test_nsys_trace.py:74-77 — Method name extraction assumes exactly 2 parts, but JSON has methods like __init__
The parsing logic class_name, method_name = parts[0], parts[-1] works, but _is_own_public_method at line 85 filters out __init__ because it starts with _. This means dunder methods in the JSON won't trigger "unreferenced method" warnings for sibling methods on the same class. This is likely intentional but worth documenting in the docstring.
🔵 Improvement: scripts/benchmarks/test/test_nsys_trace.py:89 — Module-level code execution during collection triggers AppLauncher before test discovery
_FUNCTION_PAIRS = _iter_function_pairs(_load_trace_entries())This runs at import time, meaning the JSON is loaded during pytest collection. If the JSON has syntax errors, the error message will be confusing (collection failure vs. test failure). Consider wrapping in a fixture or moving inside tests:
@pytest.fixture(scope="module")
def function_pairs():
return _iter_function_pairs(_load_trace_entries())🔵 Improvement: scripts/benchmarks/test/test_nsys_trace.py:17-19 — AppLauncher initialization is heavy for JSON validation tests
The AppLauncher with simulation context is initialized even though these tests primarily validate JSON structure and import resolution. Many of the validated modules (torch.autograd, warp, importlib) don't require simulation. Consider:
- Adding a pytest marker to distinguish tests that need simulation from pure validation
- Using lazy initialization only when resolving IsaacLab-specific modules
🔵 Improvement: scripts/benchmarks/nsys_trace.json:55-56 — ManagerBasedEnv._reset_idx is a private method but included in trace
This is intentional for profiling, but the naming inconsistency with the test_warn_unreferenced_methods_on_covered_classes test's exclusion of _ prefixed methods means you won't get warnings if new private methods are added to traced classes. Document this intentional asymmetry.
🔵 Improvement: scripts/benchmarks/test/test_nsys_trace.py:36-38 — No error handling for malformed JSON
def _load_trace_entries() -> list[dict]:
with TRACE_JSON_PATH.open() as f:
return json.load(f)If the JSON is malformed or the file doesn't exist, the error will be a confusing collection-time failure. Consider adding explicit validation:
def _load_trace_entries() -> list[dict]:
if not TRACE_JSON_PATH.exists():
pytest.fail(f"Trace JSON not found at {TRACE_JSON_PATH}")
try:
with TRACE_JSON_PATH.open() as f:
return json.load(f)
except json.JSONDecodeError as e:
pytest.fail(f"Invalid JSON in {TRACE_JSON_PATH}: {e}")🔵 Improvement: scripts/benchmarks/test/test_nsys_trace.py — Missing __init__.py in test directory
The scripts/benchmarks/test/ directory should have an __init__.py file for proper pytest discovery and to follow the pattern used elsewhere in the codebase.
| @@ -0,0 +1,147 @@ | |||
| # Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
this seems to be a test script, maybe we can put it under isaaclab/tests instead. the scripts folder was meant to hold standalone scripts that can be executed directly. putting it under isaaclab should also help keep it together with the rest of the core-related test scripts
| @@ -0,0 +1,167 @@ | |||
| [ | |||
There was a problem hiding this comment.
perhaps we can have a page in the how-to section of the docs to describe this system? we can explain cases where benchmarking with nsys would be helpful, how this .json works together with nsys, and maybe an example of a profile and high level descriptions of what to look for
Description
This migrates json nsys trace definitions from omniperf benchmark to isaaclab itself. Part 1/2
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there