Handle shadowed plugins #5342
Handle shadowed plugins #5342
Conversation
WalkthroughThis pull request introduces enhancements to the FiftyOne plugin management system, focusing on improving the CLI interface and plugin handling. The changes primarily involve updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
brimoor
force-pushed
the
handle-shadowed-plugins
branch
from
d697f93 to
4a75ccf
Compare
![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 (1)
fiftyone/plugins/core.py (1)
73-75: **Simplify repeated 'all' → None pattern **The
shadowed == "all"check effectively aligns with patterns used forenabledandbuiltin. For clarity, consider refactoring repetitive checks into a small helper function if it appears in multiple areas.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/source/cli/index.rst(2 hunks)fiftyone/core/cli.py(3 hunks)fiftyone/plugins/core.py(15 hunks)fiftyone/plugins/definitions.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/core/cli.py
3676-3676: Do not use bare except
(E722)
fiftyone/plugins/core.py
82-82: Do not use bare except
(E722)
495-495: Use existing_plugins.get(plugin_name) instead of existing_plugins.get(plugin_name, None)
Replace existing_plugins.get(plugin_name, None) with existing_plugins.get(plugin_name)
(SIM910)
🔇 Additional comments (24)
fiftyone/plugins/core.py (12)
Line range hint
15-47: Add shadow_paths attribute to PluginPackage [Approved]The addition of
shadow_paths: Optional[list[str]]is well-structured. UsingOptionaland a default ofNonealigns with Python best practices for optional fields.
53-62: Enhance list_plugins to handle shadowed parameter [Approved]Introducing the new
shadowedparameter broadens the functionality to include or exclude shadowed plugins. The docstring updates at lines 61-62 also provide clear guidance on how to use this parameter.
77-83: **Exception handling on plugin parsing **A bare
exceptblock is used at line 82 for_load_plugin_definition. Although the log message at line 83 clarifies the failure, consider catching more specific exceptions (e.g.Exceptionor a narrower subset) to avoid masking unrelated errors.🧰 Tools
🪛 Ruff (0.8.2)
82-82: Do not use bare
except(E722)
Line range hint
152-173: Filtering plugins when listing downloaded, enabled, and disabled [Approved]The calls to
_list_plugins(...)with explicit parameters for builtin and shadowed are consistent with the new logic. Returning only the plugin names is a straightforward design.
176-190: New get_plugin signature [Approved]Allowing a plugin directory or a plugin name is a valuable extension. The docstring at lines 180-181 is concise, accurately describing the new usage.
251-256: Support skipping existing plugins [Approved]The introduction of
skipped_pluginscleanly handles duplicates that do not need to be overwritten. This approach helps keep the user informed of what was skipped.
492-505: Creation logic for new plugins [Approved]This block correctly prevents overwriting built-in plugins at lines 500-503. The approach to clearing out existing directories, then populating with new contents, is clear and consistent.
🧰 Tools
🪛 Ruff (0.8.2)
495-495: Use
existing_plugins.get(plugin_name)instead ofexisting_plugins.get(plugin_name, None)Replace
existing_plugins.get(plugin_name, None)withexisting_plugins.get(plugin_name)(SIM910)
Line range hint
579-602: Refactored _list_plugins to handle shadowed parameter [Approved]The introduction of
_handle_shadowedbefore filtering by enablement is a neat approach to ensure that built-in overrides take the correct precedence. The usage of a single pass to gather and then handle shadowed items is efficient.🧰 Tools
🪛 Ruff (0.8.2)
596-596: Do not use bare
except(E722)
614-633: _handle_shadowed logic [Approved]This function suitably updates the
existing_plugin.shadow_pathsfield. The return set elegantly accommodates hidden/shadowed plugins.
671-678: _get_plugin shadowed=False parameter [Approved]Ensuring that shadowed plugins are excluded by default is consistent with the typical usage scenario. The explicit
raise ValueErroron line 678 is correct for missing plugins.
681-689: Enhance _load_plugin_definition to capture shadow_paths [Approved]Loading the plugin definition with
shadow_pathsensures shadowed plugin directories are tracked consistently across the codebase.
Line range hint
299-326: **Safe overwrite logic for download_plugin **The logic around lines 306-310 correctly prevents overwriting built-in plugins. On line 299-300, usage of
existing_plugins.get(plugin.name, None)can be shortened toexisting_plugins.get(plugin.name)to address the static analysis hint (SIM910). Please confirm that making this refactor would not cause side effects.- existing_plugin = existing_plugins.get(plugin.name, None) + existing_plugin = existing_plugins.get(plugin.name)✅ Verification successful
Let me verify the safety of this refactoring by checking the usage of
existing_plugins.get()across the codebase.
Let me check how the
Nonevalue is used in the conditional logic.
The suggested refactoring to remove explicit
Noneis safe to applyThe code consistently checks for the existence of plugins using
if existing_plugin is not Noneafter callingexisting_plugins.get(). Sincedict.get()returnsNoneby default when the key is not found, explicitly passingNoneas the default value is redundant. The suggested refactoring won't change the behavior of the code.
fiftyone/plugins/core.py:299: Can safely change toexisting_plugins.get(plugin.name)🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar usage patterns of existing_plugins.get() rg "existing_plugins\.get\(" -A 2 -B 2 # Search for the definition and usage of existing_plugins ast-grep --pattern 'existing_plugins = $_'Length of output: 1507
Script:
#!/bin/bash # Get more context around the usage of existing_plugin rg "existing_plugin = existing_plugins\.get\(" -A 10 -B 2Length of output: 2557
fiftyone/plugins/definitions.py (4)
25-26: Clarify docstring for shadow_paths [Approved]These lines clearly describe that
shadow_pathsis a list of plugin directories that the plugin shadows. The docstring provides valuable context for consumers of this class.
31-34: Initialize shadow_paths in constructor [Approved]Introducing
shadow_pathsin the__init__method is straightforward and maintains backward compatibility by defaulting toNone.
52-56: New shadow_paths property [Approved]The property approach elegantly exposes the
_shadow_pathslist. Simple and effective.
Line range hint
227-242: from_disk method supports shadow_paths [Approved]Accepting
shadow_paths=Noneinfrom_diskensures consistent usage with the constructor. The dictionary merge and finalreturn cls(...)at line 242 is cleanly implemented.fiftyone/core/cli.py (7)
3599-3601: Request all plugins, including shadowed [Approved]Using
shadowed="all"at lines 3599-3601 ensures the CLI can provide a complete view of plugins, matching the new core logic.
3611-3615: Accumulating shadow_paths for CLI listing [Approved]Collecting
shadowed_pathsin a set is straightforward. Good approach to handle multiple shadowed sources.
3616-3623: Enrichment of output headers for plugin listing [Approved]Including
shadowedin the table columns at lines 3617-3621 is a clear, user-friendly addition.
3626-3634: Plugin enabling logic respects shadowed state [Approved]The line 3627 check
(pd.builtin or pd.name in enabled_plugins) and not shadowedproperly excludes shadowed plugins from being flagged as enabled.
3639-3641: Handle empty shadowed_paths [Approved]Removing the
shadowedcolumn from the CLI output if no shadowed paths exist prevents empty columns in the table. Nicely done.
3655-3658: Clarification of usage examples [Approved]These lines help users see that
fiftyone plugins info <dir>is now supported. Great addition to the usage example.
3662-3666: New argument name_or_dir [Approved]Updating the argument from
nametoname_or_diris a functional improvement. The docstring at lines 3664-3665 is clear.docs/source/cli/index.rst (1)
Line range hint
1242-1263: Document name_or_dir usage for plugins info [Approved]Renaming the parameter to
NAME_OR_DIRand adding an example with<dir>clarifies how to query plugins by directory path. This reflects the updated CLI functionality accurately.
brimoor
force-pushed
the
handle-shadowed-plugins
branch
from
4a75ccf to
8482229
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fiftyone/plugins/core.py (1)
495-495: Minor simplification: remove redundant default in.get()
Usingexisting_plugins.get(plugin_name)is simpler than including, None.Apply the following small diff:
- existing_plugin = existing_plugins.get(plugin_name, None) + existing_plugin = existing_plugins.get(plugin_name)🧰 Tools
🪛 Ruff (0.8.2)
495-495: Use
existing_plugins.get(plugin_name)instead ofexisting_plugins.get(plugin_name, None)Replace
existing_plugins.get(plugin_name, None)withexisting_plugins.get(plugin_name)(SIM910)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/source/cli/index.rst(2 hunks)fiftyone/core/cli.py(3 hunks)fiftyone/plugins/core.py(15 hunks)fiftyone/plugins/definitions.py(5 hunks)tests/unittests/plugins/core_tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/source/cli/index.rst
- fiftyone/plugins/definitions.py
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/plugins/core.py
82-82: Do not use bare except
(E722)
495-495: Use existing_plugins.get(plugin_name) instead of existing_plugins.get(plugin_name, None)
Replace existing_plugins.get(plugin_name, None) with existing_plugins.get(plugin_name)
(SIM910)
fiftyone/core/cli.py
3676-3676: Do not use bare except
(E722)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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: build
🔇 Additional comments (36)
fiftyone/core/cli.py (9)
3599-3601: Passing 'shadowed="all"' is well-considered
Including all plugins, whether shadowed or not, ensures a comprehensive listing.
3611-3615: Accumulating shadowed plugin paths
Collecting allshadow_pathsinto a set avoids duplication and helps efficiently track shadowed plugins.
3616-3623: Added 'shadowed' column for clarity
Displaying theshadowedstatus in the table headers meaningfully improves plugin visualization.
3626-3627: Logic for disabling shadowed plugins
Preventing a plugin from being enabled if it’s shadowed is a solid approach to avoid conflicts.
3632-3634: Updating 'enabled' and 'shadowed' fields
Storing and displayingenabledandshadowedstates ensures transparency about a plugin’s usability.
3639-3641: Dynamically remove 'shadowed' column when unused
Removing the column if there are no shadowed plugins keeps the table output clean.
3655-3657: Documentation now mentions plugins by directory
It’s helpful to clarify users can specify a directory for plugin info, aligning with the new plugin resolution logic.
3662-3666: Extended CLI argument accepts name or directory
EnablingNAME_OR_DIRusage provides flexible plugin lookups.
3670-3683: Refactor bare except to be more specific
Line 3676 uses a bareexcept, which can mask unexpected errors.Apply the following diff for clarity:
- except: + except ValueError:Or specify other relevant exception types that accurately reflect any legitimate failures.
🧰 Tools
🪛 Ruff (0.8.2)
3676-3676: Do not use bare
except(E722)
tests/unittests/plugins/core_tests.py (1)
153-155: Meaningful no-error check for shadowed plugins
Verifying that duplicate-named plugins do not raise an error is a good test of your new shadowing logic.fiftyone/plugins/core.py (26)
15-15: Import for typing hints
The addition offrom typing import Optionalis appropriate for the newly introduced optional fields.
47-47: Introducingshadow_paths
Adding an optionalshadow_pathslist inPluginPackageenables robust tracking of duplicate or shadowed plugins.
53-62: Expanded plugin listing with shadowed parameter
The newshadowedparameter inlist_pluginsaccommodates specialized queries for overshadowed plugins.
73-75: Graceful handling of 'all' for shadowed
Settingshadowed = Nonewhen the user supplies"all"is consistent with the existing logic for other parameters.
77-79: Passing shadowed to_list_plugins
All filter criteria (enabled, builtin, shadowed) are now delegated properly to_list_plugins.
152-153: Excluding shadowed plugins from downloaded list
Retrieving only non-shadowed, non-builtin plugins ensures this list is strictly user-downloadable plugins.
162-163: Listing enabled plugins
Filtering byenabled=Trueand excluding shadowed or builtins provides a neat set of active plugins.
172-173: Listing disabled plugins
Similarly ensuring shadowed or builtin plugins aren’t mixed simplifies plugin state visibility.
176-190: Expanded logic inget_plugin
Allowing a plugin to be fetched by name or directory fosters flexibility in various usage scenarios.
251-253: Gathering existing plugins for duplication checks
Capturing all plugins (enabled="all", builtin="all") is correct for identifying potential conflicts.
255-256: Initializedskipped_plugins
Using a set maintains quick membership checks for any plugin intentionally skipped during download.
299-300: Safe retrieval of existing plugins
Usingexisting_plugins.get(plugin.name, None)is functional here, though see the subsequent note for a minor improvement.
306-310: Prevent overwriting of builtin plugins
Raising an error ensures system-level plugins remain untouched, preserving core stability.
311-313: Selective overwrite of existing user plugin
Clearing out the old plugin directory before copying in the new code is prudent to avoid stale files.
322-326: Inform if requested plugins aren’t found
Gracefully warns about missing plugins for better user feedback.
492-496: Consolidated plugin fetch for creation
Consistently enumerates "all" plugins to detect name collisions, aligning with the approach used elsewhere.🧰 Tools
🪛 Ruff (0.8.2)
495-495: Use
existing_plugins.get(plugin_name)instead ofexisting_plugins.get(plugin_name, None)Replace
existing_plugins.get(plugin_name, None)withexisting_plugins.get(plugin_name)(SIM910)
500-505: Alerting users when builtin plugins are locked
Mirroring the approach indownload_pluginkeeps the plugin system robust.
514-514: Overwriting existing plugin directory
Displays a clear log message before resetting directories, keeping maintainers informed.
Line range hint
579-602: Refined_list_pluginshandles builtins, user plugins, and shadowed
Merging them seamlessly, then filtering on shadowed status before applying enablement info, ensures consistent plugin finalization.🧰 Tools
🪛 Ruff (0.8.2)
596-596: Do not use bare
except(E722)
614-623: Core logic for tracking shadowed plugins
This block neatly appends newly discovered duplicates toshadow_paths, designating them as overshadowed.
625-627: Conditional acceptance of strictly shadowed plugins
Whenshadowedis True or None, these duplicates appear in the returned list; otherwise, they remain hidden.
628-630: Adding non-shadowed to plugin list
Includes the primary plugin in the final set ifshadowedin (False, None).
631-633: Caching plugin references
Storing references inexisting_pluginsensures duplicates can be properly tracked as futureshadow_paths.
671-676: Known plugin check
Returns the first matching plugin by name, ignoring overshadowed duplicates due toshadowed=False.
678-678: Raising error for unknown plugin
Clear error message if no plugin matches, preventing silent failures.
681-689: Support for shadowed paths in_load_plugin_definition
Retainingshadow_pathsensures detailed introspection of overshadowed plugin definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unittests/plugins/core_tests.py (2)
Line range hint
143-152: Remove unused mock object and improve test setup clarity.The mock object
mis created but never used in the test. Additionally, the test setup could be more explicit about the expected plugin paths and configurations.Consider this improvement:
def test_duplicate_plugins(mocker, fiftyone_plugins_dir): mocker.patch("fiftyone.config.plugins_dir", fiftyone_plugins_dir) plugin_name = "test-plugin1" dup_plugin_dir = fiftyone_plugins_dir / "test-plugin2" - m = mock.Mock(spec=fop.core.PluginPackage(plugin_name, "path/to/plugin")) + # Create two plugins with the same name but different paths + original_plugin_dir = fiftyone_plugins_dir / "test-plugin1" with open(os.path.join(dup_plugin_dir, "fiftyone.yml"), "w") as f: pd = {k: plugin_name + "-" + k for k in _REQUIRED_YML_KEYS} f.write(yaml.dump(pd))
153-158: Enhance test coverage for shadowed plugins.While the test verifies the count of plugins, it could be more comprehensive by checking:
- Which plugin is considered active vs shadowed
- The paths of both plugins
- Other relevant plugin attributes
Consider adding these verifications:
plugin_names = [p.name for p in fop.list_plugins()] assert plugin_names.count("test-plugin1-name") == 1 + # Verify the active plugin is the one we expect + active_plugin = next(p for p in fop.list_plugins() if p.name == "test-plugin1-name") + assert active_plugin.path == str(fiftyone_plugins_dir / "test-plugin1") plugin_names = [p.name for p in fop.list_plugins(shadowed="all")] assert plugin_names.count("test-plugin1-name") == 2 + # Verify both plugins are found with correct paths + plugins = [p for p in fop.list_plugins(shadowed="all") if p.name == "test-plugin1-name"] + paths = {str(fiftyone_plugins_dir / "test-plugin1"), str(fiftyone_plugins_dir / "test-plugin2")} + assert {p.path for p in plugins} == paths
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittests/plugins/core_tests.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-app
- GitHub Check: build
Lots of unrelated output, would have expected just the error message instead of the trace. |
|
Manually tested duplicate plugins, duplicate builtins, disabling builtins (possibly unrelated to this PR!) ✅ |
|
Also tested duplicate builtins in the app ✅ |
| @@ -44,19 +44,22 @@ class PluginPackage: | |||
|
|
|||
| name: str | |||
| path: str | |||
| shadow_paths: Optional[list[str]] = None | |||
There was a problem hiding this comment.
I never thought I would see the day 😀. Doesn't this break your mantra: don't introduce types partially?
There was a problem hiding this comment.
My official mantra is follow the existing convention in the module you're contributing to, so I had to do it 😄
And... also had to do it because it's required when defining data classes 😉
|
|
||
| Args: | ||
| enabled (True): whether to include only enabled plugins (True) or only | ||
| disabled plugins (False) or all plugins ("all") | ||
| builtin (False): whether to include only builtin plugins (True) or only | ||
| non-builtin plugins (False) or all plugins ("all") | ||
| shadowed (False): whether to include only "shadowed" duplicate plugins | ||
| (True) or only usable plugins (False) or all plugins ("all") |
There was a problem hiding this comment.
Can builtin and shadowed be used together? The wording here "all plugins" is confusing.
There was a problem hiding this comment.
Yep all of these can be specified independently or together. They're composable filters
|
|
||
| logger.info(f"Overwriting existing plugin '{plugin.name}'") | ||
| plugin_dir = existing_plugin.directory | ||
| etau.delete_dir(plugin_dir) |
There was a problem hiding this comment.
When does this happen? I would hope that something like fiftyone plugins download XXX --force would be required to avoid accidentally deleting something. Possibly exists, just commenting here to make sure we double check this.
There was a problem hiding this comment.
Correct, user must specify download_plugin(..., overwrite=True) or fiftyone plugins download ... --overwrite in order to overwrite an existing plugin.
They'll get an error without forcing the overwrite.
| for plugin in _list_plugins( | ||
| enabled=enabled, builtin=builtin, shadowed=False | ||
| ): | ||
| if plugin.name == name: |
There was a problem hiding this comment.
We use this style all over and its not that efficient. Instead of loading everything and then searching for a name we should load only until we get the named plugin. Mostly concerned about unnecessary fs calls.
Maybe a _find_plugin(name=, plugin_dir=) would be useful for this.
Can handle such improvements elsewhere.
There was a problem hiding this comment.
yes refactoring _list_plugins() to be a generator would 2x average performance here.
Importantly, however, I believe all of the App's plugin usage goes through build_plugin_contexts(), which is cached via @plugins_cache.
| @@ -194,6 +202,7 @@ def to_dict(self): | |||
| """ | |||
| return { | |||
| "name": self.name, | |||
| "builtin": self.builtin, | |||
There was a problem hiding this comment.
I find that moving things like this adds to the OSS => teams conflicts.
There was a problem hiding this comment.
I moved it so that builtin appears immediately below name when printing plugin info:
$ fiftyone plugins info @voxel51/operators
key value
---------------------- -----------------------------------------------------
name @voxel51/operators
builtin True
...
That's a bit hacky of course as it relies on dicts having a stable iteration order, which isn't necessarily guaranteed in general.
There was a problem hiding this comment.
+1 for not causing diffs that have no functional purpose though
There was a problem hiding this comment.
Ah actually this makes sense, just didn't realize.
Change log
download_plugin(..., overwrite=True)andcreate_plugin(..., overwrite=True)cannot overwrite builtin pluginsExample usage
Summary by CodeRabbit
Release Notes
New Features
fiftyone plugins infocommand to accept plugin name or directory path.Documentation
Improvements
These updates provide more flexibility and clarity in managing FiftyOne plugins, making it easier to discover and interact with plugin resources.