Skip to content
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

Release v1.0.2 cherry picks #4991

Open
wants to merge 40 commits into
base: release/v1.0.2
Choose a base branch
from
Open

Conversation

brimoor
Copy link
Contributor

@brimoor brimoor commented Oct 25, 2024

Cherry-picks the following for release

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced TimelineView for rendering timelines with enhanced configuration and error handling.
    • Added support for new YOLOv11 models in the Ultralytics integration.
    • Enhanced model documentation for Hugging Face, including new depth estimation models.
    • Added UltralyticsOutputProcessor class for improved model output processing.
  • Bug Fixes

    • Fixed overlay z-index issues for improved panel rendering.
    • Resolved concurrency issues during data downloads from cloud buckets.
    • Addressed a bug where the timeline name was not forwarded in seek utilities.
  • Documentation

    • Updated integration documentation for clarity and accuracy, including details on loading datasets and models.
    • Added new examples for handling datasets with multiple media fields.
    • Improved documentation for remote model sources and plugin development guidelines.
  • Chores

    • Version updated from 1.0.1 to 1.0.2.

ritch and others added 7 commits October 24, 2024 23:59
Copy link
Contributor

coderabbitai bot commented Oct 25, 2024

Walkthrough

The changes in this pull request involve multiple updates across various components in the FiftyOne framework. Key modifications include enhancements to the ContainerizedComponent, FrameLoaderView, PlotlyView, and the introduction of a new TimelineView. The updates focus on improving event handling, rendering logic, and user interaction capabilities. Additionally, ESLint configurations have been refined, and documentation has been updated to reflect new features and changes in model handling. Overall, these changes aim to enhance the functionality and usability of the application.

Changes

File Path Change Summary
app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx Added import for lodash's has function; introduced hasOverlay boolean to manage overlay z-index in rendering logic.
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx Added FRAME_LOADED_EVENT constant; modified event dispatching logic using frameDataRef for better frame handling.
app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx Removed useResizeHandler prop; updated event handling logic for various plot types; added memoization for layout and configuration.
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx Introduced new TimelineView component for rendering timelines using FiftyOne playback library.
app/packages/core/src/plugins/SchemaIO/components/index.ts Added exports for TimelineView; removed export for FrameLoaderView.
app/packages/playback/eslint.config.mjs Updated ESLint configuration to include hooksPlugin for React Hooks linting.
app/packages/playback/src/views/Timeline.tsx Refactored props handling; updated dependencies for seeking functionality.
docs/.gitignore Updated ignored file path for model documentation.
docs/scripts/make_model_zoo_docs.py Adjusted model application logic and output file path for documentation.
docs/source/integrations/huggingface.rst Updated documentation to include new models and corrected dataset references.
docs/source/integrations/openclip.rst Clarified model evaluation mode in OpenCLIP integration documentation.
docs/source/integrations/ultralytics.rst Added support for YOLOv11 models in documentation; updated code snippets.
docs/source/model_zoo/remote.rst Enhanced documentation for remotely-sourced zoo models and metadata fields.
docs/source/plugins/developing_plugins.rst Expanded guidelines for plugin development, including execution context properties.
docs/source/release-notes.rst Added release notes for FiftyOne Teams version 2.1.2 and FiftyOne version 1.0.2.
fiftyone/operators/executor.py Modified ExecutionContext class and resolve_type_with_context function for better user information access.
fiftyone/operators/types.py Added new TimelineView class for animation playback.
fiftyone/utils/open_clip.py Updated model loading and inference handling in TorchOpenClipModel class.
fiftyone/utils/ultralytics.py Enhanced model conversion and output processing for Ultralytics models; added UltralyticsOutputProcessor class.
setup.py Updated version number from 1.0.1 to 1.0.2.

Possibly related PRs

Suggested labels

documentation, enhancement, bug

Suggested reviewers

  • findtopher

