Add NVTX instrumentation to Newton Warp Renderer#5294
Conversation
Greptile SummaryThis PR adds NVTX domain instrumentation to
Confidence Score: 4/5Safe to merge after adding the required changelog entry and bumping the extension version. The instrumentation pattern (try/except import, pre-computed attrs, try/finally for pop_range) is correct. The only outstanding issue is a process violation: no CHANGELOG entry and no extension.toml version bump, as required by project guidelines. All findings are P2. source/isaaclab_newton/docs/CHANGELOG.rst and source/isaaclab_newton/config/extension.toml need a new version entry and bump respectively. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant _nvtx_range as _nvtx_range decorator
participant nvtxDomain as nvtx.Domain
participant Method as Wrapped method
Caller->>_nvtx_range: call decorated method
_nvtx_range->>nvtxDomain: push_range(attrs)
_nvtx_range->>Method: fn(*args, **kwargs)
alt success
Method-->>_nvtx_range: return value
else exception
Method-->>_nvtx_range: raise
end
_nvtx_range->>nvtxDomain: pop_range() [always, via finally]
_nvtx_range-->>Caller: return / re-raise
note over _nvtx_range,nvtxDomain: If nvtx not installed, decorator is a no-op pass-through
Reviews (1): Last reviewed commit: "Formatting" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| import nvtx | ||
|
|
||
| _nvtx_domain = nvtx.Domain("NewtonWarpRenderer") | ||
|
|
||
| def _nvtx_range(message: str, color: str | None = None): | ||
| """Decorator that wraps a function in a Domain.push_range/pop_range pair.""" | ||
| attrs = _nvtx_domain.get_event_attributes(message=message, color=color) | ||
|
|
||
| def decorator(fn): | ||
| @functools.wraps(fn) | ||
| def wrapper(*args, **kwargs): | ||
| _nvtx_domain.push_range(attrs) | ||
| try: | ||
| return fn(*args, **kwargs) | ||
| finally: | ||
| _nvtx_domain.pop_range() | ||
|
|
||
| return wrapper | ||
|
|
||
| return decorator | ||
|
|
||
| except ImportError: | ||
|
|
||
| def _nvtx_range(message: str, color: str | None = None): | ||
| def decorator(fn): | ||
| return fn | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
nvtx is a new optional third-party package not previously used by isaaclab_newton. Per project guidelines, every source change requires (1) a new version heading in source/isaaclab_newton/docs/CHANGELOG.rst (never adding to an existing version) and (2) a matching version bump in source/isaaclab_newton/config/extension.toml. Neither was done here — extension.toml still shows 0.5.13 and the changelog has no entry for this feature. The PR checklist also leaves the changelog item unchecked.
Additionally, adding nvtx as a new optional dependency is noted by the project guidelines as something to avoid unless the benefit justifies it. If this is intentional, a brief comment or documentation note explaining how users can install nvtx to enable profiling would help discoverability.
|
Do we only wish to add nvtx to Newton Warp Renderer? Will OVRTX and IsaacSim RTX have nvtx instrumentation in the future, or are those updates not necessary? |
| @@ -211,6 +243,7 @@ def update_camera( | |||
| See :meth:`~isaaclab.renderers.base_renderer.BaseRenderer.update_camera`.""" | |||
| render_data.update(positions, orientations, intrinsics) | |||
|
|
|||
| @_nvtx_range("render", color="green") | |||
| def render(self, render_data: RenderData): | |||
| """Render and write to output buffers. See :meth:`~isaaclab.renderers.base_renderer.BaseRenderer.render`.""" | |||
| self.newton_sensor.update( | |||
| @@ -224,6 +257,7 @@ def render(self, render_data: RenderData): | |||
| shape_index_image=render_data.outputs.instance_segmentation_image, | |||
| ) | |||
|
|
|||
| @_nvtx_range("read_output", color="orange") | |||
| def read_output(self, render_data: RenderData, camera_data: CameraData) -> None: | |||
| """Copy rendered outputs to the camera data buffers. | |||
| See :meth:`~isaaclab.renderers.base_renderer.BaseRenderer.read_output`.""" | |||
There was a problem hiding this comment.
Hello, Huidong. Just curious, how did you find out about the JSON file (say, documentation or from Toby's results .md)? I am not too familiar with instrumentation, so this was something I thought was neat.
) We don't need custom instrumentation. Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. Fixes # (issue) <!-- 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) - New feature (non-breaking change which adds functionality) - Breaking change (existing functionality will not work without user modification) - Documentation update ## 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 read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [ ] 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 -->
)" (isaac-sim#5348) We don't need custom instrumentation. Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. Fixes # (issue) <!-- 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) - New feature (non-breaking change which adds functionality) - Breaking change (existing functionality will not work without user modification) - Documentation update ## 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 read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [ ] 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 -->
3 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there