Merge release/v1.1.0 to develop
#5190
Conversation
* fix delete and recreate dataset for server * _force_load
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/server/query.py (1)
588-588: Minor suggestion: Evaluate the necessity ofdataset.reload()after forcing dataset load.Since the dataset is forcibly loaded using
fo.Dataset(dataset_name, _create=False, _force_load=True), callingdataset.reload()immediately afterward may be redundant. Consider removing thedataset.reload()call if it's not necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/core/singletons.py(2 hunks)fiftyone/server/query.py(1 hunks)
🔇 Additional comments (1)
fiftyone/core/singletons.py (1)
41-43: Implementation looks good, verify related changes
The implementation correctly handles the force loading parameter with proper default value handling. However, we should verify the implementation of the _update_last_loaded_at() method to ensure it properly handles the new force parameter.
Let's verify the implementation of the _update_last_loaded_at() method:
✅ Verification successful
Implementation verified and properly integrated
The implementation is correctly designed and integrated across the codebase:
- The
_update_last_loaded_at(force=False)method is properly implemented infiftyone/core/dataset.pywith the force parameter - The method correctly handles the force parameter by checking
if os.environ.get("FIFTYONE_SERVER", False) and not force - The feature is used appropriately in the server query handling (
fiftyone/server/query.py) with_force_load=True - The change maintains backward compatibility through default values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation and usage of _update_last_loaded_at
ast-grep --pattern 'def _update_last_loaded_at($$$)'
# Search for any tests related to force loading
rg -l "force.*load|load.*force" "tests/"
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for the method implementation
rg -A 5 "_update_last_loaded_at"
# Also search for force loading related code
rg -A 5 "_force_load"
# Search for any class that might contain this method
ast-grep --pattern 'class $_ {
$$$
_update_last_loaded_at($$$) {
$$$
}
$$$
}'
Length of output: 3649
* fix confusion matrix in model evaluation panel * improve confusion matrix legends * reverse y-axis range * update hovertemplate --------- Co-authored-by: brimoor <brimoor@umich.edu> Co-authored-by: Lanny W <lanzhenwang9@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx (1)
33-35: Consider adding documentation about the reversed y-axis behavior.The y-axis reversal is an important visual characteristic that should be documented. Consider adding a comment explaining why the y-axis is reversed and its relationship to confusion matrix visualization.
yaxis: { showgrid: true, zeroline: true, visible: true, zerolinecolor: theme.text.tertiary, color: theme.text.secondary, gridcolor: theme.primary.softBorder, automargin: true, // Enable automatic margin adjustment + // Reversed y-axis for proper confusion matrix visualization autorange: "reversed", },fiftyone/utils/eval/base.py (1)
362-365: Consider improving readability of the numpy delete operationsWhile the logic is correct, the nested
np.deleteoperations could be made more readable.Consider this alternative structure:
- ids = np.delete( - np.delete(ids, rm_inds, axis=0), rm_inds, axis=1 - ) + # Remove rows and columns for unused labels + ids = np.delete(ids, rm_inds, axis=0) # Remove rows + ids = np.delete(ids, rm_inds, axis=1) # Remove columnsapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
1158-1168: Consider extracting hover template to avoid code duplication.The hover template configuration is duplicated between the two confusion matrix plots. Consider extracting it into a reusable constant or function to improve maintainability.
+const getHoverTemplate = (evaluation) => [ + "<b>count: %{z:d}</b>", + `${evaluation?.info?.config?.gt_field || "truth"}: %{y}`, + `${evaluation?.info?.config?.pred_field || "predicted"}: %{x}`, +].join(" <br>") + "<extra></extra>"; // Then use it in both places: -hovertemplate: [ - "<b>count: %{z:d}</b>", - `${evaluation?.info?.config?.gt_field || "truth"}: %{y}`, - `${evaluation?.info?.config?.pred_field || "predicted"}: %{x}`, -].join(" <br>") + "<extra></extra>", +hovertemplate: getHoverTemplate(evaluation), // And in the compare section: -hovertemplate: [ - "<b>count: %{z:d}</b>", - `${evaluation?.info?.config?.gt_field || "truth"}: %{y}`, - `${evaluation?.info?.config?.pred_field || "predicted"}: %{x}`, -].join(" <br>") + "<extra></extra>", +hovertemplate: getHoverTemplate(compareEvaluation),Also applies to: 1197-1207
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx(1 hunks)fiftyone/core/plots/plotly.py(7 hunks)fiftyone/utils/eval/base.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (7)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx (1)
34-34: Verify the impact of y-axis reversal on existing visualizations.
The change from scaleanchor: "x" to autorange: "reversed" improves confusion matrix visualization but may affect other plots using this component.
fiftyone/utils/eval/base.py (2)
336-337: LGTM: Proper conditional handling of ids array
The code correctly guards the ids array modification with the tabulate_ids check, preventing potential errors when ID tabulation is disabled.
344-346: LGTM: Consistent handling of edge cases
The code maintains consistency in handling the ids array for the intersection of "other" and "missing" labels, with proper conditional checks.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
1557-1558: LGTM! Consistent slicing implementation.
The matrix slicing implementation correctly handles the limit parameter by applying it consistently to both the classes and matrix arrays.
fiftyone/core/plots/plotly.py (3)
106-107: LGTM: Parameters added to improve hover information.
The addition of gt_field and pred_field parameters enhances the customization of hover tooltips in confusion matrix plots.
130-131: LGTM: Clean implementation of hover template customization.
The changes effectively:
- Add optional parameters with proper defaults
- Use the field names to customize the hover template
- Maintain backward compatibility
Also applies to: 137-138, 211-212
1972-1975: LGTM: Clean addition of field name customization to InteractiveHeatmap.
The changes properly integrate field name customization into the InteractiveHeatmap class, following existing patterns and maintaining clean code organization.
Also applies to: 2010-2011
[Docs] Treat delegated execution separately
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
docs/source/plugins/using_plugins.rst (2)
597-598: Consider enhancing the metadata computation exampleWhile the example is correct, it would be more helpful to add a brief comment explaining what metadata is being computed and why one might want to overwrite it.
# (Re)compute the dataset's metadata +# This computes basic metadata like image dimensions, file sizes, etc. +# Use overwrite=True to force recomputation of existing metadata compute_metadata(dataset, overwrite=True)
656-695: Enhance the delegation documentation with more contextThe section effectively explains the mechanics but could benefit from explaining the practical benefits of delegation.
Requesting delegation ~~~~~~~~~~~~~~~~~~~~~ + +Delegation allows you to offload computationally intensive tasks to a separate process, +preventing the main application from becoming unresponsive during long-running operations. + Operators that support :ref:`delegated execution <delegated-operations>` canfiftyone/operators/executor.py (1)
264-270: LGTM! Consider enhancing error logging.The added null check and error handling for inputs schema serialization is a good improvement. It prevents potential null pointer exceptions and gracefully handles serialization errors.
Consider including the error type in the warning message for better debugging:
- f"Failed to resolve inputs schema for the operation: {str(e)}" + f"Failed to resolve inputs schema for the operation: {type(e).__name__}: {str(e)}"docs/source/plugins/developing_plugins.rst (2)
Line range hint
401-1144: Comprehensive examples with room for enhancementThe examples section provides good coverage of different plugin types with clear code samples. However, consider adding more inline comments within the code examples to explain specific implementation choices.
Consider adding more inline comments in code examples like this:
def execute(self, ctx): # Get the brain key from the current panel state brain_key = ctx.panel.state.menu.actions.brain_key if brain_key is None: return # Load and process the brain results results = ctx.dataset.load_brain_results(brain_key) # Format the data for the plot data = {"points": results.points, ...} # Store as panel data for efficient reuse ctx.panel.set_data("embeddings", data)
Line range hint
1244-2500: Comprehensive panel development guide with excellent patternsThe panel development section is thorough and well-organized. The common patterns section is particularly valuable, providing clear examples of state management, callbacks, and interactive components.
Consider adding a troubleshooting section that addresses common issues developers might encounter when building panels, such as:
- State management pitfalls
- Common callback mistakes
- Performance optimization tips
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
docs/source/plugins/developing_plugins.rst(3 hunks)docs/source/plugins/using_plugins.rst(5 hunks)fiftyone/operators/executor.py(1 hunks)
🔇 Additional comments (4)
docs/source/plugins/using_plugins.rst (1)
929-929: LGTM: Environment configuration note
The addition of the environment configuration note is clear and helpful.
docs/source/plugins/developing_plugins.rst (3)
Line range hint 1-145: Well-structured introduction to plugin development concepts
The introduction and overview sections provide a clear and comprehensive explanation of plugin types and components. The documentation effectively explains the distinction between Python and JS plugins, and clearly outlines the roles of panels, operators, and components.
Line range hint 146-400: Clear setup instructions and configuration documentation
The development setup and plugin anatomy sections are well-documented with precise instructions. The fiftyone.yml configuration section is particularly thorough, with a clear table of available fields and good examples.
Line range hint 1145-1243: Clear explanation of execution options and delegation
The operator execution section effectively explains immediate vs delegated execution options. The progress reporting examples are particularly helpful.
| .. _delegating-function-calls: | ||
|
|
||
| Delegating function calls | ||
| ------------------------- | ||
|
|
||
| The | ||
| `@voxel51/utils/delegate <https://github.com/voxel51/fiftyone-plugins/tree/main/plugins/utils>`_ | ||
| operator provides a general purpose utility for | ||
| :ref:`delegating execution <delegated-operations>` of an arbitrary function | ||
| call that can be expressed in any of the following forms: | ||
|
|
||
| - Execute an arbitrary function: `fcn(*args, **kwargs)` | ||
| - Apply a function to a dataset or view: | ||
| `fcn(dataset_or_view, *args, **kwargs)` | ||
| - Call an instance method of a dataset or view: | ||
| `dataset_or_view.fcn(*args, **kwargs)` | ||
|
|
||
| Here's some examples of delegating common tasks that can be expressed in the | ||
| above forms: | ||
|
|
||
| .. code-block:: python | ||
| :linenos: | ||
|
|
||
| import fiftyone as fo | ||
| import fiftyone.operators as foo | ||
| import fiftyone.zoo as foz | ||
|
|
||
| dataset = foz.load_zoo_dataset("quickstart") | ||
| delegate = foo.get_operator("@voxel51/utils/delegate") | ||
|
|
||
| # Compute metadata | ||
| delegate("compute_metadata", dataset=dataset) | ||
|
|
||
| # Compute visualization | ||
| delegate( | ||
| "fiftyone.brain.compute_visualization", | ||
| dataset=dataset, | ||
| brain_key="img_viz", | ||
| ) | ||
|
|
||
| # Export a view | ||
| delegate( | ||
| "export", | ||
| view=dataset.to_patches("ground_truth"), | ||
| export_dir="/tmp/patches", | ||
| dataset_type="fiftyone.types.ImageClassificationDirectoryTree", | ||
| label_field="ground_truth", | ||
| ) | ||
|
|
||
| # Load the exported patches into a new dataset | ||
| delegate( | ||
| "fiftyone.Dataset.from_dir", | ||
| dataset_dir="/tmp/patches", | ||
| dataset_type="fiftyone.types.ImageClassificationDirectoryTree", | ||
| label_field="ground_truth", | ||
| name="patches", | ||
| persistent=True, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling guidance to function delegation examples
The function delegation examples are comprehensive but should include guidance on error handling.
# Load the exported patches into a new dataset
delegate(
"fiftyone.Dataset.from_dir",
dataset_dir="/tmp/patches",
dataset_type="fiftyone.types.ImageClassificationDirectoryTree",
label_field="ground_truth",
name="patches",
persistent=True,
+ # Note: Errors in delegated operations can be monitored using:
+ # fiftyone delegated list --state FAILED
)Committable suggestion skipped: line range outside the PR's diff.
* bugfix/yaxis-autorange * deep merge layout styles
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (2)
83-88: LGTM! Consider adding a list comprehension for better readabilityThe filtering logic to include only evaluations with results is correct and improves the user experience. However, the code could be more concise.
Consider using a list comprehension for better readability:
- if self.has_evaluation_results(ctx.dataset, key): - evaluation = { - "key": key, - "id": self.get_evaluation_id(ctx.dataset, key), - } - evaluations.append(evaluation) + evaluations = [ + {"key": key, "id": self.get_evaluation_id(ctx.dataset, key)} + for key in ctx.dataset.list_evaluations() + if self.has_evaluation_results(ctx.dataset, key) + ]
Line range hint
359-363: Add docstring to improve code documentationThe method implementation is correct, but adding a docstring would improve maintainability.
Consider adding documentation:
def has_evaluation_results(self, dataset, eval_key): + """Check if an evaluation has results. + + Args: + dataset: The FiftyOne dataset containing the evaluation + eval_key: The key identifying the evaluation + + Returns: + bool: Whether the evaluation has results + """ try: return bool(dataset._doc.evaluations[eval_key].results) except Exception: return False
[Docs] Leaky splits
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/source/brain.rst (5)
1775-1780: Consider enhancing the introduction with a clear definition of data leaks.While the introduction effectively explains why leaks are problematic, it would be helpful to explicitly define what constitutes a "leak" (e.g., exact duplicates, near-duplicates, or semantically equivalent samples) to help users better understand what the tool will identify.
1792-1811: Enhance code examples with more descriptive comments and type hints.The code examples would be more educational with:
- Type hints for the return values
- Example values for the split field
- More descriptive comments explaining each approach's use case
Consider updating the examples like this:
import fiftyone as fo import fiftyone.brain as fob dataset = fo.load_dataset(...) -# splits via tags +# Method 1: Define splits using dataset tags +# Useful when samples can belong to multiple splits split_tags = ['train', 'test'] index, leaks = fob.compute_leaky_splits(dataset, split_tags=split_tags) -# splits via field -split_field = ['split'] # holds split values e.g. 'train' or 'test' +# Method 2: Define splits using a field +# Useful when each sample belongs to exactly one split +split_field = 'split' # field containing values like 'train', 'test', 'validation' index, leaks = fob.compute_leaky_splits(dataset, split_field=split_field) -# splits via views +# Method 3: Define splits using dataset views +# Useful when splits are defined by complex criteria split_views = { 'train' : some_view 'test' : some_other_view } index, leaks = fob.compute_leaky_splits(dataset, split_views=split_views)
1833-1840: Add guidance on interpreting leak analysis results.The example would be more helpful if it included:
- What patterns to look for when reviewing leaks
- Common types of leaks found in real datasets
- Example output showing what leaks look like
1897-1905: Enhance threshold selection guidance.The documentation would benefit from:
- Typical threshold ranges that work well in practice
- Examples of how different thresholds affect results
- A methodology for finding the optimal threshold for a given dataset
1861-1862: Fix typographical errors.There are a few minor typos in this section:
- "Performance on the clean test set will can be closer" should be "Performance on the clean test set can be closer"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/source/images/brain/brain-leaky-splits.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (1)
docs/source/brain.rst(2 hunks)
add guards for missing location/dimensions while rendering 3d bounding boxes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/packages/looker-3d/src/labels/index.tsx (2)
138-142: LGTM! Consider enhancing type safety.The additional safety checks for
dimensionsandlocationproperties are good. However, consider using TypeScript type guards to improve type safety and maintainability.Consider this type-safe approach:
-if ( - overlay._cls === "Detection" && - overlay.dimensions && - overlay.location -) { +interface Detection { + _cls: "Detection"; + dimensions: { width: number; height: number; depth: number }; + location: { x: number; y: number; z: number }; +} + +function isValidDetection(overlay: OverlayLabel): overlay is Detection { + return ( + overlay._cls === "Detection" && + !!overlay.dimensions && + !!overlay.location + ); +} + +if (isValidDetection(overlay)) {
138-142: Consider memoizing the type check for better performance.Since this check is performed within a loop processing raw overlays, consider extracting the validation logic to a memoized function to optimize performance when dealing with large datasets.
Here's a suggested optimization:
const isValidOverlay = useCallback((overlay: OverlayLabel) => { if (overlay._cls === "Detection") { return !!overlay.dimensions && !!overlay.location; } if (overlay._cls === "Polyline") { return !!(overlay as unknown as PolyLineProps).points3d; } return false; }, []); // Then in the loop: if (overlay._cls === "Detection" && isValidOverlay(overlay)) { // ... rest of the code }app/packages/looker/src/worker/threed-label-processor.ts (2)
62-62: LGTM! Consistent validation across code paths.The null check aligns with the validation added for the Detections type above.
Consider extracting the validation logic into a helper function to maintain DRY principles:
+const isValidDetection = (detection: DetectionLabel): boolean => { + return Boolean(detection.location && detection.dimensions); +}; -if (cls === "Detection" && label.location && label.dimensions) { +if (cls === "Detection" && isValidDetection(label)) {
Line range hint
19-22: Consider implementing cache management.The
inferredParamsCacheis implemented as a global object without any cache invalidation strategy. This could lead to memory leaks in long-running applications as the cache grows unbounded with sample IDs.Consider:
- Implementing a cache size limit
- Adding cache invalidation for old entries
- Using a proper caching library with built-in LRU functionality
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/looker-3d/src/labels/index.tsx(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/worker/threed-label-processor.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker-3d/src/labels/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/detection.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/threed-label-processor.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/looker/src/worker/threed-label-processor.ts (1)
50-52: LGTM! Robust null-safety check added.
The added validation prevents runtime errors by skipping detections with missing location or dimensions.
Let's verify that all detection processing code paths have similar null checks:
✅ Verification successful
Let me gather more context about the detection processing to ensure we have a complete view.
Null checks are properly implemented throughout the codebase
After reviewing the complete file, the null checks for detection.location and detection.dimensions are consistently implemented in both code paths where these properties are accessed:
- Lines 50-52: In the parameters inference loop for
Detectionstype - Line 62: In the single
Detectiontype processing with an identical check
The validation is thorough and prevents any potential runtime errors from accessing undefined properties. No additional checks are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for detection processing code that might need similar null checks
ast-grep --pattern 'detection.$_' app/packages/looker/src
Length of output: 389
Script:
#!/bin/bash
# Get the full context of the threed-label-processor.ts file
cat app/packages/looker/src/worker/threed-label-processor.ts
Length of output: 6636
app/packages/looker/src/overlays/detection.ts (1)
91-94: LGTM! Improved robustness of 3D rendering condition.
The enhanced condition properly validates the existence of required 3D properties before attempting to render, preventing potential runtime errors. This defensive approach aligns well with the TypeScript interface definition where these properties are optional.
* Updated from Slack to Discord * Updated doc and logo data * Added discord png logo to doc Co-authored-by: Sashank Aryal <sashank2012@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/source/_static/images/icons/discord-logo-256px.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (10)
app/packages/app/src/components/Nav.tsx(2 hunks)app/packages/app/src/components/Setup.tsx(2 hunks)app/packages/components/src/components/Icons/Icons.tsx(1 hunks)app/packages/components/src/components/Icons/index.ts(1 hunks)docs/source/_templates/layout.html(2 hunks)docs/source/conf.py(1 hunks)docs/source/getting_started/troubleshooting.rst(1 hunks)docs/source/index.rst(2 hunks)docs/source/teams/overview.rst(1 hunks)fiftyone/core/session/session.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- app/packages/app/src/components/Nav.tsx
- app/packages/components/src/components/Icons/index.ts
- docs/source/conf.py
- docs/source/teams/overview.rst
- fiftyone/core/session/session.py
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/app/src/components/Setup.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/components/src/components/Icons/Icons.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (7)
app/packages/components/src/components/Icons/Icons.tsx (2)
11-25: LGTM! The DiscordLink component implementation follows best practices.
The component is well-structured and consistent with other link components in the file. It properly uses the MUI SvgIcon component and maintains consistent styling through the theme system.
14-15: Verify the Discord community URL.
The URL points to a generic community subdomain. Let's verify this is the correct and permanent Discord community URL.
✅ Verification successful
The Discord community URL is correct and consistently used across the codebase
The URL https://community.voxel51.com/ is the official Discord community URL for Voxel51, as evidenced by its consistent usage across multiple critical locations in the codebase:
- Documentation configuration and templates
- Core session files
- Multiple UI components
- Navigation and setup components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of Discord-related URLs to ensure consistency
rg -i "discord|community\.voxel51\.com" --type ts --type tsx
Length of output: 90
Script:
#!/bin/bash
# Search for Discord-related URLs with proper file extensions
rg -i "discord|community\.voxel51\.com" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts'
# Also check for any configuration files that might contain URLs
rg -i "discord|community\.voxel51\.com" --type json --type yaml
Length of output: 1739
app/packages/app/src/components/Setup.tsx (2)
5-5: LGTM! Import statement updated correctly.
The import statement has been properly updated to use DiscordLink from the shared components package.
193-193: LGTM! Component usage updated correctly.
The DiscordLink component is properly integrated within the header's icon container, maintaining consistency with other social links.
Let's verify the DiscordLink component implementation:
✅ Verification successful
Let me analyze the output to verify the DiscordLink component implementation.
The previous search didn't return the DiscordLink implementation. Let me try another pattern:
Let me try one more search to find the actual DiscordLink component implementation:
Let me check the @fiftyone/components package where the DiscordLink component is imported from:
Component verified - correctly implemented and integrated
The DiscordLink component is properly implemented in the @fiftyone/components package and correctly integrated in both Setup.tsx and Nav.tsx components. The component is a simple external link to the Discord community that renders with an icon, maintaining consistency with other social links in the header.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the DiscordLink component implementation and its usage across the codebase
# Check the DiscordLink component implementation
ast-grep --pattern 'export const DiscordLink = () => {
$$$
}'
# Check for any remaining SlackLink references that might have been missed
rg -i 'slack(link)?'
Length of output: 131437
Script:
#!/bin/bash
# Let's check for the DiscordLink component implementation and any remaining Slack references
ast-grep --pattern 'export const DiscordLink = ({$$$}) => {
$$$
}'
# Search for any remaining Slack references in components
rg -i "slack" app/packages/app/src/components/
Length of output: 117
Script:
# Try another pattern to find DiscordLink component
ast-grep --pattern 'const DiscordLink = {$$$}'
# Also check for any files containing DiscordLink
rg "DiscordLink" app/packages/app/src/components/
Length of output: 344
Script:
#!/bin/bash
# Search for the DiscordLink component implementation with a broader pattern
ast-grep --pattern 'DiscordLink = $$$'
# Also check the full content of files that reference DiscordLink
rg -A 10 -B 10 "DiscordLink" app/packages/app/src/components/
Length of output: 5172
Script:
#!/bin/bash
# Search for DiscordLink in the components package
rg -A 10 -B 10 "DiscordLink" app/packages/components/src/
# Also check for any exports of DiscordLink
rg "export.*DiscordLink" app/packages/components/src/
Length of output: 2763
docs/source/_templates/layout.html (1)
57-57: LGTM: Learn menu Discord link update
Clean replacement of Slack with Discord in the Learn dropdown menu.
docs/source/index.rst (2)
17-19: LGTM: Social links updated correctly for Discord migration
The changes appropriately update the community platform links from Slack to Discord, maintaining consistency in both the URL and visual elements.
475-475: LGTM: Support section updated for Discord migration
The support section has been correctly updated to reference the new Discord community platform, maintaining consistency with the social links section.
| @@ -9,7 +9,7 @@ This page lists common issues encountered when installing FiftyOne and possible | |||
| solutions. If you encounter an issue that this page doesn't help you resolve, | |||
| feel free to | |||
| `open an issue on GitHub <https://github.com/voxel51/fiftyone/issues/new?labels=bug&template=installation_issue_template.md&title=%5BSETUP-BUG%5D>`_ | |||
| or `contact us on Slack <https://slack.voxel51.com>`_. | |||
| or `contact us on Discord <https://community.voxel51.com>`_. | |||
There was a problem hiding this comment.
💡 Codebase verification
Discord link needs to be updated to the direct Discord invite URL
The community.voxel51.com URL redirects to https://discord.gg/fiftyone-community. For better user experience and to avoid an unnecessary redirect, the documentation should use the direct Discord invite URL.
- Update line 12 in
docs/source/getting_started/troubleshooting.rstto usehttps://discord.gg/fiftyone-communityinstead ofhttps://community.voxel51.com
🔗 Analysis chain
LGTM! Let's verify the Discord link.
The change from Slack to Discord is consistent with the platform transition mentioned in the PR summary.
Let's verify the Discord community link is accessible:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Discord community link is accessible
# Expected: HTTP 200 OK response
curl -I https://community.voxel51.com
Length of output: 571
| @@ -140,7 +140,7 @@ | |||
| <a href="{{link_voxel51_blog}}">Blog</a> | |||
| <a href="{{link_docs_fiftyone}}">Docs</a> | |||
| <a href="{{link_voxel51_github}}">GitHub</a> | |||
| <a href="{{link_voxel51_slack}}">Slack</a> | |||
| <a href="{{link_voxel51_discord}}">Discord</a> | |||
There was a problem hiding this comment.
💡 Codebase verification
Additional updates needed for complete Slack to Discord migration
While the footer link has been updated to Discord, there are still remaining Slack references in other files that need to be addressed:
docs/source/conf.py: Contains an unused Slack link variabledocs/source/release-notes.rst: Contains a reference to an expired Slack link
Please update these remaining references to maintain consistency with the Discord migration.
🔗 Analysis chain
LGTM: Footer Discord link update
Clean replacement of Slack with Discord in the footer links section.
Let's verify the Discord link configuration:
Let me analyze the output to verify the Discord link configuration.
Let me check the content of the remaining files that contain Slack references.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Discord link configuration in conf.py and its usage
# Check if link_voxel51_discord is defined in conf.py
rg "link_voxel51_discord" docs/source/conf.py
# Check for any remaining Slack references that might need updating
rg -i "slack" docs/source/
Length of output: 131388
Script:
#!/bin/bash
# Check for any remaining Slack references in the layout template
rg "slack" docs/source/_templates/layout.html
# Check for any remaining Slack references in other files that might need updating
rg -l "slack" docs/source/
Length of output: 122
Script:
#!/bin/bash
# Check the content of conf.py and release-notes.rst for Slack references
rg -A 2 -B 2 "slack" docs/source/conf.py docs/source/release-notes.rst
Length of output: 728
* make icon in panel table have circle background on hover * cleanup * fix icon
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1)
98-121: Consider consolidating styles and improving type safetyThe conditional rendering logic is well-implemented, but there are opportunities for improvement:
- The sx prop spreads could be consolidated to reduce repetition
- The size prop could benefit from stricter typing with the SizeType
Consider refactoring to:
+ const commonSx = { + color: resolvedColor, + }; + + const buttonSx = { + ...commonSx, + padding: size === "small" ? 0 : undefined, + minWidth: size === "small" ? 40 : undefined, + }; label ? ( <Button variant={variant} startIcon={Icon} onClick={handleClick} - sx={{ - color: resolvedColor, - padding: size === "small" ? 0 : undefined, - minWidth: size === "small" ? 40 : undefined, - }} + sx={buttonSx} > {label} </Button> ) : ( <IconButton onClick={handleClick} size={size} - sx={{ - color: resolvedColor, - }} + sx={commonSx} > {Icon} </IconButton> )app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
232-232: Consider extracting the minHeight value as a constant.While the Box component effectively maintains consistent row heights, the "32px" value would be better managed as a named constant at the component level or in a theme configuration.
+ const ACTION_CELL_MIN_HEIGHT = '32px'; // ... rest of the component code - <Box sx={{ minHeight: "32px" }} /> + <Box sx={{ minHeight: ACTION_CELL_MIN_HEIGHT }} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1)
81-86: LGTM! Props formatting improves readability
The multi-line formatting of the MuiIconFont component's props enhances code readability while maintaining proper TypeScript typing.
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
225-231: LGTM! Clean conditional rendering implementation.
The ternary operator provides a clear and concise way to handle the conditional rendering of the ActionsMenu component.
Merge
release/v1.1.0todevelopSummary by CodeRabbit
Release Notes
New Features
Evaluationcomponent with hover templates for confusion matrices, providing detailed prediction insights.compute_leaky_splits()method for analyzing dataset splits, improving model evaluation accuracy.DiscordLinkcomponent, replacingSlackLinkacross various components for community engagement.Bug Fixes
Documentation