🐰 In the meadow, changes bloom,
Components dance, dispelling gloom.
Overlays rise with z-index flair,
Timelines weave through the air.
With each update, we hop and play,
Celebrating code in a joyful way! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

🧹 Outside diff range and nitpick comments (10)
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1)

1-5: Add type definition for DEFAULT_CONFIG

Consider adding a type definition for better type safety and documentation:

+type TimelineConfig = {
+  loop: boolean;
+};

-const DEFAULT_CONFIG = { loop: false };
+const DEFAULT_CONFIG: TimelineConfig = { loop: false };
app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (1)

26-26: Consider using a more explicit boolean check

While the double bang (!!) operator works, consider using a more explicit boolean check for better code readability.

-const hasOverlay = !!schema?.view?.overlay;
+const hasOverlay = Boolean(schema?.view?.overlay);
app/packages/core/src/plugins/SchemaIO/components/index.ts (1)

Line range hint 1-54: Maintain documentation for exported components.

Consider adding JSDoc comments for the newly exported components to maintain consistency with TypeScript best practices.

Add documentation above each new export:

+/** Modal component for displaying content in a dialog */
 export { default as ModalView } from "./ModalView";

+/** Text display component */
 export { default as TextView } from "./TextView";

+/** Timeline visualization component */
 export { default as TimelineView } from "./TimelineView";

+/** Toast notification component */
 export { default as ToastView } from "./ToastView";
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (1)

13-14: Consider adding explicit type for the event name constant

The code looks good! The event constant and ref are well-placed. For better type safety, consider defining the event name as a string literal type.

-const FRAME_LOADED_EVENT = "frames-loaded";
+const FRAME_LOADED_EVENT: 'frames-loaded' = "frames-loaded";

Also applies to: 24-24

app/packages/playback/src/views/Timeline.tsx (1)

29-31: Consider enhancing props interface documentation

While the props handling is correct, the interface could benefit from better documentation:

 interface TimelineProps {
+  /** Unique identifier for the timeline instance */
   name: TimelineName;
+  /** Custom styles for the timeline container */
   style?: React.CSSProperties;
+  /** Custom styles for the timeline controls section */
   controlsStyle?: React.CSSProperties;
 }
docs/scripts/make_model_zoo_docs.py (1)

150-168: LGTM! Consider adding inline documentation for the field mappings.

The model application logic for different model types is well-structured. The field mappings (label_field and prompt_field) are correctly configured for each model type.

Consider adding inline comments explaining the purpose of each field mapping to help users understand the expected input and output fields. For example:

# prompt_field: expects ground truth detections/keypoints as input
# label_field: stores the generated segmentation masks
docs/source/model_zoo/remote.rst (1)

280-290: Consider enhancing the metadata field descriptions.

While the table structure is consistent with the previous one, the descriptions for the new metadata fields could be more detailed:

  • For the name field, it would be helpful to specify the recommended naming format or provide an example.
  • For the url field, clarify if this should match the URL used for registration.
     | Field                            | Required? | Description                                                                               |
     +==================================+===========+===========================================================================================+
-    | `name`                           |           | A name for the remote model source                                                        |
+    | `name`                           |           | A human-readable name for the remote model source (e.g., "voxel51/openai-clip")          |
-    | `url`                            |           | The URL of the remote model source                                                        |
+    | `url`                            |           | The canonical URL of the remote model source (should match the registration URL)          |
fiftyone/operators/executor.py (1)

Line range hint 388-404: Improve parameter handling in resolve_type_with_context function.

The function signature change from target: str to target: None makes the parameter more flexible, but the implementation could be improved to handle edge cases better.

Consider this improvement:

-async def resolve_type_with_context(request_params, target=None):
+async def resolve_type_with_context(
+    request_params: dict,
+    target: Optional[str] = None
+) -> Optional[types.Property]:
     """Resolves the "inputs" or "outputs" schema of an operator with the given
     context.

     Args:
         request_params: a dictionary of request parameters
         target (None): the target schema ("inputs" or "outputs")

     Returns:
         the schema of "inputs" or "outputs"
         :class:`fiftyone.operators.types.Property` of an operator, or None
     """
