feat: auto runtime selection with plugin harness registry#2335
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds lazy public exports for runtime-selection symbols and adjusts indentation around an existing error yield in the builtin runtime stream handler. ChangesRuntime selection surface
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the runtime registry system in praisonaiagents to support auto-selection and lazy loading of runtimes via a new SimpleRuntimeRegistry. However, the removal of the resolve_runtime function from registry.py while keeping it in the lazy-import exports of __init__.py will lead to an AttributeError when imported. It is highly recommended to restore resolve_runtime as a convenience function that resolves and instantiates the runtime to preserve backwards compatibility and prevent runtime crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| # Runtime Selection (Auto harness selection and registry) | ||
| 'AgentRuntimeProtocol': ('praisonaiagents.runtime.protocols', 'AgentRuntimeProtocol'), | ||
| 'resolve_runtime': ('praisonaiagents.runtime.registry', 'resolve_runtime'), |
There was a problem hiding this comment.
The resolve_runtime function is mapped here for lazy loading, but it has been completely removed from praisonaiagents/runtime/registry.py in this PR (replaced by resolve_runtime_factory). This will cause an AttributeError when resolve_runtime is imported or resolved. Please restore resolve_runtime in registry.py as a convenience function that resolves and instantiates the runtime, or update the lazy imports if resolve_runtime_factory was intended to replace it entirely.
| def resolve_runtime_factory(runtime_id: str) -> Callable[[], Any]: | ||
| """Resolve a runtime factory by ID. | ||
|
|
||
| Args: | ||
| runtime_id: Runtime identifier (e.g., "praisonai") | ||
| runtime_id: Runtime identifier | ||
|
|
||
| Returns: | ||
| AgentRuntimeProtocol instance | ||
| Factory function that creates AgentRuntimeProtocol instance | ||
|
|
||
| Raises: | ||
| ValueError: If runtime_id is not registered | ||
| """ | ||
| return _global_registry.resolve(runtime_id) | ||
|
|
||
|
|
||
| def add_runtime_alias(alias: str, canonical_runtime_id: str) -> None: | ||
| """Add an alias for a runtime. | ||
|
|
||
| Args: | ||
| alias: Alias name | ||
| canonical_runtime_id: The canonical runtime ID this alias points to | ||
|
|
||
| Raises: | ||
| ValueError: If canonical runtime ID is not registered | ||
| """ | ||
| _global_registry.add_alias(alias, canonical_runtime_id) | ||
| registry = _get_agent_runtime_registry() | ||
| return registry.resolve(runtime_id) |
There was a problem hiding this comment.
The resolve_runtime function was removed from this file, but it is still exported in praisonaiagents/__init__.py and praisonaiagents/runtime/__init__.py. This will cause AttributeError on import. Restoring resolve_runtime as a convenience function that resolves and instantiates the runtime preserves backwards compatibility and ensures the public API works as expected.
def resolve_runtime_factory(runtime_id: str) -> Callable[[], Any]:
"""Resolve a runtime factory by ID.
Args:
runtime_id: Runtime identifier
Returns:
Factory function that creates AgentRuntimeProtocol instance
Raises:
ValueError: If runtime_id is not registered
"""
registry = _get_agent_runtime_registry()
return registry.resolve(runtime_id)
def resolve_runtime(runtime_id: str) -> Any:
"""Resolve a runtime by ID and return an instance.
Args:
runtime_id: Runtime identifier
Returns:
Runtime instance
Raises:
ValueError: If runtime_id is not registered
"""
factory = resolve_runtime_factory(runtime_id)
return factory()
Greptile SummaryThis PR adds automatic runtime selection through the runtime registry. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: address critical runtime issues fou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def resolve_runtime_factory(runtime_id: str) -> Callable[[], Any]: | ||
| """Resolve a runtime factory by ID. | ||
|
|
||
| Args: | ||
| runtime_id: Runtime identifier (e.g., "praisonai") | ||
| runtime_id: Runtime identifier | ||
|
|
||
| Returns: | ||
| AgentRuntimeProtocol instance | ||
| Factory function that creates AgentRuntimeProtocol instance | ||
|
|
||
| Raises: | ||
| ValueError: If runtime_id is not registered | ||
| """ | ||
| return _global_registry.resolve(runtime_id) | ||
|
|
||
|
|
||
| def add_runtime_alias(alias: str, canonical_runtime_id: str) -> None: | ||
| """Add an alias for a runtime. | ||
|
|
||
| Args: | ||
| alias: Alias name | ||
| canonical_runtime_id: The canonical runtime ID this alias points to | ||
|
|
||
| Raises: | ||
| ValueError: If canonical runtime ID is not registered | ||
| """ | ||
| _global_registry.add_alias(alias, canonical_runtime_id) | ||
| registry = _get_agent_runtime_registry() | ||
| return registry.resolve(runtime_id) |
There was a problem hiding this comment.
praisonaiagents.__init__ now lazy-exports resolve_runtime from this module, but the changed registry only defines resolve_runtime_factory. Any caller using from praisonaiagents import resolve_runtime reaches this module and fails during lazy import instead of resolving the built-in runtime.
| def resolve_runtime_factory(runtime_id: str) -> Callable[[], Any]: | |
| """Resolve a runtime factory by ID. | |
| Args: | |
| runtime_id: Runtime identifier (e.g., "praisonai") | |
| runtime_id: Runtime identifier | |
| Returns: | |
| AgentRuntimeProtocol instance | |
| Factory function that creates AgentRuntimeProtocol instance | |
| Raises: | |
| ValueError: If runtime_id is not registered | |
| """ | |
| return _global_registry.resolve(runtime_id) | |
| def add_runtime_alias(alias: str, canonical_runtime_id: str) -> None: | |
| """Add an alias for a runtime. | |
| Args: | |
| alias: Alias name | |
| canonical_runtime_id: The canonical runtime ID this alias points to | |
| Raises: | |
| ValueError: If canonical runtime ID is not registered | |
| """ | |
| _global_registry.add_alias(alias, canonical_runtime_id) | |
| registry = _get_agent_runtime_registry() | |
| return registry.resolve(runtime_id) | |
| def resolve_runtime_factory(runtime_id: str) -> Callable[[], Any]: | |
| """Resolve a runtime factory by ID. | |
| Args: | |
| runtime_id: Runtime identifier | |
| Returns: | |
| Factory function that creates AgentRuntimeProtocol instance | |
| Raises: | |
| ValueError: If runtime_id is not registered | |
| """ | |
| registry = _get_agent_runtime_registry() | |
| return registry.resolve(runtime_id) | |
| def resolve_runtime(runtime_id: str) -> Any: | |
| """Resolve a runtime instance by ID.""" | |
| return resolve_runtime_factory(runtime_id)() |
| @runtime_checkable | ||
| class RuntimeRegistryProtocol(Protocol): | ||
| class RuntimeRegistry(Protocol): |
There was a problem hiding this comment.
RuntimeRegistry was the concrete registry class exposed from this module, but this change turns that name into a Protocol. Existing callers or tests that instantiate RuntimeRegistry() now fail immediately with a protocol-instantiation error instead of getting a usable registry.
| def is_runtime_available(runtime_id: str) -> bool: | ||
| """Check if a runtime is available in the global registry. | ||
|
|
||
| def is_available(self, runtime_id: str) -> bool: | ||
| """Check if a runtime is available (registered). | ||
|
|
||
| This is an alias for is_registered() to match the test expectations. | ||
| Args: | ||
| runtime_id: Runtime identifier to check | ||
|
|
||
| Args: | ||
| runtime_id: Runtime identifier | ||
|
|
||
| Returns: | ||
| True if runtime is registered or aliased, False otherwise | ||
| """ | ||
| return self.is_registered(runtime_id) | ||
|
|
||
| def list_runtimes(self) -> List[RuntimeRegistryEntry]: | ||
| """List all registered runtimes.""" | ||
| _initialize_builtin_runtimes() | ||
| with _registry_lock: | ||
| entries = [] | ||
| for runtime_id in _runtimes: | ||
| metadata = _runtime_metadata.get(runtime_id, {}) | ||
| entries.append(RuntimeRegistryEntry( | ||
| runtime_id=runtime_id, | ||
| display_name=metadata.get("display_name"), | ||
| description=metadata.get("description"), | ||
| is_builtin=metadata.get("is_builtin", False), | ||
| metadata=metadata | ||
| )) | ||
| return entries | ||
| Returns: | ||
| True if runtime is available, False otherwise | ||
| """ | ||
| registry = get_global_registry() | ||
| if registry is None: | ||
| return False | ||
|
|
||
| def list_names(self) -> List[str]: | ||
| """List all registered runtime names/IDs.""" | ||
| entries = self.list_runtimes() | ||
| return [e.runtime_id for e in entries] | ||
| return registry.is_registered(runtime_id) |
There was a problem hiding this comment.
is_runtime_available() now checks only the injected global registry, which starts as None, while the built-in praisonai runtime is registered in _agent_runtime_registry. In the default SDK path, list_runtimes() can show praisonai while is_runtime_available("praisonai") returns False, so runtime selection can incorrectly treat the default runtime as unavailable.
| def register(self, name: str, factory_loader: Callable[[], Any]) -> None: | ||
| """Register a runtime factory loader.""" | ||
| with self._lock: | ||
| self._factories[name] = factory_loader() |
There was a problem hiding this comment.
SimpleRuntimeRegistry.register() now assigns into _factories without checking whether the name already exists. A plugin or caller registering praisonai can silently replace the built-in runtime for all later resolutions, whereas the previous registry rejected duplicate runtime IDs.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2335 ✅
Key Finding (architecture)
This branch carried an older, parallel implementation ( Conflict resolution
Net diff vs main is now 7 lines — purely the genuinely additive top-level exports. Reviewer issues — all resolved by adopting main's implementation
Verification
Verdict: ✅ Approve (merge-ready)The branch is now rebased, conflict-free, and reduced to a minimal, non-redundant contribution that strengthens the SDK by surfacing the runtime-selection API at the package top level. No Agent-class bloat, no new dependencies, no hot-path regressions. Pushed |
…ixes #1939) - Add AgentRuntimeProtocol with supports() and selection_priority() methods - Implement auto resolution logic in runtime/resolve.py with priority-based selection - Add runtime registry with entry point discovery for plugin harnesses - Include built-in praisonai runtime as default fallback - Add comprehensive unit tests for auto selection behavior - Export runtime functionality through core SDK __init__.py Supports: - Auto mode selects highest-priority supporting runtime - Explicit runtime selection by ID - Entry point registration via 'praisonai.runtimes' group - Tie-breaking by priority and fallback to praisonai default - Lazy loading of plugin dependencies on selection only Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
- Fix TypeError in builtin.py: pop prompt/messages from kwargs to avoid duplicate argument error - Fix async/sync mismatch: use aexecute() instead of execute() in async context - Fix Python 3.8/3.9 compatibility: handle TypeError when entry_points() doesn't accept group parameter - Fix priority docstring: clarify that lower numbers = higher priority with correct example - Implement lazy loading for entry points: defer module loading until runtime is resolved Co-authored-by: Mervin Praison <454862+MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-1939-20260613-0842
branch
from
b2da59a to
7ee4854
Compare
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2335 ✅
State of the branchThis branch was already rebased onto The broken Reviewer issues — verified resolved (live in the SDK now)
Verification output: Tests
Phase 1 (AGENTS.md) assessment
Phase 2 (fixes)No code fixes required — all valid reviewer issues were already resolved by adopting Verdict: ✅ Approve (merge-ready)Rebased, conflict-free, minimal (7 lines), backward-compatible, no new dependencies, no hot-path regressions, and all reviewer-flagged bugs verified fixed. This strengthens the SDK by exposing the runtime-selection API cleanly.
• • Branch |
1 participant
Summary
Implements automatic runtime selection using the plugin harness registry.
Fixes #1939
Test plan
Rebased branch from triage agent; please verify against current
mainbefore merge.Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes