-
Notifications
You must be signed in to change notification settings - Fork 8.2k
feat(unified_models): implement lazy loading for individual model classes #11051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduced a per-provider lazy loading mechanism for model classes via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
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 |
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (39.25%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11051 +/- ##
==========================================
- Coverage 33.11% 33.10% -0.01%
==========================================
Files 1389 1389
Lines 65714 65727 +13
Branches 9730 9735 +5
==========================================
+ Hits 21760 21761 +1
- Misses 42836 42848 +12
Partials 1118 1118
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lfx/src/lfx/base/models/unified_models.py (2)
22-48: Good lazy loading implementation, but consider adding type hints and error handling.The per-provider lazy loading is well-implemented and achieves the stated goals. However, consider these improvements:
- Missing return type annotation: Add a return type hint for better type safety.
- Import error handling: When an optional dependency is missing, the bare import will raise
ImportError. Consider wrapping imports in try-except blocks to provide clearer error messages about missing packages.Apply this diff to add type hints:
-def get_model_class(class_name: str): +def get_model_class(class_name: str) -> type | None: """Lazy load a specific model class to avoid importing unused dependencies.Consider adding error handling for optional dependencies:
def get_model_class(class_name: str) -> type | None: """Lazy load a specific model class to avoid importing unused dependencies. This imports only the requested provider, not all providers at once. """ if class_name == "ChatOpenAI": - from langchain_openai import ChatOpenAI - - return ChatOpenAI + try: + from langchain_openai import ChatOpenAI + return ChatOpenAI + except ImportError as e: + msg = f"Cannot import {class_name}. Install langchain-openai: pip install langchain-openai" + raise ImportError(msg) from e if class_name == "ChatAnthropic": - from langchain_anthropic import ChatAnthropic - - return ChatAnthropic + try: + from langchain_anthropic import ChatAnthropic + return ChatAnthropic + except ImportError as e: + msg = f"Cannot import {class_name}. Install langchain-anthropic: pip install langchain-anthropic" + raise ImportError(msg) from e # ... similar pattern for other providers
60-66: Consider restoring@lru_cachefor consistency with similar utility functions.While
get_model_classes()does import all provider packages (already documented in the docstring), restoring the@lru_cache(maxsize=1)decorator would align with the pattern used byget_embedding_classes()andget_model_provider_metadata()in the same module. This is a minor optimization since the function has no parameters and is called infrequently (once per batch operation in batch_run.py), but caching the resulting dict avoids recreating it on repeated invocations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lfx/src/lfx/base/models/unified_models.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lfx/src/lfx/base/models/unified_models.py (1)
src/lfx/src/lfx/base/models/google_generative_ai_model.py (1)
ChatGoogleGenerativeAIFixed(4-38)
⏰ 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). (2)
- GitHub Check: Update Starter Projects
- GitHub Check: Update Component Index
🔇 Additional comments (4)
src/lfx/src/lfx/base/models/unified_models.py (4)
50-57: Clean internal registry for model class names.This list provides a clear registry of available model classes and keeps them in sync with
get_model_class().
902-907: Excellent lazy loading implementation inget_llm().This change is the key improvement that enables lazy loading. By calling
get_model_class(model_class_name)instead ofget_model_classes().get(...), the function now imports only the specific provider needed rather than all providers upfront.The error handling is appropriate, with a clear ValueError raised when the model class cannot be loaded.
297-300: Appropriate use of pragma comments for false-positive suppression.The
# pragma: allowlist secretcomments correctly suppress false-positive secret detection on variable and parameter names that contain "api_key" or similar strings. These are configuration keys, not actual secrets.Also applies to: 654-654, 665-665, 679-679, 855-855
755-860: Note: Inconsistency with AI summary.The AI summary states that
normalize_model_names_to_dicts()was updated to useget_model_classinstead ofget_model_classes(), but no changes are marked in this function. Upon inspection, this function doesn't appear to directly call either function—it usesget_unified_models_detailed()and static mappings instead.This doesn't indicate a code issue, but there may be a discrepancy in the summary.
Summary
get_model_class(class_name)function for loading specific providers on demandChanges
get_model_class(class_name)function that imports only the requested providerget_llm()to use the new lazy loading functionget_model_classes()now uses the individual loader internallyBenefits
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.