-    computed_target = target or request_params.get("target", None)
+    computed_target = target if target is not None else request_params.get("target")
     computed_request_params = {**request_params, "target": computed_target}
     operator_uri = request_params.get("operator_uri", None)
     registry = OperatorRegistry()
     return await resolve_type(registry, operator_uri, computed_request_params)
docs/source/plugins/developing_plugins.rst (1)

984-988: LGTM! Consider enhancing the user context documentation.

The addition of ctx.user and ctx.user_request_token properties is well documented. These properties provide valuable user context and authentication information to plugin developers.

Consider adding a small code example demonstrating how to use these properties for access control or user-specific customization:

def execute(self, ctx):
    # Example: Restrict operation to admin users
    if ctx.user and ctx.user.role == "admin":
        # Perform admin-only operation
        pass
    else:
        raise ValueError("This operation requires admin privileges")
docs/source/release-notes.rst (1)

19-43: Consider adding more context to the release notes.

While the changes are well documented, consider adding:

  1. Brief description of the impact/benefit for major changes
  2. Migration steps if any changes are breaking
  3. Links to relevant documentation for new features
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1f5eaa8 and 586159b.

⛔ Files ignored due to path filters (3)
  • app/packages/playback/package.json is excluded by !**/*.json
  • app/yarn.lock is excluded by !**/yarn.lock, !**/*.lock, !**/*.lock
  • fiftyone/zoo/models/manifest-torch.json is excluded by !**/*.json
📒 Files selected for processing (20)
  • app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (2 hunks)
  • app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (3 hunks)
  • app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx (0 hunks)
  • app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1 hunks)
  • app/packages/core/src/plugins/SchemaIO/components/index.ts (2 hunks)
  • app/packages/playback/eslint.config.mjs (1 hunks)
  • app/packages/playback/src/views/Timeline.tsx (2 hunks)
  • docs/.gitignore (1 hunks)
  • docs/scripts/make_model_zoo_docs.py (3 hunks)
  • docs/source/integrations/huggingface.rst (19 hunks)
  • docs/source/integrations/openclip.rst (1 hunks)
  • docs/source/integrations/ultralytics.rst (4 hunks)
  • docs/source/model_zoo/remote.rst (1 hunks)
  • docs/source/plugins/developing_plugins.rst (3 hunks)
  • docs/source/release-notes.rst (1 hunks)
  • fiftyone/operators/executor.py (3 hunks)
  • fiftyone/operators/types.py (1 hunks)
  • fiftyone/utils/open_clip.py (2 hunks)
  • fiftyone/utils/ultralytics.py (0 hunks)
  • setup.py (1 hunks)
💤 Files with no reviewable changes (2)
  • app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx
  • fiftyone/utils/ultralytics.py
✅ Files skipped from review due to trivial changes (2)
  • docs/.gitignore
  • setup.py
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.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/FrameLoaderView.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/TimelineView.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/index.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/playback/src/views/Timeline.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 (18)
app/packages/playback/eslint.config.mjs (2)

1-2: LGTM! Necessary imports for React Hooks linting.

The imports are correctly structured and provide the required dependencies for the hooks configuration.


13-18: LGTM! Essential React Hooks linting configuration.

The configuration properly integrates the React Hooks ESLint plugin using the recommended ruleset, which will help catch common mistakes in hooks usage such as:

  • Breaking the rules of hooks
  • Missing dependencies in useEffect
  • Incorrect hooks ordering
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1)

2-2: Verify Timeline component integration

Let's verify the correct usage of the Timeline component from @fiftyone/playback:

Also applies to: 42-42

✅ Verification successful

Let me gather more information about the Timeline component's implementation and props to ensure it's being used correctly.


Timeline component is correctly integrated

The Timeline component from @fiftyone/playback is being used correctly in TimelineView.tsx. The implementation shows:

  • Proper import of required dependencies (Timeline, useCreateTimeline, useTimeline)
  • Correct usage of Timeline component with the required name prop
  • Appropriate integration with TimelineCreator that handles initialization and configuration

The component usage matches the implementation in the playback package, where Timeline expects:

  • Required name prop of type TimelineName
  • Optional style and controlsStyle props for customization
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Timeline component usage patterns and its available props
ast-grep --pattern 'import { $$$, Timeline, $$$ } from "@fiftyone/playback"'

# Search for other Timeline usages for consistency
rg -A 2 '<Timeline' 

Length of output: 2510


Script:

#!/bin/bash
# Check Timeline component implementation
ast-grep --pattern 'const Timeline = $_' 

# Check TimelineCreator implementation since it's used in TimelineView
ast-grep --pattern 'const TimelineCreator = $_'

# Look for Timeline props interface/type definition
rg -A 5 'interface.*Timeline.*Props|type.*Timeline.*Props'

Length of output: 13846

app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (1)

Line range hint 13-37: Implementation maintains good React practices

The changes to overlay handling are well-integrated into the existing component structure. The implementation properly handles composite views, maintains type safety, and follows React best practices. The conditional rendering and style application are implemented correctly.

app/packages/core/src/plugins/SchemaIO/components/index.ts (1)

Line range hint 1-54: Verify impact of FrameLoaderView removal.

The AI summary indicates that FrameLoaderView was removed. We should verify if there are any remaining references to this component that need to be updated.

#!/bin/bash
# Description: Search for any remaining references to FrameLoaderView

# Test: Look for any imports or usage of FrameLoaderView
rg "FrameLoaderView" --type ts --type tsx
fiftyone/utils/open_clip.py (1)

98-98: LGTM! Important addition for inference mode.

Setting the model to evaluation mode is crucial for correct inference behavior, particularly affecting dropout and batch normalization layers.

docs/source/integrations/openclip.rst (1)

91-95: LGTM! Clear and helpful note about model evaluation mode.

The note effectively communicates the model's behavior during inference, which is crucial for users to understand. This aligns well with the implementation in fiftyone/utils/open_clip.py.

docs/scripts/make_model_zoo_docs.py (1)

426-426: Verify documentation structure changes.

The output path has been moved from source/user_guide/model_zoo/models.rst to source/model_zoo/models.rst, which suggests a documentation restructuring.

Let's verify that this path change is consistently reflected across the documentation:

✅ Verification successful

Let me gather more information to verify if there are any other files that might need updates due to this path change.


The evidence shows that this documentation restructuring change is consistent and intentional:

  1. The old path reference in docs/scripts/make_model_zoo_docs.py is only in a docstring comment describing the old location
  2. The new directory structure docs/source/model_zoo/ exists and is properly referenced in the documentation index files
  3. The model_zoo directory appears to be a new top-level section in the documentation, alongside other sections like user_guide

Therefore, this path change appears to be part of an intentional documentation restructuring and is properly reflected across the codebase.

Documentation restructuring change is valid and consistent.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old path and verify the new path structure

# Check for any remaining references to the old path
echo "Checking for old path references..."
rg "user_guide/model_zoo/models" -t rst -t py

# Verify the existence of the new directory structure
echo "Verifying new directory structure..."
fd "model_zoo" -t d docs/source

Length of output: 349


Script:

#!/bin/bash
# Check for any references to model_zoo in RST files and Python scripts
echo "Checking for model_zoo references in documentation..."
rg "model_zoo" -t rst -t py -A 2 -B 2

# Check the content of the index files to understand the documentation structure
echo "Checking index files..."
fd "index.rst" docs/source -x cat {}

Length of output: 228253

docs/source/integrations/ultralytics.rst (4)

150-150: LGTM!

The YOLOv11 model zoo example follows the correct naming pattern and syntax.


193-197: LGTM!

The YOLOv11 segmentation model examples are correctly formatted and follow the established pattern.


223-223: LGTM!

The documentation text correctly includes YOLOv11 in the list of supported segmentation models.


229-243: LGTM!

The YOLOv11 segmentation model zoo examples are well-organized and follow the correct naming convention.

docs/source/model_zoo/remote.rst (1)

278-279: LGTM: Clear introduction of remote source metadata.

The addition of this section provides a logical separation between model-specific metadata and source-level metadata.

fiftyone/operators/executor.py (1)

485-485: LGTM: Explicit initialization of user attribute.

The explicit initialization of self.user = None improves code clarity and prevents potential attribute access issues.

docs/source/integrations/huggingface.rst (2)

405-412: LGTM: New depth estimation models added

The addition of Depth Anything and Depth Anything-V2 models enhances the monocular depth estimation capabilities. The code examples are clear and follow the established pattern.


1321-1321: LGTM: Dataset repository links updated

The repository links have been updated to reflect the current locations of datasets on the Hugging Face Hub. The changes maintain consistency and provide accurate references for users.

Also applies to: 1344-1344, 1566-1567, 1577-1577, 1591-1593, 1595-1597, 1599-1599, 1604-1604, 1615-1615, 1650-1650, 1683-1684, 1692-1692, 1702-1702, 1723-1723, 1732-1732, 1850-1850, 1859-1859, 1956-1956, 1965-1965

docs/source/release-notes.rst (2)

6-16: LGTM! Clear and well-structured release notes.

The release notes clearly document that this is a cherry-pick release and list all included PRs.


Line range hint 45-2824: Historical release notes preserved for reference.

These older release notes serve as valuable historical documentation.

Comment on lines +21 to +26
if (!timeline_name) {
throw new Error("Timeline name is required");
}
if (!finalConfig.totalFrames) {
throw new Error("Total frames is required");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling with descriptive messages

The error messages should be more descriptive and include available context. Also, consider wrapping the component in an error boundary.

-    throw new Error("Timeline name is required");
+    throw new Error("TimelineView: timeline_name is required in the view configuration");

-    throw new Error("Total frames is required");
+    throw new Error(`TimelineView: totalFrames is required in the configuration for timeline "${timeline_name}"`);

Consider implementing an error boundary:

class TimelineErrorBoundary extends React.Component<{ children: React.ReactNode }> {
  componentDidCatch(error: Error) {
    console.error("Timeline failed to render:", error);
    // Add your error reporting logic here
  }

  render() {
    return this.props.children;
  }
}

Comment on lines +7 to +10
export default function TimelineView(props: ViewPropsType) {
const { schema } = props;
const { view = {} } = schema;
const { timeline_name, loop, total_frames } = view;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve props type safety with specific destructuring

The view object's shape should be explicitly typed for better type safety:

+type TimelineViewData = {
+  timeline_name?: string;
+  loop?: boolean;
+  total_frames?: number;
+};

export default function TimelineView(props: ViewPropsType) {
  const { schema } = props;
-  const { view = {} } = schema;
+  const { view = {} as TimelineViewData } = schema;
  const { timeline_name, loop, total_frames } = view;
📝 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.

Suggested change
export default function TimelineView(props: ViewPropsType) {
const { schema } = props;
const { view = {} } = schema;
const { timeline_name, loop, total_frames } = view;
type TimelineViewData = {
timeline_name?: string;
loop?: boolean;
total_frames?: number;
};
export default function TimelineView(props: ViewPropsType) {
const { schema } = props;
const { view = {} as TimelineViewData } = schema;
const { timeline_name, loop, total_frames } = view;

Comment on lines +31 to +43
export const TimelineCreator = ({ timelineName, totalFrames, loop }) => {
const config = useMemo(() => ({ totalFrames, loop }), [totalFrames, loop]);
const { isTimelineInitialized } = useCreateTimeline({
name: timelineName,
config,
});

if (!isTimelineInitialized) {
return null;
}

return <Timeline name={timelineName} />;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add prop types and improve error handling for TimelineCreator

The component needs proper TypeScript types and better error/loading state handling:

-export const TimelineCreator = ({ timelineName, totalFrames, loop }) => {
+type TimelineCreatorProps = {
+  timelineName: string;
+  totalFrames: number;
+  loop: boolean;
+};

+export const TimelineCreator: React.FC<TimelineCreatorProps> = ({
+  timelineName,
+  totalFrames,
+  loop,
+}) => {
  const config = useMemo(() => ({ totalFrames, loop }), [totalFrames, loop]);
-  const { isTimelineInitialized } = useCreateTimeline({
+  const { isTimelineInitialized, error } = useCreateTimeline({
    name: timelineName,
    config,
  });

+  if (error) {
+    throw new Error(`Failed to initialize timeline: ${error.message}`);
+  }

  if (!isTimelineInitialized) {
-    return null;
+    return <div>Initializing timeline...</div>;
  }

  return <Timeline name={timelineName} />;
};
📝 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.

Suggested change
export const TimelineCreator = ({ timelineName, totalFrames, loop }) => {
const config = useMemo(() => ({ totalFrames, loop }), [totalFrames, loop]);
const { isTimelineInitialized } = useCreateTimeline({
name: timelineName,
config,
});
if (!isTimelineInitialized) {
return null;
}
return <Timeline name={timelineName} />;
};
type TimelineCreatorProps = {
timelineName: string;
totalFrames: number;
loop: boolean;
};
export const TimelineCreator: React.FC<TimelineCreatorProps> = ({
timelineName,
totalFrames,
loop,
}) => {
const config = useMemo(() => ({ totalFrames, loop }), [totalFrames, loop]);
const { isTimelineInitialized, error } = useCreateTimeline({
name: timelineName,
config,
});
if (error) {
throw new Error(`Failed to initialize timeline: ${error.message}`);
}
if (!isTimelineInitialized) {
return <div>Initializing timeline...</div>;
}
return <Timeline name={timelineName} />;
};

@@ -7,6 +7,7 @@ import {
overlayToSx,
} from "../utils";
import { ViewPropsType } from "../utils/types";
import { has } from "lodash";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unused lodash import

The has function from lodash is imported but never used in the code. This unnecessary import should be removed to maintain a clean codebase and optimize bundle size.

-import { has } from "lodash";
📝 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.

Suggested change
import { has } from "lodash";

Comment on lines +28 to +30
if (hasOverlay) {
sxForOverlay.zIndex = 999;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using a named constant for z-index value

The hardcoded z-index value of 999 should be defined as a named constant at the module level for better maintainability and to prevent potential layering conflicts.

+const OVERLAY_Z_INDEX = 999;

// Later in the code
 if (hasOverlay) {
-  sxForOverlay.zIndex = 999;
+  sxForOverlay.zIndex = OVERLAY_Z_INDEX;
 }
📝 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.

Suggested change
if (hasOverlay) {
sxForOverlay.zIndex = 999;
}
const OVERLAY_Z_INDEX = 999;
if (hasOverlay) {
sxForOverlay.zIndex = OVERLAY_Z_INDEX;
}

Comment on lines +36 to +38
const { getSeekValue, seekTo } = useTimelineVizUtils(name);

const seekBarValue = React.useMemo(() => getSeekValue(), [frameNumber]);
const seekBarValue = React.useMemo(() => getSeekValue(), [getSeekValue]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Memoization dependencies are incomplete

The seekBarValue memoization only depends on getSeekValue, but getSeekValue itself depends on frameNumber and config.totalFrames. Since these dependencies can change independently of the getSeekValue function reference, they should be included in the useMemo dependency array:

const seekBarValue = React.useMemo(() => getSeekValue(), [getSeekValue, frameNumber, config.totalFrames]);
🔗 Analysis chain

Verify seekBarValue memoization dependencies

The memoization depends only on getSeekValue, but consider if there are other dependencies that could affect the seek value calculation.


Let me gather more information about the implementation of useTimelineVizUtils to understand the dependencies.


Let me check the complete implementation of getSeekValue and its dependencies in useTimelineVizUtils.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to getSeekValue to understand its dependencies
ast-grep --pattern 'const { $$$, getSeekValue, $$$ } = useTimelineVizUtils($$$)'

Length of output: 196


Script:

#!/bin/bash
# Search for useTimelineVizUtils implementation
ast-grep --pattern 'function useTimelineVizUtils($_) {
  $$$
}'

# Also search for any file containing useTimelineVizUtils
rg "useTimelineVizUtils" -A 10

Length of output: 2753


Script:

#!/bin/bash
# Get the complete implementation of useTimelineVizUtils
rg -A 30 "export const useTimelineVizUtils = \(name\?: TimelineName\)" app/packages/playback/src/lib/use-timeline-viz-utils.ts

Length of output: 1019

@@ -144,7 +145,7 @@ def _predict_all(self, imgs):
if self._using_gpu:
imgs = imgs.cuda()

with torch.no_grad(), torch.cuda.amp.autocast():
with torch.no_grad(), torch.amp.autocast("cuda"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making the autocast device type dynamic.

The update to the newer torch.amp.autocast API is good, but we could make it more flexible by using the current device instead of hardcoding "cuda".

Consider this modification:

-        with torch.no_grad(), torch.amp.autocast("cuda"):
+        with torch.no_grad(), torch.amp.autocast(device_type=self.device.type):

This would support both CUDA and potential future device types while maintaining the same functionality.

Committable suggestion was skipped due to low confidence.

Comment on lines +108 to +114
# YOLOv11
# model = YOLO("yolo11n.pt)
# model = YOLO("yolo11s.pt)
# model = YOLO("yolo11m.pt)
# model = YOLO("yolo11l.pt)
# model = YOLO("yolo11x.pt)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix missing closing quotes in YOLOv11 model names

The model names are missing closing quotes which would cause syntax errors if uncommented.

Apply this diff to fix the syntax:

-    # model = YOLO("yolo11n.pt)
-    # model = YOLO("yolo11s.pt)
-    # model = YOLO("yolo11m.pt)
-    # model = YOLO("yolo11l.pt)
-    # model = YOLO("yolo11x.pt)
+    # model = YOLO("yolo11n.pt")
+    # model = YOLO("yolo11s.pt")
+    # model = YOLO("yolo11m.pt")
+    # model = YOLO("yolo11l.pt")
+    # model = YOLO("yolo11x.pt")
📝 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.

Suggested change
# YOLOv11
# model = YOLO("yolo11n.pt)
# model = YOLO("yolo11s.pt)
# model = YOLO("yolo11m.pt)
# model = YOLO("yolo11l.pt)
# model = YOLO("yolo11x.pt)
# YOLOv11
# model = YOLO("yolo11n.pt")
# model = YOLO("yolo11s.pt")
# model = YOLO("yolo11m.pt")
# model = YOLO("yolo11l.pt")
# model = YOLO("yolo11x.pt")

Comment on lines +651 to +657
@property
def user_request_token(self):
"""The request token authenticating the user executing the operation,
if known.
"""
return self.user._request_token if self.user else None

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation for user_request_token property.

The new property provides access to the user's request token, but it should validate the user object's structure before accessing _request_token.

Consider this improvement:

     @property
     def user_request_token(self):
         """The request token authenticating the user executing the operation,
         if known.
         """
-        return self.user._request_token if self.user else None
+        if not self.user:
+            return None
+        return getattr(self.user, '_request_token', None)
📝 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.

Suggested change
@property
def user_request_token(self):
"""The request token authenticating the user executing the operation,
if known.
"""
return self.user._request_token if self.user else None
@property
def user_request_token(self):
"""The request token authenticating the user executing the operation,
if known.
"""
if not self.user:
return None
return getattr(self.user, '_request_token', None)

Comment on lines +2416 to +2428
class TimelineView(View):
"""Represents a timeline for playing animations.

Args:
timeline_name (None): the name of the timeline
total_frames (None): the total number of frames in the timeline
loop (False): whether to loop the timeline
"""

def __init__(self, **kwargs):
super().__init__(**kwargs)


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider enhancing the TimelineView implementation.

While the class documentation is clear, the implementation could be improved in several ways:

  1. The class should store and expose the timeline-specific parameters (timeline_name, total_frames, loop) as instance attributes.
  2. The to_json() method should be overridden to include these parameters in the JSON representation.

Apply this diff to enhance the implementation:

 class TimelineView(View):
     """Represents a timeline for playing animations.
 
     Args:
         timeline_name (None): the name of the timeline
         total_frames (None): the total number of frames in the timeline
         loop (False): whether to loop the timeline
     """
 
     def __init__(self, **kwargs):
+        self.timeline_name = kwargs.get("timeline_name", None)
+        self.total_frames = kwargs.get("total_frames", None)
+        self.loop = kwargs.get("loop", False)
         super().__init__(**kwargs)
+
+    def to_json(self):
+        return {
+            **super().to_json(),
+            "timeline_name": self.timeline_name,
+            "total_frames": self.total_frames,
+            "loop": self.loop,
+        }
📝 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.

Suggested change
class TimelineView(View):
"""Represents a timeline for playing animations.
Args:
timeline_name (None): the name of the timeline
total_frames (None): the total number of frames in the timeline
loop (False): whether to loop the timeline
"""
def __init__(self, **kwargs):
super().__init__(**kwargs)
class TimelineView(View):
"""Represents a timeline for playing animations.
Args:
timeline_name (None): the name of the timeline
total_frames (None): the total number of frames in the timeline
loop (False): whether to loop the timeline
"""
def __init__(self, **kwargs):
self.timeline_name = kwargs.get("timeline_name", None)
self.total_frames = kwargs.get("total_frames", None)
self.loop = kwargs.get("loop", False)
super().__init__(**kwargs)
def to_json(self):
return {
**super().to_json(),
"timeline_name": self.timeline_name,
"total_frames": self.total_frames,
"loop": self.loop,
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (1)
fiftyone/core/odm/utils.py (1)

661-662: LGTM! Thread-safe iteration over sys.modules

The change to create a copy of sys.modules.values() before iteration is a good practice that prevents potential RuntimeError exceptions that could occur if modules are imported or unloaded during iteration.

This modification improves thread safety in scenarios where multiple components might be dynamically loading or unloading modules concurrently, which is particularly important in a framework like FiftyOne where dynamic class loading is a core feature.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 389557c and 3d92aab.

📒 Files selected for processing (2)
  • fiftyone/core/odm/utils.py (1 hunks)
  • requirements/common.txt (0 hunks)
💤 Files with no reviewable changes (1)
  • requirements/common.txt

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (3)
docs/source/release-notes.rst (3)

6-11: Update placeholder release date before final release

The release date is currently set to "November XX, 2024". This should be updated with the actual release date before publishing.


14-38: Update placeholder release date and standardize bullet point formatting

  1. The release date "November XX, 2024" needs to be updated before release
  2. Some bullet points end with periods while others don't - recommend standardizing the punctuation
- Added :ref:`SAM 2.1 <model-zoo-segment-anything-2.1-hiera-base-plus-image-torch>`
  to the :ref:`Model Zoo <model-zoo>`
  `#4979 <https://github.com/voxel51/fiftyone/pull/4979>`_
+ Added :ref:`SAM 2.1 <model-zoo-segment-anything-2.1-hiera-base-plus-image-torch>`
  to the :ref:`Model Zoo <model-zoo>`.
  `#4979 <https://github.com/voxel51/fiftyone/pull/4979>`_

40-46: Update placeholder release date

The release date "November XX, 2024" should be updated with the actual release date before publishing.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3d92aab and 54d0912.

📒 Files selected for processing (1)
  • docs/source/release-notes.rst (1 hunks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants