Handle tree paths in GitHub identifiers #5324
Conversation
WalkthroughThe pull request introduces enhancements to the plugin management system in FiftyOne, focusing on improving GitHub repository handling and plugin discovery. The changes primarily involve adding a Changes
Sequence DiagramsequenceDiagram
participant User
participant GitHubRepository
participant GitHub API
User->>GitHubRepository: Initialize with repo and safe=True
GitHubRepository->>GitHub API: Validate repository path
GitHub API-->>GitHubRepository: Return path details
GitHubRepository-->>User: Repository with safe path
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (2)
fiftyone/plugins/utils.py (1)
119-122: Inline expression for cleaner code
Consider merging theif/elseinto a single ternary expression to make the logic more concise.Proposed fix snippet:
- if path is not None: - root = repo.safe_path + "/" + path - else: - root = repo.safe_path + root = repo.safe_path + "/" + path if path is not None else repo.safe_path🧰 Tools
🪛 Ruff (0.8.2)
119-122: Use ternary operator
root = repo.safe_path + "/" + path if path is not None else repo.safe_pathinstead ofif-else-blockReplace
if-else-block withroot = repo.safe_path + "/" + path if path is not None else repo.safe_path(SIM108)
fiftyone/plugins/core.py (1)
213-223: Consider explicitly handling multi-subdir scenarios
Whenrepo.safe_pathis used, the code assumes there is a single subdir. If multiple subdirs exist, no action is taken. Consider logging a warning or throwing an error to handle unexpected directory structures.+ else: + logger.warning( + "Multiple subdirectories found. Cannot reliably map " + f"safe_path='{repo.safe_path}'" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fiftyone/plugins/core.py(4 hunks)fiftyone/plugins/utils.py(2 hunks)fiftyone/utils/github.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/plugins/utils.py
119-122: Use ternary operator root = repo.safe_path + "/" + path if path is not None else repo.safe_path instead of if-else-block
Replace if-else-block with root = repo.safe_path + "/" + path if path is not None else repo.safe_path
(SIM108)
fiftyone/utils/github.py
84-84: Do not use bare except
(E722)
🔇 Additional comments (10)
fiftyone/utils/github.py (4)
37-40: Docstring enhancement is clear and concise
These lines cleanly document the new safe parameter and its behavior. Nicely done!
43-43: Approach for safe initialization
The safe parameter is initialized in the constructor and leads to _handle_safe_path(). This approach is clean and straightforward, ensuring all logic for path handling is encapsulated in _handle_safe_path().
Also applies to: 52-52, 55-56
102-105: New safe_path property complements the design
Exposing _safe_path via a read-only property is a good design choice, providing a clear interface for consuming code.
224-224: Improved token retrieval and enhanced error messaging
Adding _get_token() centralizes token retrieval logic. The improved error message with suggestions to set GITHUB_TOKEN is helpful for users.
Also applies to: 231-233, 239-245
fiftyone/plugins/utils.py (2)
82-86: Example usage is informative
These example lines effectively guide the user on how to discover plugins within a subdirectory.
96-104: Docstring clarification
Expanding the parameter documentation to include “GitHub tree path” usage is great for discoverability.
fiftyone/plugins/core.py (4)
25-25: Import statement
Importing fiftyone.core.storage as fos looks good and is consistent with project style.
176-177: Docstring update for tree paths
Adding tree path examples improves clarity and helps users quickly identify how to fetch subdirectory-scoped plugins.
192-196: Consistent safe=True usage
Using safe=True for both GitHub URL paths and repo identifiers ensures consistent behavior in all code paths.
229-233: User-friendly feedback when no plugins are found
Logging the source (repo.identifier or url) aids in diagnosing plugin discovery issues.
Change log
The @voxel51/plugins/install_plugin operator stopped working as a side effect of #4614 because the operator tried to issue a command like this:
where the
urlis pulled directly from here.When the operator was initially written, the above
download_plugin()syntax worked, but only because theGitHubRepositoryclass would automatically stripmain/plugins/dashboarddown tomain. However, that was causing our plugins API to not support branches/tags that contain/(what #4614 solved).This PR brings us back to the best of both worlds:
GitHubRepository(repo)now supportsrepostrings of the form<ref>[/<path>], where both<ref>and<path>may contain/.With this patch, the @voxel51/plugins/install_plugin operator works again.
And, as an added bonus, the
download_plugin()method now supports scoping downloads by subdirectory!Previously the only way to install a subset of plugins from a repository was to specify them by name via the
plugin_names=kwarg.Example usages
Search for plugins in a specific tree root
Download plugin from a specific tree root
New
GitHubRepository(..., safe=True)syntaxThis new syntax is what enables the other syntaxes to work under the hood: