Make text pe models in ultralytics integration download to FO zoo directory #5986
Conversation
WalkthroughSupport for Ultralytics text model integration in FiftyOne is introduced, with logic to select and initialize CLIP and MobileCLIP text encoders based on the Ultralytics version. A new internal module provides adapter classes and a factory function to load these models from the FiftyOne model zoo directory, ensuring compatibility and proper device placement. Changes
Estimated code review effort2 (~20 minutes) Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
manushreegangwar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
fiftyone/utils/ultralytics_internal.py (2)
31-48: Consider documenting the unusedsizeparameter for MobileCLIPTS.The
sizeparameter extracted from the variant string is not used when instantiatingUltralyticsMobileCLIPTS(line 44). While this appears intentional based on the class implementation, it would be helpful to document this behavior.Consider adding a comment to clarify:
if version.parse(ultralytics.__version__) < version.parse("8.3.118"): return UltralyticsMobileCLIP(size, device) else: + # size parameter is not used for MobileCLIPTS variant return UltralyticsMobileCLIPTS(device)
95-113: Add missing closing newline.The file is missing a final newline character at the end.
Add a newline at the end of line 113.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/utils/ultralytics.py(3 hunks)fiftyone/utils/ultralytics_internal.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: swheaton
PR: voxel51/fiftyone#0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Learnt from: swheaton
PR: voxel51/fiftyone#0
File: :0-0
Timestamp: 2024-10-17T01:03:25.080Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Learnt from: imanjra
PR: voxel51/fiftyone#6057
File: plugins/panels/model_evaluation/__init__.py:44-44
Timestamp: 2025-06-25T15:59:07.326Z
Learning: In the FiftyOne model evaluation panel, the CACHE_VERSION constant uses its own versioning scheme (starting from "v1.0.0") instead of being tied to the FiftyOne release version, allowing for independent cache invalidation control.
fiftyone/utils/ultralytics_internal.py (4)
Learnt from: swheaton
PR: voxel51/fiftyone#4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-10-08T15:36:21.356Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: voxel51/fiftyone#4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: voxel51/fiftyone#4885
File: fiftyone/core/clips.py:77-85
Timestamp: 2024-10-04T15:40:07.346Z
Learning: The `ClipsView` class in `fiftyone/core/clips.py` intentionally uses double underscores in attribute names like `__stages`, `__media_type`, and `__name`.
Learnt from: swheaton
PR: voxel51/fiftyone#4885
File: fiftyone/core/clips.py:77-85
Timestamp: 2024-10-08T15:36:21.356Z
Learning: The `ClipsView` class in `fiftyone/core/clips.py` intentionally uses double underscores in attribute names like `__stages`, `__media_type`, and `__name`.
🧬 Code Graph Analysis (1)
fiftyone/utils/ultralytics.py (1)
fiftyone/utils/ultralytics_internal.py (1)
build_text_model(31-48)
⏰ 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: build
🔇 Additional comments (4)
fiftyone/utils/ultralytics_internal.py (2)
25-28: LGTM! Version-based class selection is implemented correctly.The conditional logic appropriately selects the base class based on the Ultralytics version, matching the PR objectives that mention MobileCLIPTS was introduced in version 8.3.118.
51-65: LGTM! Clean adapter implementation for CLIP models.The adapter correctly ensures CLIP models are downloaded to the FiftyOne model zoo directory and properly configures the model for inference.
fiftyone/utils/ultralytics.py (2)
505-524: LGTM! Well-structured conditional text model initialization.The implementation correctly:
- Checks for the availability of
build_text_modelbefore using it- Uses appropriate text model variants for different model types (MobileCLIP for YOLOE, standard CLIP for others)
- Properly handles embeddings for YOLOE models with caching enabled
933-944: LGTM! Clean version-aware helper function.The function properly handles the conditional import based on Ultralytics version, with clear documentation explaining its purpose. This aligns well with the PR objectives regarding version compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
fiftyone/utils/ultralytics_internal.py (1)
127-127: Add missing evaluation mode setup for consistency.The other adapter classes call
.eval()to set the model to evaluation mode. This should be added for consistency.self.tokenizer = clip.clip.tokenize self.device = device + self.eval()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/ultralytics_internal.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: swheaton
PR: voxel51/fiftyone#0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Learnt from: swheaton
PR: voxel51/fiftyone#0
File: :0-0
Timestamp: 2024-10-17T01:03:25.080Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Learnt from: imanjra
PR: voxel51/fiftyone#6057
File: plugins/panels/model_evaluation/__init__.py:44-44
Timestamp: 2025-06-25T15:59:07.326Z
Learning: In the FiftyOne model evaluation panel, the CACHE_VERSION constant uses its own versioning scheme (starting from "v1.0.0") instead of being tied to the FiftyOne release version, allowing for independent cache invalidation control.
fiftyone/utils/ultralytics_internal.py (4)
Learnt from: swheaton
PR: voxel51/fiftyone#4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-10-08T15:36:21.356Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: voxel51/fiftyone#4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: voxel51/fiftyone#4885
File: fiftyone/core/clips.py:77-85
Timestamp: 2024-10-04T15:40:07.346Z
Learning: The `ClipsView` class in `fiftyone/core/clips.py` intentionally uses double underscores in attribute names like `__stages`, `__media_type`, and `__name`.
Learnt from: swheaton
PR: voxel51/fiftyone#4885
File: fiftyone/core/clips.py:77-85
Timestamp: 2024-10-08T15:36:21.356Z
Learning: The `ClipsView` class in `fiftyone/core/clips.py` intentionally uses double underscores in attribute names like `__stages`, `__media_type`, and `__name`.
🪛 Ruff (0.11.9)
fiftyone/utils/ultralytics_internal.py
104-104: ultralytics.utils.downloads.attempt_download_asset imported but unused
Remove unused import: ultralytics.utils.downloads.attempt_download_asset
(F401)
⏰ 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). (12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (4)
fiftyone/utils/ultralytics_internal.py (4)
1-29: LGTM! Clean import structure and version handling.The import organization is well-structured with appropriate lazy imports for heavy dependencies and correct version-dependent base class selection for MobileCLIP variants.
31-49: LGTM! Well-designed factory function with proper error handling.The factory function correctly parses variant strings, handles version-dependent routing, and provides clear error messages for unsupported models.
51-65: LGTM! Proper CLIP adapter implementation.The adapter correctly inherits from the base class, ensures models are downloaded to the FiftyOne model zoo directory, and properly sets up device placement and evaluation mode.
67-94: LGTM! Proper MobileCLIP adapter with correct file path handling.The adapter correctly constructs file paths using the FiftyOne model zoo directory (addressing the previous review comment), handles downloads appropriately, and sets up the model correctly.
| filepath, | ||
| map_location=device, | ||
| ) | ||
| self.tokenizer = clip.clip.tokenize |
There was a problem hiding this comment.
Fix the tokenizer assignment.
The tokenizer should be accessed as clip.tokenize rather than clip.clip.tokenize.
- self.tokenizer = clip.clip.tokenize
+ self.tokenizer = clip.tokenize📝 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.
| self.tokenizer = clip.clip.tokenize | |
| self.tokenizer = clip.tokenize |
🤖 Prompt for AI Agents
In fiftyone/utils/ultralytics_internal.py at line 125, the tokenizer is
incorrectly assigned as clip.clip.tokenize. Change this assignment to use
clip.tokenize instead to correctly reference the tokenizer function.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fiftyone/utils/ultralytics_internal.py (1)
123-123: Fix the tokenizer assignment.The tokenizer should be accessed as
clip.tokenizerather thanclip.clip.tokenize.- self.tokenizer = clip.clip.tokenize + self.tokenizer = clip.tokenize
🧹 Nitpick comments (1)
fiftyone/utils/ultralytics_internal.py (1)
41-44: Document that size parameter is ignored for MobileCLIPTS.When
ultralytics.__version__ >= 8.3.118, thesizeparameter from the variant string is parsed but not passed toUltralyticsMobileCLIPTS. Consider adding a comment to clarify this behavior or validate that only specific sizes are supported.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/ultralytics_internal.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: swheaton
PR: voxel51/fiftyone#0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Learnt from: swheaton
PR: voxel51/fiftyone#0
File: :0-0
Timestamp: 2024-10-17T01:03:25.080Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Learnt from: imanjra
PR: voxel51/fiftyone#6057
File: plugins/panels/model_evaluation/__init__.py:44-44
Timestamp: 2025-06-25T15:59:07.326Z
Learning: In the FiftyOne model evaluation panel, the CACHE_VERSION constant uses its own versioning scheme (starting from "v1.0.0") instead of being tied to the FiftyOne release version, allowing for independent cache invalidation control.
fiftyone/utils/ultralytics_internal.py (4)
Learnt from: swheaton
PR: voxel51/fiftyone#4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-10-08T15:36:21.356Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: voxel51/fiftyone#4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: voxel51/fiftyone#4885
File: fiftyone/core/clips.py:77-85
Timestamp: 2024-10-04T15:40:07.346Z
Learning: The `ClipsView` class in `fiftyone/core/clips.py` intentionally uses double underscores in attribute names like `__stages`, `__media_type`, and `__name`.
Learnt from: swheaton
PR: voxel51/fiftyone#4885
File: fiftyone/core/clips.py:77-85
Timestamp: 2024-10-08T15:36:21.356Z
Learning: The `ClipsView` class in `fiftyone/core/clips.py` intentionally uses double underscores in attribute names like `__stages`, `__media_type`, and `__name`.
🧬 Code Graph Analysis (1)
fiftyone/utils/ultralytics_internal.py (1)
fiftyone/core/utils.py (2)
ensure_package(396-438)lazy_import(732-754)
⏰ 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). (12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/utils/ultralytics_internal.py (3)
1-23: Well-structured imports with appropriate lazy loading.The use of lazy imports for heavy dependencies like
clip,mobileclip, andtorchis a good practice to improve import performance.
51-65: Clean adapter implementation for CLIP models.The class properly ensures CLIP models are downloaded to FiftyOne's model zoo directory and correctly handles device placement.
73-94: Properly implemented with corrected file path handling.Good to see the file existence check has been fixed to use the model zoo directory path. The implementation correctly handles model downloading and initialization.
manushreegangwar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/utils/ultralytics.py (1)
937-947: Simplify the function by removing unnecessary else clause.The function logic is correct, but the else clause after return is unnecessary and can be simplified.
Apply this diff to improve code readability:
def _get_build_text_model(): """For ultralytics version >= 8.3.99, returns ``ultralytics_internal.build_text_model`` so that models are downloaded to ``fo.config.model_zoo_dir``. For other versions, returns None. """ if version.parse(ultralytics.__version__) >= version.parse("8.3.99"): from fiftyone.utils import ultralytics_internal return ultralytics_internal.build_text_model - else: - # For ultralytics version < 8.3.99, ultralytics.nn.text_model doesn't exist. - return None + # For ultralytics version < 8.3.99, ultralytics.nn.text_model doesn't exist. + return None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/ultralytics.py(3 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: imanjra
PR: voxel51/fiftyone#6057
File: plugins/panels/model_evaluation/__init__.py:44-44
Timestamp: 2025-06-25T15:59:07.326Z
Learning: In the FiftyOne model evaluation panel, the CACHE_VERSION constant uses its own versioning scheme (starting from "v1.0.0") instead of being tied to the FiftyOne release version, allowing for independent cache invalidation control.
fiftyone/utils/ultralytics.py (2)
Learnt from: swheaton
PR: #4651
File: fiftyone/public.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In fiftyone/__public__.py, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: #4651
File: fiftyone/public.py:116-116
Timestamp: 2024-10-08T15:36:21.356Z
Learning: In fiftyone/__public__.py, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
🧬 Code Graph Analysis (1)
fiftyone/utils/ultralytics.py (1)
fiftyone/utils/ultralytics_internal.py (1)
build_text_model(31-48)
🪛 Pylint (3.3.7)
fiftyone/utils/ultralytics.py
[refactor] 941-947: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: imanjra
PR: voxel51/fiftyone#6057
File: plugins/panels/model_evaluation/__init__.py:44-44
Timestamp: 2025-06-25T15:59:07.326Z
Learning: In the FiftyOne model evaluation panel, the CACHE_VERSION constant uses its own versioning scheme (starting from "v1.0.0") instead of being tied to the FiftyOne release version, allowing for independent cache invalidation control.
fiftyone/utils/ultralytics.py (2)
Learnt from: swheaton
PR: #4651
File: fiftyone/public.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In fiftyone/__public__.py, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Learnt from: swheaton
PR: #4651
File: fiftyone/public.py:116-116
Timestamp: 2024-10-08T15:36:21.356Z
Learning: In fiftyone/__public__.py, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
🧬 Code Graph Analysis (1)
fiftyone/utils/ultralytics.py (1)
fiftyone/utils/ultralytics_internal.py (1)
build_text_model(31-48)
🪛 Pylint (3.3.7)
fiftyone/utils/ultralytics.py
[refactor] 941-947: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ 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). (13)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/utils/ultralytics.py (3)
11-11: LGTM! Import is necessary for version comparison.The packaging.version import is correctly added to support semantic version comparison in the new helper function.
509-520: Validate model type checking and device placement logic.The conditional logic correctly:
- Gets the build_text_model function based on Ultralytics version
- Checks for YOLO-E model instances using hasattr to avoid AttributeError
- Uses appropriate text model variants (mobileclip for YOLO-E, clip for others)
- Properly passes device parameter for consistent device placement
- Handles text embeddings with caching for YOLO-E models
The implementation aligns well with the PR objectives to enable text model downloads to FiftyOne's model zoo directory.
509-526: Integration of ultralytics_internal.build_text_model VerifiedAll checks confirm that:
build_text_modelis defined infiftyone/utils/ultralytics_internal.py._get_build_text_model()inultralytics.pycorrectly imports and returnsultralytics_internal.build_text_modelfor Ultralytics ≥ 8.3.99 (otherwiseNone).- The snippet at lines 509–526 properly guards and invokes the text-model builder when available.
No further changes are needed.
What changes are proposed in this pull request?
This PR extracts code from ultralytics such that unlderlying text encoding models (clip, mobileclip) can be saved to model zoo directory for YOLO-E and YOLO-World
How is this patch tested? If it is not, please explain why.
Tested for
ultralyticsversions8.3.70, 8.3.99, 8.3.118, 8.3.162.Note that versions below
8.3.99don't haveultralytics.nn.text_modeland will not be impacted by this PR. Version8.3.118introducedultralytics.nn.text_model.MobileCLIPTSand that is the class used in the PR for>=8.3.118as opposed toultralytics.nn.text_model.MobileCLIPfor older versions.developand compare the outputs against this PRRelease Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes