fix: defer framework adapter creation until YAML is loaded (fixes #1877)#1881
fix: defer framework adapter creation until YAML is loaded (fixes #1877)#1881praisonai-triage-agent[bot] wants to merge 3 commits into
Conversation
- Fixes issue where AgentsGenerator crashes with empty framework before YAML is read - Defers framework_adapter creation from __init__ to generate_crew_and_kickoff() - Changes default YAML framework from 'crewai' to 'praisonai' for better onboarding - Enables 'praisonai agents.yaml' to work without --framework flag Fixes #1877 Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
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? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAgentsGenerator defers framework adapter creation from initialization to the crew generation phase, allowing YAML framework configuration to be read before instantiating the adapter. The default framework fallback changes from crewai to praisonai when neither the CLI nor YAML explicitly specify a framework. ChangesFramework initialization lazy-loading and default fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
Greptile SummaryThis PR defers framework adapter creation from
Confidence Score: 5/5Safe to merge — both previously-flagged blocking issues are resolved and the async path now mirrors the sync path's initialization sequence. The two issues flagged in earlier review rounds — the async path's stale 'crewai' default and the AttributeError when self.framework_adapter was never assigned for an explicit framework — are both corrected. The async _arun_framework now unconditionally sets self.framework_adapter before calling arun, eliminating the crash. Both sync and async paths use the same resolve → assert_available → init_observability → setup sequence, and new tests verify the deferred-initialization behavior and the 'praisonai' default end-to-end. No files require special attention; agents_generator.py has the most logic change but the async path is now symmetric with the well-tested sync path. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI
participant AgentsGenerator
participant _run_framework
participant _arun_framework
participant FrameworkAdapter
CLI->>AgentsGenerator: "__init__(framework="")"
Note over AgentsGenerator: self.framework_adapter = None (deferred)
alt Sync path
AgentsGenerator->>_run_framework: generate_crew_and_kickoff()
_run_framework->>_run_framework: load YAML config
_run_framework->>_run_framework: "framework = self.framework or config.get('framework', 'praisonai')"
_run_framework->>FrameworkAdapter: _get_framework_adapter(framework)
FrameworkAdapter-->>_run_framework: initial_adapter
_run_framework->>FrameworkAdapter: initial_adapter.resolve()
FrameworkAdapter-->>_run_framework: adapter
_run_framework->>_run_framework: assert_framework_available(adapter.name)
_run_framework->>_run_framework: init_observability(adapter.name)
_run_framework->>FrameworkAdapter: adapter.setup()
_run_framework->>AgentsGenerator: "self.framework_adapter = adapter"
_run_framework->>FrameworkAdapter: adapter.run(config, ...)
else Async path
AgentsGenerator->>_arun_framework: agenerate_crew_and_kickoff()
_arun_framework->>_arun_framework: load YAML config
_arun_framework->>_arun_framework: "framework = self.framework or config.get('framework', 'praisonai')"
_arun_framework->>FrameworkAdapter: _get_framework_adapter(framework)
FrameworkAdapter-->>_arun_framework: initial_adapter
_arun_framework->>FrameworkAdapter: initial_adapter.resolve()
FrameworkAdapter-->>_arun_framework: adapter
_arun_framework->>_arun_framework: assert_framework_available(adapter.name)
_arun_framework->>_arun_framework: init_observability(adapter.name)
_arun_framework->>FrameworkAdapter: adapter.setup()
_arun_framework->>AgentsGenerator: "self.framework_adapter = adapter"
_arun_framework->>FrameworkAdapter: self.framework_adapter.arun(config, ...)
end
Reviews (3): Last reviewed commit: "test: add mandatory tests for framework ..." | Re-trigger Greptile |
|
@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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/praisonai/praisonai/agents_generator.py (2)
743-743:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Async path still uses 'crewai' default instead of 'praisonai'.
The synchronous path (line 597) correctly changed the default fallback to
'praisonai', but this async path still uses'crewai'. This creates inconsistent behavior:
generate_crew_and_kickoff()→ defaults to praisonaiagenerate_crew_and_kickoff()→ defaults to crewaiThis violates the PR objective to change the default framework to
'praisonai'and causes sync/async divergence.🔧 Proposed fix
- framework = self.framework or config.get('framework', 'crewai') + framework = self.framework or config.get('framework', 'praisonai')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/agents_generator.py` at line 743, The async path still falls back to 'crewai'—update the fallback default to 'praisonai' to match the sync path; locate the assignment to framework in agenerate_crew_and_kickoff (the line "framework = self.framework or config.get('framework', 'crewai')") and change the config.get default to 'praisonai' so both synchronous (generate_crew_and_kickoff) and asynchronous code use the same default framework.
776-796:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Async path missing key initialization steps present in sync path.
The async path has incomplete framework adapter initialization compared to the sync path. Missing components:
- Line 778: No
.resolve()call on the adapter (sync path line 601 calls it)- No observability initialization: Sync path calls
init_observability(adapter.name)(line 609)- No adapter setup hook: Sync path calls
adapter.setup(framework_tag=adapter.name)(line 612)- Line 782: Uses
frameworkstring instead ofadapter.namefor validation (sync path line 605 usesadapter.nameafter resolution)The
.resolve()call is particularly important as it handles framework variant selection (e.g., AutoGen version selection), and without it, the adapter may not be properly configured.🔧 Proposed fix to align async with sync initialization
# Update framework adapter if framework changed if framework != self.framework: self.framework = framework - self.framework_adapter = self._get_framework_adapter(framework) + initial_adapter = self._get_framework_adapter(framework) + self.framework_adapter = initial_adapter.resolve() # Validate framework availability from .framework_adapters.validators import assert_framework_available - assert_framework_available(framework) + assert_framework_available(self.framework_adapter.name) + + # Initialize observability hooks + from .observability.hooks import init_observability + init_observability(self.framework_adapter.name) + + # Run adapter setup hooks + self.framework_adapter.setup(framework_tag=self.framework_adapter.name) # Validate cli_backend compatibility - self._validate_cli_backend_compatibility(config, framework) + self._validate_cli_backend_compatibility(config, self.framework_adapter.name) - self.logger.info(f"Using framework: {framework}") + self.logger.info(f"Using framework: {self.framework_adapter.name}")Note: You may also want to remove or refactor the AutoGen version selection logic (lines 746-764) if
.resolve()now handles it internally, to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/agents_generator.py` around lines 776 - 796, The async branch of run/generate is missing adapter initialization steps present in the sync branch: after obtaining the adapter with _get_framework_adapter(framework) call adapter.resolve() to select the correct variant, then call init_observability(adapter.name) and adapter.setup(framework_tag=adapter.name) before running _validate_cli_backend_compatibility; also change the assert_framework_available(framework) / _validate_cli_backend_compatibility calls to use adapter.name (not the raw framework string) so validation targets the resolved adapter, then proceed to await self.framework_adapter.arun(...) as before.
🧹 Nitpick comments (1)
src/praisonai/praisonai/agents_generator.py (1)
746-764: Reduce AutoGen version selection duplication (sync usesadapter.resolve(), async hardcodes logic)Sync path calls
initial_adapter.resolve()(viaadapter = initial_adapter.resolve()), andAutoGenAdapter.resolve()performs AutoGen v0.2 vs v0.4 selection based onAUTOGEN_VERSION. Async path instead applies its own AutoGen selection logic and only calls_get_framework_adapter(framework)after rewritingframework(noresolve()call).This duplicates the same decision-making and can also create inconsistent behavior because async considers
config.get('autogen_version', ...), whileAutoGenAdapter.resolve()reads onlyos.environ["AUTOGEN_VERSION"].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/agents_generator.py` around lines 746 - 764, Replace the manual AutoGen version selection block with the same resolve-based flow used by the sync path: obtain the initial adapter via _get_framework_adapter("autogen") and call its resolve() (i.e., use AutoGenAdapter.resolve()) to determine the concrete adapter/framework instead of re-implementing autogen_version logic; remove the ad-hoc uses of autogen_v4_adapter, autogen_v2_adapter, autogen_version, and use_v4 and set framework based on the resolved adapter so async and sync share the same decision logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Line 743: The async path still falls back to 'crewai'—update the fallback
default to 'praisonai' to match the sync path; locate the assignment to
framework in agenerate_crew_and_kickoff (the line "framework = self.framework or
config.get('framework', 'crewai')") and change the config.get default to
'praisonai' so both synchronous (generate_crew_and_kickoff) and asynchronous
code use the same default framework.
- Around line 776-796: The async branch of run/generate is missing adapter
initialization steps present in the sync branch: after obtaining the adapter
with _get_framework_adapter(framework) call adapter.resolve() to select the
correct variant, then call init_observability(adapter.name) and
adapter.setup(framework_tag=adapter.name) before running
_validate_cli_backend_compatibility; also change the
assert_framework_available(framework) / _validate_cli_backend_compatibility
calls to use adapter.name (not the raw framework string) so validation targets
the resolved adapter, then proceed to await self.framework_adapter.arun(...) as
before.
---
Nitpick comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Around line 746-764: Replace the manual AutoGen version selection block with
the same resolve-based flow used by the sync path: obtain the initial adapter
via _get_framework_adapter("autogen") and call its resolve() (i.e., use
AutoGenAdapter.resolve()) to determine the concrete adapter/framework instead of
re-implementing autogen_version logic; remove the ad-hoc uses of
autogen_v4_adapter, autogen_v2_adapter, autogen_version, and use_v4 and set
framework based on the resolved adapter so async and sync share the same
decision logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e7ee4cd-242a-4193-924d-5889257b201a
📒 Files selected for processing (1)
src/praisonai/praisonai/agents_generator.py
|
@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 Architecture Review & Fix Implementation ✅I've conducted a comprehensive architecture review and successfully implemented critical fixes to align the async path with the sync path. Phase 1: Architecture Review ✅
Phase 2: Critical Issues Fixed ✅🔧 Fixed in commit
Phase 3: Final Verdict ✅APPROVED - All critical issues have been resolved:
The PR now correctly implements the deferred framework adapter initialization while maintaining feature parity between sync and async execution paths. The "better defaults" improvement (praisonai vs crewai) is consistently applied across both paths. Architecture compliance: All changes align with AGENTS.md protocol-driven design principles. Files Modified:
What Was Skipped:
|
- Fix async path default framework inconsistency (praisonai vs crewai) - Add missing adapter.resolve(), init_observability(), and adapter.setup() calls - Remove duplicated AutoGen version selection logic in favor of adapter.resolve() - Move AgentOps initialization after adapter resolution for consistency - Ensure both sync and async paths follow identical initialization patterns This fixes AttributeError crashes when self.framework_adapter is None and ensures consistent behavior between sync/async execution paths. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| initial_adapter = self._get_framework_adapter(framework) | ||
| adapter = initial_adapter.resolve() |
There was a problem hiding this comment.
autogen_version config silently dropped in async path
The old _arun_framework read autogen version from YAML first — config.get('autogen_version', os.environ.get("AUTOGEN_VERSION", "auto")) — so a YAML key like autogen_version: v0.4 controlled which AutoGen variant ran. The refactored path now calls AutoGenAdapter.resolve(), which only reads os.environ.get("AUTOGEN_VERSION", "auto"). Any YAML-specified autogen_version is silently ignored, causing users who pin the version in YAML to get the wrong AutoGen variant without any warning or error.
|
Merge review — AGENTS.md gap (CI green, merge CLEAN) CI ✅ | wrapper/YAML fix (#1877) Fix list:
Tests mandatory before merge. |
|
Claude finished @MervinPraison's task —— View job Merge Review & Test Implementation ✅Added mandatory tests and documentation before merge approval. Todo List:
Summary of Changes✅ Tests Added (commit
✅ Documentation Added:
✅ Tests Pass: Both new tests verify the fix works correctly and async/sync paths have proper parity. Architecture Review ConclusionThe PR now meets all AGENTS.md requirements:
READY FOR MERGE - All mandatory requirements satisfied. |
… parity - Add test verifying YAML files without framework key default to praisonai (fixes #1877) - Add test ensuring async path has same adapter.resolve() + init_observability() pattern as sync - Add FRAMEWORK_DEFAULTS_CHANGE.md documenting breaking change migration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #1877
Summary
Root Cause
The framework adapter was created in AgentsGenerator.init() before the YAML file was parsed. When running praisonai agents.yaml, the CLI passes an empty framework string, causing immediate failure.
Changes
Impact
Testing
The fix follows the protocol-driven design from AGENTS.md and maintains all existing functionality while resolving the initialization order issue.
Generated with Claude Code
Summary by CodeRabbit