-
Notifications
You must be signed in to change notification settings - Fork 127
mmiranda Nvidia style guide edits to pipeline visualization #1328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: meg miranda <[email protected]>
| - **tensor.fbs**: Tensor data structure based on DLPack | ||
|
|
||
| ## Prerequisites | ||
| ## Visualizer Python Prerequisites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a shot at what these prereq's were for. Please verify.
Greptile SummaryThis PR applies NVIDIA style guide formatting improvements to the Pipeline Visualization README. The changes include:
One minor syntax issue found: redundant wording "packages dependencies" on line 193. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant File as README.md
participant StyleGuide as NVIDIA Style Guide
Dev->>File: Read current documentation
Dev->>StyleGuide: Review formatting requirements
StyleGuide-->>Dev: Bullet points for components
StyleGuide-->>Dev: Numbered lists for sequential steps
StyleGuide-->>Dev: Formal language over casual
StyleGuide-->>Dev: Consistent punctuation
Dev->>File: Convert numbered list to bullets (lines 11-13)
Dev->>File: Restructure command examples (lines 44-55)
Dev->>File: Remove bold from inline headings (lines 57, 70)
Dev->>File: Add periods to list items (lines 78-81)
Dev->>File: Change "via" to "using" (line 94)
Dev->>File: Change "e.g." to "for example" (line 212)
Dev->>File: Improve section cross-references (lines 152-191)
Dev->>File: Standardize troubleshooting punctuation
File-->>Dev: Documentation updated with style guide compliance
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
WalkthroughREADME for applications/pipeline_visualization was reformatted and reworded: list styles, step headings, inline code/command formatting, labels, punctuation, and phrasing were standardized; no functional code, API, or exported-entity changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2025-10-22T16:33:55.411ZApplied to files:
📚 Learning: 2025-11-17T21:37:54.245ZApplied to files:
📚 Learning: 2025-12-18T16:19:45.117ZApplied to files:
📚 Learning: 2025-12-18T16:19:45.117ZApplied to files:
📚 Learning: 2025-11-11T15:40:28.150ZApplied to files:
🪛 markdownlint-cli2 (0.18.1)applications/pipeline_visualization/README.md190-190: Link fragments should be valid (MD051, link-fragments) 🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @applications/pipeline_visualization/README.md:
- Line 190: Update the broken link fragment in the README by changing the
reference from #visualizer-python-dependencies to the actual anchor
#visualizer-python-prerequisites so the phrase referring to the Visualizer
Python dependencies points to the "Visualizer Python Prerequisites" section;
locate the link text in the same paragraph referencing the Holohub container and
replace the fragment accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/pipeline_visualization/README.md
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(applications|workflows))/**/README.md : Performance insights and any latency optimizations should be documented in the application README
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-10-22T16:33:55.411Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/python/streaming_client_demo_replayer.yaml:27-36
Timestamp: 2025-10-22T16:33:55.411Z
Learning: In the video_streaming bidirectional client applications (applications/video_streaming/video_streaming_client), the pipeline has two separate data paths: (1) Outgoing: source → format_converter → streaming_client INPUT (sends to server), and (2) Incoming: streaming_client OUTPUT → holoviz (receives from server). The format_converter prepares data for transmission and does NOT feed directly into holoviz visualization.
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-11-17T21:37:54.245Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/README.md:135-137
Timestamp: 2025-11-17T21:37:54.245Z
Learning: In the Holohub repository, when both Python and C++ applications are available for the same application, the Holohub CLI defaults to Python if the `--language` argument is not provided. Users can explicitly specify `--language cpp` or `--language python` to select the desired implementation.
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/applications/**/CMakeLists.txt : CMakeLists.txt for applications must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/*.{py,cpp,hpp} : Code must adhere to Holoscan SDK coding standards including style compliance, descriptive naming, and minimal abbreviations
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-11-11T15:40:28.150Z
Learnt from: AndreasHeumann
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/cpp/create_tensor.hpp:21-22
Timestamp: 2025-11-11T15:40:28.150Z
Learning: In the pipeline_visualization application, FlatBuffers generated headers like tensor_generated.h can be included using the simplified path `#include <flatbuffers/tensor_generated.h>` because the CMake target pipeline_visualization_flatbuffers_schemas generates them and pipeline_visualization_flatbuffers_target exposes the correct include directories, which are inherited by targets that link against it.
Applied to files:
applications/pipeline_visualization/README.md
🪛 markdownlint-cli2 (0.18.1)
applications/pipeline_visualization/README.md
190-190: Link fragments should be valid
(MD051, link-fragments)
⏰ 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). (1)
- GitHub Check: Greptile Review
| installed separately. See the next section for details. | ||
|
|
||
| ### Visualizer Python Dependencies | ||
| the Holohub container. Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-dependencies) must be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix broken link fragment.
Line 190 references #visualizer-python-dependencies, but the section heading is "Visualizer Python Prerequisites" which generates the anchor #visualizer-python-prerequisites. Update the link to match the correct anchor.
🔗 Proposed fix
- Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-dependencies) must be
+ Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-prerequisites) must be📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| the Holohub container. Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-dependencies) must be | |
| the Holohub container. Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-prerequisites) must be |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
190-190: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
In @applications/pipeline_visualization/README.md at line 190, Update the broken
link fragment in the README by changing the reference from
#visualizer-python-dependencies to the actual anchor
#visualizer-python-prerequisites so the phrase referring to the Visualizer
Python dependencies points to the "Visualizer Python Prerequisites" section;
locate the link text in the same paragraph referencing the Holohub container and
replace the fragment accordingly.
| - Acquisition timestamp (nanoseconds) | ||
| - Publish timestamp (nanoseconds) | ||
|
|
||
| ### FlatBuffers Schemas (`schemas/`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section had my brain asking questions....the sort of: how does this work, why do I care about this for visualization, and do i need to install the dependencies before or after using the visualizer? the data format for what? Then to be honest my brain just started pinging me with other random and less relevant questions.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: megnvidia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR applies NVIDIA style guide edits to the pipeline visualization README, improving documentation formatting and readability. The changes include converting numbered lists to use asterisks for consistency, restructuring command examples with proper indentation, improving punctuation in step-by-step instructions, and replacing phrases like "via" with "using" for clarity.
Key improvements:
- Converted main component list from numbered format to bullet points for consistency (lines 11-13)
- Restructured command-line examples with better organization and indentation (lines 44-55)
- Added periods to all step instructions for consistency (lines 78-81)
- Changed "simple" to "basic" for more professional tone (line 141)
- Improved heading structure and cross-references between sections
Issues found:
- Broken internal link on line 190 that points to non-existent anchor
- Minor grammar issue with unnecessary comma on line 282
Confidence Score: 4/5
- This PR is safe to merge after fixing the broken internal link
- The changes are documentation-only style guide improvements that enhance readability and consistency. One critical issue (broken internal link) must be fixed as it will result in a non-functional link, and one minor style improvement would be nice to have
- The README.md file needs attention for the broken link on line 190 before merging
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| applications/pipeline_visualization/README.md | 4/5 | Documentation style guide improvements with mostly good formatting changes, but has a broken internal link and a minor grammar issue |
Sequence Diagram
sequenceDiagram
participant Author as PR Author
participant README as README.md
participant StyleGuide as NVIDIA Style Guide
participant Readers as Documentation Readers
Author->>StyleGuide: Review style guidelines
StyleGuide-->>Author: Formatting rules (bullets, indentation, punctuation)
Author->>README: Convert numbered list to bullets (lines 11-13)
Author->>README: Restructure command examples with indentation (lines 44-55)
Author->>README: Add periods to step instructions (lines 78-81)
Author->>README: Replace "via" with "using" (line 94)
Author->>README: Change "simple" to "basic" (line 141)
Author->>README: Update cross-reference links (line 160, 190)
Author->>README: Improve punctuation consistency throughout
Note over README: Internal link issue:<br/>Line 190 points to wrong anchor
README-->>Readers: Improved formatting and clarity
Readers->>README: Navigate using internal links
Note over Readers,README: Broken link on line 190<br/>needs to be fixed
| installed separately. See the next section for details. | ||
|
|
||
| ### Visualizer Python Dependencies | ||
| the Holohub container. Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-dependencies) must be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
broken internal link - the anchor #visualizer-python-dependencies doesn't exist. The section is titled "Visualizer Python Prerequisites" which generates anchor #visualizer-python-prerequisites
| the Holohub container. Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-dependencies) must be | |
| the Holohub container. Because the visualizer is run outside the Holohub container, its [dependencies](#visualizer-python-prerequisites) must be |
| - Verify the operator names match between the app and visualizer | ||
| - For static visualizer, ensure the unique IDs in the code match your operators | ||
| - Verify that `log_tensor_data_content: true` is in the YAML config | ||
| - Verify that the operator names match, between the app and visualizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awkward comma placement - "match, between" creates an unnecessary pause
| - Verify that the operator names match, between the app and visualizer | |
| - Verify that the operator names match between the app and visualizer |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
AndreasHeumann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, they look good. Please take care of the AI code review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR applies NVIDIA style guide edits to the Pipeline Visualization documentation, improving consistency and readability throughout the README.
Key Changes:
- Changed numbered lists to bullet points for component descriptions
- Improved code block formatting with proper indentation for nested steps
- Standardized punctuation (added periods to list items, removed exclamation marks)
- Replaced informal phrases: "via" → "using", "e.g." → "for example"
- Removed bold formatting from non-heading labels like "Command-line Options" and "Example with custom settings"
- Changed "simple" to "basic" for pipeline description
- Reorganized Python Visualizers section: moved shared information before subsection descriptions for better flow
- Improved heading consistency: "1. Static Visualizer" → "Static Visualizer"
- Enhanced troubleshooting steps with proper numbered list formatting and code formatting
All changes are purely stylistic and documentation-focused with no functional code modifications.
Confidence Score: 5/5
- This PR is completely safe to merge with zero risk
- Score reflects that this is purely a documentation formatting update with no code changes, no functional modifications, and all edits improve readability and consistency according to NVIDIA style guidelines
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| applications/pipeline_visualization/README.md | 5/5 | Style guide edits applied throughout: changed numbered lists to bullets, improved code block formatting, standardized punctuation, replaced informal phrases (e.g., via→using, e.g.→for example), and reorganized visualizer documentation for clarity |
Sequence Diagram
sequenceDiagram
participant Author as PR Author
participant README as README.md
participant StyleGuide as NVIDIA Style Guide
Author->>StyleGuide: Review style guidelines
Author->>README: Apply formatting changes
Note over README: Bullet points instead of numbers
Note over README: Improved code block formatting
Note over README: Standardized punctuation
Note over README: Replaced informal phrases
Author->>README: Reorganize visualizer section
Note over README: Move common info before subsections
Note over README: Update headings and references
Author->>README: Commit changes
README-->>Author: Documentation updated
nvidia style guide edits to Live Streaming Data Web Dashboard with NATS
I would provide the link to the style guide and cheat sheet for it here, but the URL it leaves are horribly long.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.