UN-2930 [MISC] Add plugin infrastructure to unstract-core #1618
Conversation
Introduce a generic, framework-agnostic plugin system that supports Django, Flask, and Workers. This foundation enables dynamic loading of plugins with metadata validation, singleton/non-singleton patterns, and support for compiled extensions. Key components: - Generic PluginManager with plugin discovery and validation - DjangoPluginManager wrapper for Django apps - FlaskPluginManager wrapper for Flask apps - Redis client enhancement (REDIS_USERNAME support) - Plugin directory exclusions in .gitignore - Development dependency: debugpy for debugging This is the first PR in a series for UN-2930, establishing the infrastructure needed for subscription usage tracking and other plugin-based features. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary by CodeRabbit
WalkthroughAdds a framework-agnostic PluginManager, Flask and Django wrappers and helpers, backend AppConfig and module accessors to load plugins at startup, plugin docs and per-directory .gitignore adjustments, REDIS_USERNAME fallback in Redis client env parsing, and debugpy added to worker dev dependencies. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (Django/Flask)
participant Wrapper as Framework PluginManager
participant Generic as Generic PluginManager
participant FS as Filesystem (plugins dir)
App->>Wrapper: instantiate (app?/plugins_dir, plugins_pkg, logger)
Note right of Wrapper #e6f7ff: lazy init / singleton storage
App->>Wrapper: load_plugins()
Wrapper->>Generic: init manager (plugins_dir, plugins_pkg, logger, registration_callback)
Generic->>FS: scan plugins directory
FS-->>Generic: plugin entries
loop per plugin
Generic->>Generic: import plugin module
Generic->>Generic: validate metadata (name, version, is_active, disable)
alt valid & active
Generic->>Wrapper: call registration_callback(plugin_data)
Wrapper->>App: register artifacts (e.g., Flask blueprint)
Generic-->>Wrapper: record plugin loaded
else skip / invalid
Generic-->>Wrapper: skip / log
end
end
Generic-->>Wrapper: finished
App->>Wrapper: get_plugin(name) / has_plugin(name)
Wrapper-->>App: metadata / boolean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (3)
workers/pyproject.toml (1)
49-50: Minor: Consider pinning upper version bound for reproducibility.The addition of
debugpy>=1.8.14follows the existing project pattern of lower-bound-only versioning. However, for dev tools that may introduce breaking changes in future major versions, consider adding an upper bound constraint (e.g.,debugpy>=1.8.14,<2.0.0) to ensure reproducible development environments and prevent unexpected breakage in CI pipelines.This is an optional improvement and applies broadly to the dev dependencies section; the change itself is sound and appropriate.
unstract/core/src/unstract/core/flask/__init__.py (1)
3-3: Consider removing the unused noqa directive.Modern linters understand re-export patterns, making the
noqa: F401directive unnecessary here.Apply this diff if you'd like to clean it up:
-from .plugin import PluginManager, plugin_loader # noqa: F401 +from .plugin import PluginManager, plugin_loaderunstract/core/src/unstract/core/django/__init__.py (1)
1-1: Consider removing the unused noqa directive.Modern linters understand re-export patterns, making the
noqa: F401directive unnecessary here.Apply this diff if you'd like to clean it up:
-from .plugin import DjangoPluginManager, PluginManager, plugin_loader # noqa: F401 +from .plugin import DjangoPluginManager, PluginManager, plugin_loader
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
workers/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignore(1 hunks)unstract/core/src/unstract/core/cache/redis_client.py(2 hunks)unstract/core/src/unstract/core/django/__init__.py(1 hunks)unstract/core/src/unstract/core/django/plugin.py(1 hunks)unstract/core/src/unstract/core/flask/__init__.py(1 hunks)unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/.gitignore(1 hunks)unstract/core/src/unstract/core/plugins/__init__.py(1 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)workers/pyproject.toml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/__init__.py
3-3: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
unstract/core/src/unstract/core/flask/plugin.py
87-90: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/django/plugin.py
76-79: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/plugins/plugin_manager.py
62-62: Unused static method argument: args
(ARG004)
113-113: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
113-113: Use explicit conversion flag
Replace with conversion flag
(RUF010)
156-156: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
156-156: Use explicit conversion flag
Replace with conversion flag
(RUF010)
unstract/core/src/unstract/core/django/__init__.py
1-1: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
⏰ 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 (18)
unstract/core/src/unstract/core/cache/redis_client.py (2)
255-255: LGTM! Documentation clearly explains the alternative variable.The documentation update appropriately describes REDIS_USERNAME as an optional alternative to REDIS_USER, providing clarity for configuration flexibility.
267-267: No changes required—the implementation is correct and intentional.The fallback logic at line 267 is documented (see the docstring at line 255) and serves as a backward-compatibility mechanism. The behavior where empty
REDIS_USERfalls back toREDIS_USERNAMEis by design, allowing flexible configuration. Verification shows:
REDIS_USERNAMEis actively used (e.g., platform-service) as an alternative toREDIS_USER- Sample configurations consistently use
REDIS_USER=default(never empty), with no issues found- The implementation correctly handles all cases: missing variables return
None, and the fallback provides configuration flexibility.gitignore (1)
646-647: LGTM! Plugin directory exclusion is consistent.The addition properly excludes the subscription usage plugin directory, aligning with the plugin infrastructure strategy.
unstract/core/src/unstract/core/plugins/.gitignore (1)
1-7: LGTM! Effective OSS/enterprise separation strategy.The ignore pattern properly isolates enterprise plugins while preserving core infrastructure files.
unstract/core/src/unstract/core/plugins/plugin_manager.py (5)
1-25: LGTM! Well-documented framework-agnostic plugin manager.The module docstring and class documentation clearly communicate the purpose and capabilities.
30-61: LGTM! Comprehensive initialization with good defaults.The parameter documentation is thorough, and the path normalization handles both string and Path inputs appropriately.
62-72: LGTM! Singleton pattern correctly implemented.The conditional singleton behavior based on
use_singletonis properly implemented. The static analysis warning about unusedargsis a false positive—the parameter is necessary for the__new__signature even though it's not directly used in this method.
74-161: LGTM! Robust plugin loading with comprehensive error handling.The plugin loading logic handles multiple scenarios well:
- Missing plugin directory with graceful degradation
- Support for both directory and .so modules
- Metadata validation with disabled plugin filtering
- Proper error isolation per plugin
The static analysis suggestions to use
logging.exception(lines 113, 156) are style preferences—usinglogging.errorin a loop context is acceptable since you want to continue processing other plugins.
162-199: LGTM! Clean accessor API with safe defaults.The plugin accessor methods provide a consistent interface with appropriate return values for missing plugins.
unstract/core/src/unstract/core/plugins/__init__.py (1)
1-12: LGTM! Clean package initialization with clear documentation.The module docstring effectively explains the plugin system's purpose and framework integration options.
unstract/core/src/unstract/core/flask/plugin.py (4)
1-14: LGTM! Well-documented Flask integration.The module docstring clearly explains the Flask-specific functionality.
25-60: LGTM! Flexible singleton pattern with parameter updates.The
__new__implementation properly handles the singleton pattern while allowing parameter updates when plugins directory or package changes. The conditional reinitialization ensures the manager stays synchronized with configuration changes.
74-91: LGTM! Appropriate initialization and loading with error checking.The manager initialization with Flask-specific parameters (app.logger, blueprint callback, src submodule) is correct, and the RuntimeError in
load_pluginsprovides clear guidance when used incorrectly.
93-155: LGTM! Consistent accessor API with safe defaults.The accessor methods and convenience function provide a clean, user-friendly API with appropriate safety checks.
unstract/core/src/unstract/core/django/plugin.py (4)
1-14: LGTM! Well-documented Django integration.The module docstring clearly explains Django-specific functionality and differences from the Flask wrapper.
24-71: LGTM! Clean Django-specific implementation.The singleton pattern and manager initialization are simpler than the Flask version, appropriately reflecting Django's different lifecycle. The comment on Line 69 helpfully explains why no registration callback is needed.
73-122: LGTM! Consistent accessor API with safe defaults.The methods mirror the Flask wrapper's interface, providing consistency across framework integrations.
128-163: LGTM! Excellent documentation with practical example.The docstring example demonstrates the complete usage pattern clearly, making it easy for developers to integrate the plugin system into Django apps.
chandrasekharan-zipstack
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
.gitignore(0 hunks)backend/plugins/.gitignore(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
⏰ 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
NOTE: Existing mechanism is left untouched to avoid breaking anything
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/plugins/__init__.py (1)
37-60: Convenience function simplifies plugin access.The get_plugin() wrapper provides a clean single-line interface for retrieving plugin metadata, which is well-documented with clear examples.
Optionally, you can sort
__all__alphabetically to address the static analysis hint (RUF022), though this is a minor formatting preference.Apply this diff if you want to address the linting hint:
-__all__ = ["get_plugin_manager", "get_plugin"] +__all__ = ["get_plugin", "get_plugin_manager"]backend/plugins/apps.py (1)
20-34: Plugin loading implementation is appropriate for opt-in plugin system.The ready() method correctly:
- Uses local import to avoid AppRegistryNotReady errors
- Loads plugins after Django initialization
- Implements graceful error handling that allows Django to continue even if plugins fail
This is the right approach for an additive, backward-compatible plugin system.
Optionally, consider using
logger.debug()instead oflogger.info()on line 31 to reduce log noise in production, since successful plugin loading is expected behavior rather than noteworthy information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
backend/backend/settings/base.py(1 hunks)backend/plugins/__init__.py(1 hunks)backend/plugins/apps.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
backend/plugins/__init__.py
60-60: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ 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)
backend/plugins/__init__.py (2)
1-11: LGTM!The module initialization is clean and follows Python best practices with proper docstring, imports, and logger configuration.
17-34: Lazy initialization pattern is appropriate for Django AppConfig integration.The singleton pattern correctly defers plugin loading until requested, and the docstring clearly documents that loading happens separately in PluginsConfig.ready(). The implementation is clean and follows the module-level singleton pattern.
Note: The pattern is not thread-safe (race condition possible if multiple threads call this concurrently), but since Django's ready() hook is called once during startup and the PR describes this as opt-in, this should be acceptable for the current use case.
backend/backend/settings/base.py (1)
289-289: LGTM!Explicitly referencing the AppConfig class (
plugins.apps.PluginsConfig) follows Django best practices and ensures thePluginsConfig.ready()hook is properly invoked during Django startup to load plugins.backend/plugins/apps.py (1)
1-18: LGTM!The AppConfig class is properly configured with appropriate name and verbose_name attributes. The docstring clearly explains the purpose of deferred loading to avoid Django initialization issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/plugins/.gitignore (1)
1-15: Good—critical fix has been properly applied.The past review flagged a critical issue where the broad
*/pattern (line 3) would prevent negations on child paths because git doesn't descend into ignored parent directories. The current code correctly addresses this by un-ignoring parent directories (!authentication/,!api/) before their child paths, allowing git to descend and apply the negation rules.The structure is now semantically correct: each child path negation is now reachable because its parent has been un-ignored first.
Minor note: Line 15 uses
!workflow_managerwhile other directories use the trailing slash form (!authentication/,!api/,!api/dto/). For consistency and explicitness, consider changing it to!workflow_manager/if it's a directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
backend/plugins/.gitignore(1 hunks)
chandrasekharan-zipstack
requested review from
Deepak-Kesavan,
athul-rs and
muhammad-ali-e
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
unstract/core/src/unstract/core/plugins/README.md (1)
101-106: Add language specifier to fenced code block.The directory structure example should specify a language identifier for the code fence (e.g.,
textorplaintext) to improve rendering and satisfy markdown linting rules.Apply this diff:
-``` +```text unstract/core/src/unstract/core/plugins/ ├── README.md # This file ├── __init__.py # Package initialization ├── plugin_manager.py # Core plugin discovery and loading</blockquote></details> <details> <summary>unstract/core/src/unstract/core/flask/plugin.py (2)</summary><blockquote> `25-60`: **Consider documenting the three-parameter initialization requirement.** The singleton initialization logic requires all three parameters (`app`, `plugins_dir`, `plugins_pkg`) to be present before initializing the manager (line 50). This requirement isn't immediately obvious from the method signature, which marks all parameters as optional. Consider either: 1. Adding a note in the docstring explaining that initialization only occurs when all three parameters are provided, or 2. Validating at instantiation time and providing a clear error if only some parameters are provided This would prevent confusing usage patterns where a user provides only some parameters and expects initialization to occur. --- `84-91`: **Error message could be extracted to a constant.** Ruff suggests extracting long error messages to a constant or defining them in a custom exception class. This is a minor style improvement that can enhance maintainability. If desired, apply this refactor: ```diff +_NOT_INITIALIZED_ERROR = ( + "FlaskPluginManager not initialized. " + "Call with app, plugins_dir, and plugins_pkg first." +) + def load_plugins(self) -> None: """Load plugins using the generic manager.""" if not self._initialized: - raise RuntimeError( - "FlaskPluginManager not initialized. " - "Call with app, plugins_dir, and plugins_pkg first." - ) + raise RuntimeError(_NOT_INITIALIZED_ERROR) self._manager.load_plugins()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
backend/plugins/.gitignore(1 hunks)backend/plugins/README.md(1 hunks)unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/.gitignore(1 hunks)unstract/core/src/unstract/core/plugins/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/plugins/README.md
- backend/plugins/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- unstract/core/src/unstract/core/plugins/.gitignore
🧰 Additional context used
🪛 LanguageTool
unstract/core/src/unstract/core/plugins/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # Unstract Plugin System ## Plugin Metadata Forma...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
unstract/core/src/unstract/core/plugins/README.md
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
87-90: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (3)
unstract/core/src/unstract/core/flask/plugin.py (3)
65-72: LGTM! Blueprint registration correctly uses metadata.The blueprint registration callback correctly retrieves the plugin name from
metadata.get('name', 'unknown'), addressing the previous review comment. The implementation properly registers Flask blueprints and logs the activity.
93-124: LGTM! Safe default pattern for uninitialized access.The implementation correctly returns safe defaults (empty dict or False) when the manager is not yet initialized, preventing errors while maintaining a clean API. This is good defensive programming.
131-155: LGTM! Clean convenience function with helpful documentation.The
plugin_loaderconvenience function provides a clean, straightforward API for the common use case of creating a plugin manager and loading plugins in one call. The docstring example is clear and helpful.
| host=os.getenv("REDIS_HOST", "localhost"), | ||
| port=int(os.getenv("REDIS_PORT", "6379")), | ||
| username=os.getenv("REDIS_USER"), | ||
| username=os.getenv("REDIS_USER") or os.getenv("REDIS_USERNAME"), |
There was a problem hiding this comment.
@chandrasekharan-zipstack why we need to introduce a new variable instead of using existing for redis user?.
There was a problem hiding this comment.
@jaseemjaskp platform-service alone differs by checking for this REDIS_USERNAME env. I figured that changing it might mean updating the secrets and to ensure everything is backward compatible I did this. Later when we choose to standardize some of the envs across services, we could do away with this line
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
86-89: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/plugins/plugin_manager.py
61-61: Unused static method argument: args
(ARG004)
133-133: Consider (*rel_parts, pkg_name) instead of concatenation
Replace with (*rel_parts, pkg_name)
(RUF005)
142-142: Consider (*rel_parts, item.name) instead of concatenation
Replace with (*rel_parts, item.name)
(RUF005)
179-179: Loop control variable plugin_path not used within loop body
Rename unused plugin_path to _plugin_path
(B007)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
237-239: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
238-238: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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
- Update plugin_loader docstring to clarify plugins_dir and plugins_pkg are required - Add parameter validation with helpful ValueError message - Include practical example showing correct usage pattern - Improves developer experience with clear setup requirements Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
unstract/core/src/unstract/core/flask/plugin.py (1)
83-123: Consider consistent error handling across methods.There's an inconsistency in how methods handle the uninitialized state:
load_plugins()raisesRuntimeErrorwith a helpful messageget_plugin(),has_plugin(), andpluginsreturn safe defaults ({}orFalse)While this may be intentional (mutations vs. queries), it could surprise callers who expect consistent behavior. Consider either:
- Having all methods raise when uninitialized (fail-fast approach), or
- Having all methods return safe defaults (defensive approach)
The current approach is acceptable if documented, but consistency improves predictability.
Note: Static analysis (Ruff TRY003) suggests extracting the long error messages on lines 86-89 into custom exception classes, though this is a style preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
86-89: Avoid specifying long messages outside the exception class
(TRY003)
158-164: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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)
unstract/core/src/unstract/core/flask/plugin.py (4)
1-14: LGTM! Clear module documentation and appropriate imports.The module docstring clearly describes the Flask-specific wrapper purpose, and all imports are necessary for the implementation.
62-81: LGTM! Blueprint registration correctly implemented.The Flask registration callback properly retrieves the plugin name from metadata (line 71), and the generic manager is correctly initialized with Flask-specific parameters. The past review issue regarding incorrect name source has been properly addressed.
126-127: LGTM! Backward compatibility maintained.The alias ensures existing code using
PluginManagercontinues to work.
130-167: LGTM! Parameter validation addresses previous concerns.The function now properly validates that both
plugins_dirandplugins_pkgare provided before creating the manager (lines 157-164), which addresses the previous review comment about failing with default usage. The clear error message with example usage helps developers understand the requirements.
Note: Static analysis (Ruff TRY003) suggests extracting the long error message on lines 158-164 into a custom exception class, though this is a style preference and the current inline message with example is quite helpful.
- Add threading.Lock at class level to protect singleton instance - Guard instance creation and state mutations with lock - Prevent race conditions in multi-threaded Flask environments - Ensure thread-safe initialization of app, plugins_dir, and plugins_pkg 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unstract/core/src/unstract/core/flask/plugin.py (1)
87-94: Consider extracting exception messages to exception classes.The error message is clear and helpful. However, static analysis flags TRY003 for long messages defined inline. This is a minor style consideration—the current implementation is functional.
If you prefer to align with Ruff's recommendation, you could define custom exception classes:
class PluginManagerNotInitializedError(RuntimeError): """Raised when FlaskPluginManager is used before initialization.""" def __init__(self): super().__init__( "FlaskPluginManager not initialized. " "Call with app, plugins_dir, and plugins_pkg first." )Then raise with
raise PluginManagerNotInitializedError(). The same pattern applies to line 162-168. However, this adds complexity for minimal benefit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
90-93: Avoid specifying long messages outside the exception class
(TRY003)
162-168: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (6)
unstract/core/src/unstract/core/flask/plugin.py (6)
1-15: LGTM: Clean imports and clear documentation.The module docstring effectively describes the Flask integration purpose, and all imports are necessary and properly organized.
24-64: LGTM: Thread-safe singleton pattern correctly implemented.The previous thread-safety concern has been properly addressed. The lock guards the entire instance creation and initialization logic, preventing race conditions in multi-threaded Flask environments. The short-circuit evaluation on line 56 ensures
_plugins_dirand_plugins_pkgare only checked after they've been set.
66-85: LGTM: Flask integration callback correctly implemented.The previous issue with blueprint registration logging has been fixed—line 75 now correctly retrieves the plugin name from
metadatarather thanplugin_data. The registration callback properly integrates Flask blueprints and logging.
96-127: LGTM: Consistent defensive query API.The query methods (
get_plugin,has_plugin, andpluginsproperty) all follow a consistent pattern of returning safe defaults when the manager is not initialized, preventing crashes while signaling the uninitialized state. This is a good defensive design that maintains usability.
130-131: LGTM: Clean backward compatibility alias.The alias maintains compatibility with existing code while the new
FlaskPluginManagername better reflects the class's purpose.
134-171: LGTM: Well-designed convenience function with proper validation.The previous issue where
plugin_loadercould fail with default parameters has been properly fixed. The validation on lines 161-168 now ensures both required parameters are provided before initialization, and the error message includes helpful usage examples. The function signature and documentation clearly indicate the requirements.
…dling - Merge nested if statements in FlaskPluginManager.getInstance() into single compound condition - Remove redundant PermissionError from plugin_manager (OSError parent class covers it) 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
|
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
unstract/core/src/unstract/core/plugins/plugin_manager.py (3)
133-133: Consider using tuple unpacking for concatenation.The tuple concatenations can be simplified using unpacking syntax for slightly better readability and performance.
Apply this diff:
- module_path = ".".join(rel_parts + (pkg_name,)) + module_path = ".".join((*rel_parts, pkg_name))- new_rel_parts = rel_parts + (item.name,) + new_rel_parts = (*rel_parts, item.name)Also applies to: 142-142
179-179: Rename unused loop variable to indicate it's intentionally unused.The
plugin_pathvariable is not used within the loop body. Following Python conventions, prefix it with an underscore to indicate it's intentionally unused.Apply this diff:
- for plugin_path, module_name in plugin_candidates: + for _plugin_path, module_name in plugin_candidates:
236-239: Uselogging.exceptionfor better error diagnostics.When logging errors in an exception handler, use
logging.exception()instead oflogging.error()to automatically include the stack trace, which aids debugging.Apply this diff:
except KeyError as e: - self.logger.error( + self.logger.exception( f"Invalid metadata for plugin '{module_name}': {str(e)}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
94-97: Avoid specifying long messages outside the exception class
(TRY003)
166-172: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/plugins/plugin_manager.py
61-61: Unused static method argument: args
(ARG004)
133-133: Consider (*rel_parts, pkg_name) instead of concatenation
Replace with (*rel_parts, pkg_name)
(RUF005)
142-142: Consider (*rel_parts, item.name) instead of concatenation
Replace with (*rel_parts, item.name)
(RUF005)
179-179: Loop control variable plugin_path not used within loop body
Rename unused plugin_path to _plugin_path
(B007)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
237-239: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
238-238: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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 (2)
unstract/core/src/unstract/core/flask/plugin.py (2)
24-68: Thread-safe singleton implementation looks good!The double-checked locking pattern with
threading.Lockcorrectly handles concurrent access in multi-threaded Flask applications. The lock guards both instance creation and state updates, preventing race conditions.
165-175: Parameter validation prevents initialization errors effectively.The validation requiring both
plugins_dirandplugins_pkgcorrectly prevents the RuntimeError that would occur if the manager were created without proper initialization parameters. The helpful error message with usage example improves developer experience.
| from unstract.core.plugins import PluginManager as GenericPluginManager | ||
|
|
||
|
|
||
| class FlaskPluginManager: |
There was a problem hiding this comment.
looks like same as the django.plugin .. can we have a Base class for this Plugin manager to accommodate duplicate methods and base methods...?
There was a problem hiding this comment.
Will see if there is an easy refactor for this, however I tried this since Flask based ones accept a blueprint in case they define URLs (similar to our pluggable_apps). Let me revisit the code duplication here and get back to you
| return super().__new__(cls) | ||
|
|
||
| def _find_plugin_modules( | ||
| self, base_path: Path, max_depth: int = 2 |
There was a problem hiding this comment.
why this is 2? is it meant to the main plugin directory?
There was a problem hiding this comment.
Yeah @muhammad-ali-e. Kept it 2 to cover all our existing scenarios of how we manage plugins across backend and prompt-service. We sometimes group plugins and nest it under a folder
| if item.name.endswith(".so"): | ||
| pkg_name = item.name.split(".")[0] | ||
| module_path = ".".join(rel_parts + (pkg_name,)) | ||
| plugins.append((item, module_path)) |
There was a problem hiding this comment.
Is it like this? Whe do we remove .so?
There was a problem hiding this comment.
This is from our existing plugin loading mechanisms of including a shared library. Left this logic as is to not cause any regression. We could revisit its necessity later maybe
| "flake8>=6.0.0", | ||
| "mypy>=1.5.0" | ||
| "mypy>=1.5.0", | ||
| "debugpy>=1.8.14", |
There was a problem hiding this comment.
This is for attaching a debugger to a worker process. This is present for the other services as a dev dependency
What
unstract-coreas wellWhy
How
Core Plugin Infrastructure:
unstract/core/src/unstract/core/plugins/plugin_manager.py- Generic PluginManager with:.socompiled extensionsFramework-Specific Wrappers:
unstract/core/src/unstract/core/django/plugin.py- DjangoPluginManager wrapperplugin_loader()functionunstract/core/src/unstract/core/flask/plugin.py- FlaskPluginManager wrapperSupporting Changes:
.gitignoreto exclude plugin directories that will contain cloud-specific codedebugpy>=1.8.14to workers dev dependencies for debugging supportREDIS_USERNAMEenv var (alternative toREDIS_USER)Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No, this PR will not break any existing features.
Reasoning:
REDIS_USERsupportEnv Config
Optional (backward compatible):
REDIS_USERNAME- Alternative toREDIS_USERfor Redis authentication (not required)Related Issues or PRs
Dependencies Versions
Added:
debugpy>=1.8.14(dev dependency in workers/pyproject.toml)No changes to runtime dependencies.
Notes on Testing
Pre-commit Validation
Screenshots
N/A - Infrastructure changes only, no UI changes
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]