Cherry pick builtin use lists #5423
Conversation
WalkthroughThe pull request introduces modifications across three files: Changes
Sequence DiagramsequenceDiagram
participant User
participant Operators
participant Dataset
participant ODM
User->>Operators: Select field operations
Operators->>Dataset: Process field requests
Dataset->>ODM: Filter and validate paths
ODM-->>Dataset: Return filtered paths
Dataset-->>Operators: Execute field operations
Operators-->>User: Display results
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
plugins/operators/__init__.py (5)
259-261: Refactor repeated validation logic into a helper functionThe validation code that checks if
field_nameexists inschemaand handles the error messaging is repeated multiple times across the codebase. Consider refactoring this repeated logic into a helper function to improve maintainability and reduce code duplication.Here’s how you might refactor the repeated code:
def _validate_field_name(field_name, schema, field_prop, field_type): if field_name is None: return True if field_name not in schema and "." not in field_name: field_prop.invalid = True field_prop.error_message = f"{field_type} field '{field_name}' does not exist" return True return FalseAnd replace the repeated blocks with:
if _validate_field_name(field_name, schema, field_prop, "Sample"): returnAlso applies to: 377-379, 454-456
647-647: Simplify dictionary iteration by removing.keys()In the line
for key in schema.keys():, it's more Pythonic and efficient to iterate directly over the dictionary keys usingfor key in schema:.Apply this diff to simplify the code:
-for key in schema.keys(): +for key in schema:🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
755-755: Simplify dictionary iteration by removing.keys()Similarly, in this loop, you can iterate directly over the dictionary:
Apply this diff:
-for key in schema.keys(): +for key in schema:🧰 Tools
🪛 Ruff (0.8.2)
755-755: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
907-907: Simplify dictionary iteration by removing.keys()Consider removing
.keys()to iterate over the dictionary keys directly:Apply this diff:
-for key in schema.keys(): +for key in schema:🧰 Tools
🪛 Ruff (0.8.2)
907-907: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
982-982: Simplify dictionary iteration by removing.keys()It's more concise to iterate over the dictionary without
.keys():Apply this diff:
-for key in schema.keys(): +for key in schema:🧰 Tools
🪛 Ruff (0.8.2)
982-982: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
fiftyone/core/odm/mixins.py (1)
1872-1878: Optimize_remove_nested_pathsfunction for scalabilityThe
_remove_nested_pathsfunction uses a nested loop, which can lead to performance issues with large lists of paths. Consider optimizing the function to improve efficiency.Here's an optimized version using a set for faster lookups:
-def _remove_nested_paths(paths): - return [ - path - for path in paths - if not any(path.startswith(p + ".") for p in paths) - ] +def _remove_nested_paths(paths): + path_set = set(paths) + result = [] + for path in paths: + parent = path + while '.' in parent: + parent = parent.rsplit('.', 1)[0] + if parent in path_set: + break + else: + result.append(path) + return resultfiftyone/core/dataset.py (1)
1967-1968: Improved list field handling in summary field creation.The change to iterate and unwind all list fields is a significant improvement over only unwinding the first list field. This ensures proper handling of multiple list fields when creating summary fields.
Consider adding a comment explaining that all list fields must be unwound to properly handle cases where samples contain multiple list fields that need to be summarized.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fiftyone/core/dataset.py(1 hunks)fiftyone/core/odm/mixins.py(4 hunks)plugins/operators/__init__.py(20 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
plugins/operators/__init__.py
647-647: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
755-755: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
907-907: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
982-982: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test-app
- GitHub Check: build / build
Cherry pick #5379 for release.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes