-
Notifications
You must be signed in to change notification settings - Fork 584
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
add Rerun integrations doc #5255
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a new integration card for "Rerun" in the FiftyOne documentation, enhancing the existing tutorial cards. A dedicated documentation file ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/source/integrations/rerun.rst (5)
22-26
: Consider adding version requirementsIt would be helpful to specify minimum version requirements for both FiftyOne and Rerun SDK packages.
51-61
: Enhance code example with error handling and path clarificationConsider:
- Adding error handling for file operations
- Clarifying that the path should be absolute or relative
- Adding a note about supported file formats
# create a new recording recording = rr.new_recording(application_id="my_dataset", recording_id="my_scene.rrd") # log example data -recording.log("my_points", rr.Points3D(positions, colors=colors, radii=0.5)) +try: + recording.log("my_points", rr.Points3D(positions, colors=colors, radii=0.5)) + # Use os.path for cross-platform compatibility + recording.save(os.path.abspath("/path/to/my_scene.rrd")) +except Exception as e: + print(f"Error logging data: {e}") -recording.save("/path/to/my_scene.rrd")
73-79
: Enhance dataset example with concrete use caseThe example could be more helpful with:
- A concrete dataset initialization example
- Proper path handling
- Error handling for file existence
-dataset = fo.Dataset(...) +# Example with a concrete dataset +dataset = fo.Dataset("my_dataset") + for sample in dataset: - rrd_path = "/path/to/my_scene.rrd" + rrd_path = os.path.join(data_dir, f"{sample.id}.rrd") + if not os.path.exists(rrd_path): + print(f"Warning: RRD file not found: {rrd_path}") + continue sample["rerun"] = RrdFile(rrd_path)
81-93
: Add troubleshooting section for common server issuesConsider adding a troubleshooting section addressing:
- Common connection issues
- Port conflicts
- Network accessibility problems
114-123
: Enhance example usage section with more contextConsider:
- Adding a brief overview of what the example demonstrates
- Listing prerequisites for running the example
- Including expected outcomes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/source/_static/images/integrations/rerun.jpeg
is excluded by!**/*.jpeg
📒 Files selected for processing (2)
docs/source/integrations/index.rst
(2 hunks)docs/source/integrations/rerun.rst
(1 hunks)
🔇 Additional comments (2)
docs/source/integrations/rerun.rst (1)
1-14
: Well-structured introduction!
The introduction clearly explains what Rerun is and how it integrates with FiftyOne.
docs/source/integrations/index.rst (1)
184-189
: Integration card properly added!
The Rerun integration card follows the established format and is correctly added to both the cards section and toctree.
Also applies to: 230-230
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: 0
🧹 Outside diff range and nitpick comments (6)
docs/source/integrations/rerun.rst (6)
26-26
: Consider specifying version requirementsFor better reproducibility and compatibility, consider specifying minimum required versions for both packages.
- $ pip install fiftyone rerun-sdk + $ pip install "fiftyone>=0.22.0" "rerun-sdk>=0.9.0"
20-23
: Add system requirements sectionConsider adding system requirements or prerequisites before the installation steps, such as:
- Supported Python versions
- Operating system compatibility
- Hardware requirements (if any)
75-79
: Add error handling to the code exampleConsider enhancing the code example with error handling for common scenarios:
for sample in dataset: - rrd_path = "/path/to/my_scene.rrd" - sample["rerun"] = RrdFile(rrd_path) + rrd_path = "/path/to/my_scene.rrd" + try: + if os.path.exists(rrd_path): + sample["rerun"] = RrdFile(rrd_path) + else: + logger.warning(f"RRD file not found: {rrd_path}") + except Exception as e: + logger.error(f"Failed to process {rrd_path}: {e}")
87-88
: Add server configuration detailsConsider adding information about:
- How to configure a different port
- Network configuration for remote access
- Troubleshooting common connection issues
117-123
: Enhance the example sectionConsider adding:
- A brief overview of what users will learn from the example
- Screenshots or diagrams showing the expected visualization
- Key features demonstrated in the example
108-109
: Add a troubleshooting sectionConsider adding a "Troubleshooting" section that covers:
- Common issues and their solutions
- FAQ
- Known limitations
- Best practices for optimal performance
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
docs/source/integrations/rerun.rst
(1 hunks)
🔇 Additional comments (1)
docs/source/integrations/rerun.rst (1)
1-14
: LGTM! Well-structured documentation header
The file structure follows RST best practices with proper reference anchors and formatting. The overview effectively communicates the purpose and value proposition of the Rerun integration.
What changes are proposed in this pull request?
Add a dedicated integration doc for Rerun. Note that a new integration category (
Visualization
) has been added.Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit
New Features
.rrd
files with FiftyOne datasets.Documentation