fix: prevent test collection failures from optional modules#1546
fix: prevent test collection failures from optional modules#1546MervinPraison merged 2 commits intomainfrom
Conversation
Fixes test collection issues when optional dependencies (praisonaiui, google.generativeai) are not installed in base setup. Changes: - praisonai/inc/models.py: Add try/catch guards around importlib.util.find_spec() calls to handle missing parent packages gracefully - praisonai/ui/_aiui_datastore.py: Move dependency imports to lazy loading in __init__ to allow module import during test collection Resolves #1541 Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughAdded defensive exception handling around provider SDK availability detection in models.py and implemented lazy dependency loading in _aiui_datastore.py. These changes defer dependency validation from module import time to runtime instance creation, allowing test collection to proceed when optional modules are unavailable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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:
|
Greptile SummaryThis PR fixes test-collection failures in base editable installs by (1) wrapping Confidence Score: 5/5Safe to merge; both issues from the bug report are resolved and no runtime regressions introduced. The two targeted fixes are minimal, focused, and well-reasoned. The only remaining finding is a P2 style concern ( No files require special attention; Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant PraisonAISessionDataStore
participant _build_impl_cls
participant _PraisonAISessionDataStoreImpl
participant BaseDataStore
Caller->>PraisonAISessionDataStore: PraisonAISessionDataStore(store)
PraisonAISessionDataStore->>_build_impl_cls: __new__(cls, store)
alt _impl_cls already cached
_build_impl_cls-->>PraisonAISessionDataStore: return cached _impl_cls
else first call
_build_impl_cls->>_build_impl_cls: import praisonaiui.datastore.BaseDataStore
_build_impl_cls->>_build_impl_cls: import praisonaiagents.session
_build_impl_cls->>_PraisonAISessionDataStoreImpl: define class(BaseDataStore)
_build_impl_cls->>_build_impl_cls: cache _impl_cls
_build_impl_cls-->>PraisonAISessionDataStore: return _impl_cls
end
PraisonAISessionDataStore->>_PraisonAISessionDataStoreImpl: impl_cls(store)
_PraisonAISessionDataStoreImpl->>BaseDataStore: super().__init__()
BaseDataStore-->>_PraisonAISessionDataStoreImpl: initialized
_PraisonAISessionDataStoreImpl-->>Caller: _PraisonAISessionDataStoreImpl instance
Reviews (2): Last reviewed commit: "fix: implement thread-safe class factory..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (2)
src/praisonai/praisonai/inc/models.py (1)
19-34: Nit: fold the three guards into a small helper (DRY).The fix is correct —
find_spec("google.generativeai")raisesModuleNotFoundErrorwhen the parentanthropic/coherespecs,find_specnormally just returnsNone, so the wrappers are defensive but redundant. Consolidating into a helper removes the duplication and makes intent clearer, and you can optionally apply it toOPENAI_AVAILABLEat Line 18 for consistency.♻️ Proposed refactor
-OPENAI_AVAILABLE = importlib.util.find_spec("openai") is not None - -# Guard against missing parent packages (fixes test collection in base install) -try: - GOOGLE_GENAI_AVAILABLE = importlib.util.find_spec("google.generativeai") is not None -except (ImportError, AttributeError, ValueError): - GOOGLE_GENAI_AVAILABLE = False - -try: - ANTHROPIC_AVAILABLE = importlib.util.find_spec("anthropic") is not None -except (ImportError, AttributeError, ValueError): - ANTHROPIC_AVAILABLE = False - -try: - COHERE_AVAILABLE = importlib.util.find_spec("cohere") is not None -except (ImportError, AttributeError, ValueError): - COHERE_AVAILABLE = False +def _is_available(spec: str) -> bool: + """Safely check if a module spec is importable. + + Guards against missing parent packages (e.g. `google.generativeai` + when the `google` namespace is absent) so test collection in a + base install does not fail. + """ + try: + return importlib.util.find_spec(spec) is not None + except (ImportError, AttributeError, ValueError): + return False + +OPENAI_AVAILABLE = _is_available("openai") +GOOGLE_GENAI_AVAILABLE = _is_available("google.generativeai") +ANTHROPIC_AVAILABLE = _is_available("anthropic") +COHERE_AVAILABLE = _is_available("cohere")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/inc/models.py` around lines 19 - 34, Create a small helper (e.g., is_module_available(module_name)) that calls importlib.util.find_spec(module_name) inside a try/except catching ImportError, AttributeError, ValueError (and ModuleNotFoundError if desired) and returns a boolean, then replace the repeated guards for GOOGLE_GENAI_AVAILABLE, ANTHROPIC_AVAILABLE, and COHERE_AVAILABLE with calls to that helper (and optionally use it for OPENAI_AVAILABLE) so the logic is DRY and centralized while preserving the original defensive behavior.src/praisonai/praisonai/ui/_aiui_datastore.py (1)
33-33: Nit (Ruff RUF005): prefer tuple unpacking over concatenation.Only relevant if the class-factory refactor above is not applied.
- self.__class__.__bases__ = (BaseDataStore,) + self.__class__.__bases__ + self.__class__.__bases__ = (BaseDataStore, *self.__class__.__bases__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/ui/_aiui_datastore.py` at line 33, Replace the tuple concatenation used to prepend BaseDataStore to the current bases by using tuple unpacking instead; update the assignment to self.__class__.__bases__ in the class-factory code so BaseDataStore is prepended with tuple unpacking rather than by concatenating tuples (locate the assignment to self.__class__.__bases__ and change the concatenation approach to use unpacking).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/ui/_aiui_datastore.py`:
- Around line 28-38: Replace the runtime mutation of self.__class__.__bases__ in
PraisonAISessionDataStore with a lazy cached class factory: create a
module-level function (e.g., _get_praisonai_datastore_class) that imports
BaseDataStore, constructs a new subclass type("PraisonAISessionDataStoreImpl",
(BaseDataStore, PraisonAISessionDataStore), {}), and caches it (use
functools.lru_cache or a module-level variable plus threading.Lock for
thread-safety); then change PraisonAISessionDataStore.__new__ to call that
factory and return an instance of the created subclass (or have callers
instantiate the factory-returned class), removing any direct mutation of
__bases__ so isinstance checks, MRO, and static tools remain correct.
---
Nitpick comments:
In `@src/praisonai/praisonai/inc/models.py`:
- Around line 19-34: Create a small helper (e.g.,
is_module_available(module_name)) that calls
importlib.util.find_spec(module_name) inside a try/except catching ImportError,
AttributeError, ValueError (and ModuleNotFoundError if desired) and returns a
boolean, then replace the repeated guards for GOOGLE_GENAI_AVAILABLE,
ANTHROPIC_AVAILABLE, and COHERE_AVAILABLE with calls to that helper (and
optionally use it for OPENAI_AVAILABLE) so the logic is DRY and centralized
while preserving the original defensive behavior.
In `@src/praisonai/praisonai/ui/_aiui_datastore.py`:
- Line 33: Replace the tuple concatenation used to prepend BaseDataStore to the
current bases by using tuple unpacking instead; update the assignment to
self.__class__.__bases__ in the class-factory code so BaseDataStore is prepended
with tuple unpacking rather than by concatenating tuples (locate the assignment
to self.__class__.__bases__ and change the concatenation approach to use
unpacking).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c209b660-e124-4cda-b771-cbdca95ac46b
📒 Files selected for processing (2)
src/praisonai/praisonai/inc/models.pysrc/praisonai/praisonai/ui/_aiui_datastore.py
| # Lazy import dependencies only when class is instantiated | ||
| try: | ||
| from praisonaiui.datastore import BaseDataStore | ||
| # Make this class inherit from BaseDataStore at runtime | ||
| if BaseDataStore not in self.__class__.__bases__: | ||
| self.__class__.__bases__ = (BaseDataStore,) + self.__class__.__bases__ | ||
| except ImportError as e: | ||
| raise ImportError( | ||
| "praisonaiui is required for PraisonAISessionDataStore. " | ||
| "Install with: pip install 'praisonai[ui]'" | ||
| ) from e |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate BaseDataStore and inspect __init__ to see what setup is skipped.
fd -t f 'datastore*.py' | head -50
rg -nP -C5 '^\s*class\s+BaseDataStore\b'
rg -nP -C10 'class\s+BaseDataStore\b[\s\S]*?def\s+__init__'Repository: MervinPraison/PraisonAI
Length of output: 161
🏁 Script executed:
# View the full implementation of the file
cat -n src/praisonai/praisonai/ui/_aiui_datastore.pyRepository: MervinPraison/PraisonAI
Length of output: 5308
🏁 Script executed:
# Check the test file to understand actual usage
cat -n src/praisonai/tests/unit/test_aiui_datastore.pyRepository: MervinPraison/PraisonAI
Length of output: 10390
🏁 Script executed:
# Search for any references to BaseDataStore or praisonaiui in the repo
rg -i "basedatastore|praisonaiui" --type pyRepository: MervinPraison/PraisonAI
Length of output: 6814
🏁 Script executed:
# Check if there are any requirements or setup files mentioning praisonaiui
fd -t f -e txt -e cfg -e toml -e ini | xargs grep -l "praisonaiui" 2>/dev/null | head -20Repository: MervinPraison/PraisonAI
Length of output: 49
The runtime __class__.__bases__ mutation creates subtle hazards — refactor to a lazy class factory.
This lazy-inheritance pattern works but introduces several structural issues that warrant fixing:
- Global class object mutation.
self.__class__is the shared class object. After the first instantiation,PraisonAISessionDataStore.__bases__permanently includesBaseDataStore, and any instances created before that moment retroactively become subclasses. This meansisinstance(x, BaseDataStore)can flip fromFalse→Truedepending on construction order, which breaks equality checks and isinstance-based type validation in aiui's set_datastore(). - Not thread-safe. Concurrent first-instantiations race on the
not in __bases__check and the tuple assignment. If two threads call__init__simultaneously, the bases tuple can be corrupted or the check bypassed entirely. - Opaque to static tools. Type checkers,
inspect.getmro, and isinstance checks in aiui's internals run in contexts where the base isn't yet attached. This silences type errors and can cause runtime failures.
A lazy class factory avoids all of the above while preserving lazy-import intent:
♻️ Proposed refactor: cached class factory
+_impl_cls = None
+
+
+def _build_impl_cls():
+ global _impl_cls
+ if _impl_cls is not None:
+ return _impl_cls
+ try:
+ from praisonaiui.datastore import BaseDataStore
+ except ImportError as e:
+ raise ImportError(
+ "praisonaiui is required for PraisonAISessionDataStore. "
+ "Install with: pip install 'praisonai[ui]'"
+ ) from e
+ try:
+ from praisonaiagents.session import get_hierarchical_session_store
+ except ImportError as e:
+ raise ImportError(
+ "praisonaiagents is required for PraisonAISessionDataStore. "
+ "Install with: pip install praisonaiagents"
+ ) from e
+
+ class _PraisonAISessionDataStoreImpl(BaseDataStore):
+ def __init__(self, store: Optional[Any] = None):
+ super().__init__()
+ self._store = store or get_hierarchical_session_store()
+ # ... (move the rest of the methods here)
+
+ _impl_cls = _PraisonAISessionDataStoreImpl
+ return _impl_cls
+
+
class PraisonAISessionDataStore:
"""Adapter that bridges PraisonAI SessionStoreProtocol to aiui BaseDataStore.
- Note: This class defers dependency imports until instantiation to allow
- test collection without optional modules installed.
+ Dependency imports are deferred until instantiation to allow test
+ collection without optional modules installed.
"""
-
- def __init__(self, store: Optional[Any] = None):
- """Initialize with an optional session store, defaults to hierarchical store.
-
- Args:
- store: Optional SessionStoreProtocol implementation
- """
- # Lazy import dependencies only when class is instantiated
- try:
- from praisonaiui.datastore import BaseDataStore
- # Make this class inherit from BaseDataStore at runtime
- if BaseDataStore not in self.__class__.__bases__:
- self.__class__.__bases__ = (BaseDataStore,) + self.__class__.__bases__
- except ImportError as e:
- raise ImportError(
- "praisonaiui is required for PraisonAISessionDataStore. "
- "Install with: pip install 'praisonai[ui]'"
- ) from e
-
- try:
- from praisonaiagents.session import get_hierarchical_session_store
- except ImportError as e:
- raise ImportError(
- "praisonaiagents is required for PraisonAISessionDataStore. "
- "Install with: pip install praisonaiagents"
- ) from e
-
- self._store = store or get_hierarchical_session_store()
-
- def _new_id(self) -> str:
- """Generate a new session ID."""
- return str(uuid.uuid4())
-
- async def list_sessions(self) -> list[dict[str, Any]]:
- # ...
+ def __new__(cls, store: Optional[Any] = None):
+ impl = _build_impl_cls()
+ return impl(store)This ensures BaseDataStore.__init__ runs normally, isinstance checks are stable and thread-safe, and no class-level mutations happen.
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 33-33: Consider (BaseDataStore, *self.__class__.__bases__) instead of concatenation
Replace with (BaseDataStore, *self.__class__.__bases__)
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/ui/_aiui_datastore.py` around lines 28 - 38, Replace
the runtime mutation of self.__class__.__bases__ in PraisonAISessionDataStore
with a lazy cached class factory: create a module-level function (e.g.,
_get_praisonai_datastore_class) that imports BaseDataStore, constructs a new
subclass type("PraisonAISessionDataStoreImpl", (BaseDataStore,
PraisonAISessionDataStore), {}), and caches it (use functools.lru_cache or a
module-level variable plus threading.Lock for thread-safety); then change
PraisonAISessionDataStore.__new__ to call that factory and return an instance of
the created subclass (or have callers instantiate the factory-returned class),
removing any direct mutation of __bases__ so isinstance checks, MRO, and static
tools remain correct.
|
@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 completed the final architecture review and successfully implemented fixes for all critical issues identified by the reviewers. Todo List:
Phase 1: Architecture Review per AGENTS.md ✅✅ Protocol-driven: Changes follow AGENTS.md principles - optional dependencies are properly lazy-loaded, no heavy implementations in core SDK. ✅ Backward compatible: Zero feature regressions, all existing APIs work unchanged. ✅ Performance: No hot-path regressions - changes only affect test collection and startup path for optional modules. Phase 2: Critical Issues Fixed ✅✅ FIXED P1 - Thread-unsafe ✅ FIXED P1 - Silent initialization skip: Implementation class now properly calls ✅ FIXED Minor - DRY violation: Created Changes Made:src/praisonai/praisonai/ui/_aiui_datastore.py:
src/praisonai/praisonai/inc/models.py:
Test Results ✅:
Phase 3: Final Verdict ✅✅ APPROVED FOR MERGE All blocking issues identified by CodeRabbit, Greptile, and other reviewers have been resolved:
The PR successfully fixes the test collection failures from issue #1541 while addressing all architectural concerns raised by reviewers. The solution is now thread-safe, follows proper object-oriented patterns, and maintains excellent code quality. |
…refactor for models - Replace dangerous runtime __bases__ mutation with thread-safe lazy class factory - Ensure BaseDataStore.__init__ is properly called via super().__init__() - Add _is_module_available() helper to eliminate code duplication in models.py - Maintain lazy loading behavior for test collection compatibility - Fix critical thread safety and initialization issues identified by reviewers Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes test collection issues when optional dependencies are not installed in base setup.
Problem
Test collection was failing in base editable installs due to:
Solution
Testing
Follows AGENTS.md Guidelines
Fixes #1541
Generated with Claude Code
Summary by CodeRabbit