Allow lists wherever possible in builtin operators #5379
Conversation
WalkthroughThe pull request introduces modifications across multiple files to enhance field handling and operator functionality. In Changes
Possibly related PRs
Suggested labels
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
|
![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
🧹 Nitpick comments (4)
plugins/operators/__init__.py (2)
259-261: Refactor duplicated validation logic into a helper functionThe validation code that checks whether a
field_nameexists inschemaand sets theinvalidanderror_messageproperties is repeated across multiple functions:
- Lines 259-261 in
_clone_sample_field_inputs- Lines 377-379 in
_clone_frame_field_inputs- Lines 454-456 in
_rename_sample_field_inputs- Lines 546-548 in
_rename_frame_field_inputs- Lines 661-664 in
_clear_sample_field_inputs- Lines 769-774 in
_clear_frame_field_inputs- Lines 921-924 in
_delete_sample_field_inputs- Lines 996-1001 in
_delete_frame_field_inputsConsider refactoring this repeated logic into a shared helper function to reduce code duplication and improve maintainability.
Also applies to: 377-379, 454-456, 546-548, 661-664, 769-774, 921-924, 996-1001
647-647: Simplify dictionary iteration by removing.keys()In Python, iterating over a dictionary or checking for membership automatically uses the keys, so explicitly calling
.keys()is unnecessary. This makes the code more concise and Pythonic.Lines where this applies:
Line 647:
- for key in schema.keys(): + for key in schema:Line 755:
- for key in schema.keys(): + for key in schema:Line 907:
- for key in schema.keys(): + for key in schema:Line 982:
- for key in schema.keys(): + for key in schema:Also applies to: 755-755, 907-907, 982-982
🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
fiftyone/core/odm/mixins.py (1)
1872-1878: Add a docstring to_remove_nested_pathsfor clarityConsider adding a docstring to the
_remove_nested_pathsfunction to explain its purpose and usage. This will improve code readability and assist other developers in understanding its functionality.fiftyone/core/dataset.py (1)
1939-1940: LGTM! Consider adding a comment explaining the list field unwinding logic.The change to unwind list fields one by one improves robustness compared to unwinding all fields at once. This is a good optimization.
Add a comment explaining why list fields need to be unwound individually:
+ # Unwind list fields one by one to avoid potential issues with multiple list fields for list_field in list_fields: pipeline.append({"$unwind": "$" + list_field})
📜 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 (6)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (1)
fiftyone/core/odm/mixins.py (1)
1872-1878: Approve addition of_remove_nested_pathsfunctionThe
_remove_nested_pathsfunction correctly filters out nested paths from the list, ensuring that only top-level fields are manipulated. This prevents unintended modifications to nested fields and enhances the robustness of field operations.
brimoor
force-pushed
the
builtin-use-lists2
branch
from
5c38f63 to
4a35af8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
plugins/operators/__init__.py (7)
247-261: LGTM! Field validation looks robust.The field validation logic is well-implemented, handling both direct and nested paths. The error messages are clear and consistent between sample and frame field handling.
Consider extracting the common validation logic into a helper function since the same pattern is used in both
_clone_sample_field_inputsand_clone_frame_field_inputs:def _validate_field_existence(field_name, schema, field_prop, is_frame=False): if field_name not in schema and "." not in field_name: field_prop.invalid = True prefix = "Frame field" if is_frame else "Field" field_prop.error_message = f"{prefix} '{field_name}' does not exist" return False return TrueAlso applies to: 365-379
644-670: Well-implemented multi-field clearing functionality!The implementation correctly handles:
- Multiple field selection with proper UI support
- Validation of field existence and read-only status
- Prevention of nested field selection conflicts
Consider caching the field validation results to avoid repeated database lookups when validating multiple fields:
def _validate_fields(dataset, field_names, schema, field_prop, is_frame=False): field_cache = {} for field_name in field_names: if field_name not in schema and "." not in field_name: field_prop.invalid = True prefix = "Frame field" if is_frame else "Field" field_prop.error_message = f"{prefix} '{field_name}' does not exist" return False if field_name not in field_cache: prefix = dataset._FRAMES_PREFIX if is_frame else "" field_cache[field_name] = dataset.get_field(prefix + field_name) field = field_cache[field_name] if field is not None and field.read_only: field_prop.invalid = True prefix = "Frame field" if is_frame else "Field" field_prop.error_message = f"{prefix} '{field_name}' is read-only" return False return TrueAlso applies to: 752-782
🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Line range hint
886-930: Code duplication between clear and delete operations.The field validation and selection logic is duplicated between clear and delete operations. While the implementation is correct, this introduces maintenance overhead.
Extract the common field validation and selection logic into shared utility functions:
def _setup_field_selector(schema, field_names, multiple=True): field_choices = types.AutocompleteView(multiple=multiple) for key in schema: if not any(key.startswith(f + ".") for f in field_names): field_choices.add_choice(key, label=key) return field_choices def _create_field_input(inputs, field_choices, label, description, required=True): return inputs.list( "field_names", types.String(), label=label, description=description, required=required, view=field_choices, )Also applies to: 952-1009
🧰 Tools
🪛 Ruff (0.8.2)
907-907: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
1619-1660: LGTM! Proper handling of current slice during multi-deletion.The implementation correctly handles the edge case of deleting the current slice by resetting to the default slice.
Consider using any() for a more concise current slice check:
-curr_slice = False -for name in names: - curr_slice |= name == ctx.group_slice - ctx.dataset.delete_group_slice(name) +curr_slice = any(name == ctx.group_slice for name in names) +for name in names: + ctx.dataset.delete_group_slice(name)
2184-2225: LGTM! Proper handling of current workspace during multi-deletion.The implementation correctly handles the edge case of deleting the current workspace by resetting to the default workspace.
For consistency with the suggested group slice improvement, consider using any() here as well:
-curr_spaces = False -for name in names: - curr_spaces |= name == ctx.spaces.name - ctx.dataset.delete_workspace(name) +curr_spaces = any(name == ctx.spaces.name for name in names) +for name in names: + ctx.dataset.delete_workspace(name)
647-647: Usekey in dictinstead ofkey in dict.keys()The
.keys()call is unnecessary when checking for key existence in a dictionary.Apply this change in the following locations:
- Line 647:
for key in schema:- Line 755:
for key in schema:- Line 907:
for key in schema:- Line 982:
for key in schema:Also applies to: 755-755, 907-907, 982-982
🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
1788-1790: Address the TODO comment about App bug.The comment indicates that there's a bug in the App preventing proper view setting functionality.
Would you like me to help investigate the App bug and open a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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 (5)
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build
🔇 Additional comments (3)
plugins/operators/__init__.py (3)
1124-1167: LGTM! Clean implementation of multi-index dropping.The implementation correctly:
- Filters out default indexes
- Provides a clear UI for multiple index selection
- Maintains consistency with other multi-selection operations
1443-1479: LGTM! Consistent implementation of multi-field deletion for summary fields.The implementation maintains consistency with other multi-field operations while properly handling summary field specifics.
1898-1934: LGTM! Clean implementation of multi-view deletion.The implementation maintains consistency with other multi-selection operations while properly handling saved views.
brimoor
force-pushed
the
builtin-use-lists2
branch
from
4a35af8 to
2047cfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/core/odm/mixins.py (1)
1872-1878: Add docstring and consider performance optimization.The function correctly filters out nested paths, but could benefit from:
- A docstring explaining its purpose and parameters
- Type hints for better code maintainability
- Performance optimization for large lists by using sets
+from typing import List, Set + -def _remove_nested_paths(paths): +def _remove_nested_paths(paths: List[str]) -> List[str]: + """Filters a list of paths to exclude any that are nested within other paths. + + Args: + paths: List of field paths + + Returns: + List of paths excluding any that are nested within other paths + + Example: + >>> _remove_nested_paths(['a', 'a.b', 'c']) + ['a', 'c'] + """ + # Convert to set for O(1) lookups + path_set: Set[str] = set(paths) return [ path - for path in paths - if not any(path.startswith(p + ".") for p in paths) + for path in paths + if not any( + path.startswith(p + ".") + for p in path_set + if p != path + ) ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/core/dataset.py(1 hunks)fiftyone/core/odm/mixins.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/core/dataset.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: modified-files
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/core/odm/mixins.py (3)
673-674: LGTM! Clean integration of nested path filtering.The integration prevents redundant operations by filtering out nested paths before clearing fields.
721-722: LGTM! Clean integration of nested path filtering.The integration prevents redundant operations by filtering out nested paths before deleting fields.
826-827: LGTM! Clean integration of nested path filtering.The integration prevents redundant operations by filtering out nested paths before removing dynamic fields.
brimoor
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugins/operators/__init__.py (4)
259-261: Consider extracting field validation logic into a helper function.The field existence validation logic is duplicated across multiple functions. Consider extracting it into a helper function to improve maintainability and reduce code duplication.
+def _validate_field_exists(field_prop, field_name, schema, is_frame_field=False): + if field_name not in schema and "." not in field_name: + field_prop.invalid = True + field_prop.error_message = ( + f"Frame field '{field_name}' does not exist" + if is_frame_field + else f"Field '{field_name}' does not exist" + ) + return False + return TrueAlso applies to: 377-379, 454-456, 546-548, 661-663, 769-773
1788-1790: TODO comment needs attention.The TODO comment indicates a bug in the App that needs to be fixed.
Would you like me to create a GitHub issue to track this App bug that prevents
ctx.ops.set_view(name=name)from working?
2193-2196: Consider usingany()for better readability and performance.The loop that checks if the current workspace is being deleted could be simplified using
any().-curr_spaces = False -for name in names: - curr_spaces |= name == ctx.spaces.name - ctx.dataset.delete_workspace(name) +curr_spaces = any(name == ctx.spaces.name for name in names) +for name in names: + ctx.dataset.delete_workspace(name)
647-647: Usekey in dictinstead ofkey in dict.keys().Using
key in dictis more idiomatic Python and slightly more efficient as it avoids creating a view object.-for key in schema.keys(): +for key in schema:Also applies to: 755-755, 907-907, 982-982
🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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 (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: build / changes
- GitHub Check: build
🔇 Additional comments (1)
plugins/operators/__init__.py (1)
646-649: Good use ofallow_duplicates=Falseand nested field handling.The implementation correctly prevents:
- Duplicate field selection using
allow_duplicates=False- Selection of nested fields using the
startswithcheck🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Change log
list()to allow users to act on multiple fields/entities wherever it makes sense