Repository navigation
fix(wren-ai-service): Settings Loading and Multi-Pipeline Component Mechanism for Evaluation Framework - #1198
Conversation
WalkthroughThis pull request introduces significant refactoring of the Wren AI service's evaluation module, focusing on configuration management and code structure. The changes involve removing the Changes
Sequence DiagramsequenceDiagram
participant User
participant EvalSettings
participant Providers
participant Pipelines
participant Metrics
User->>EvalSettings: Initialize with configuration
EvalSettings-->>Providers: Generate components
Providers-->>Pipelines: Provide configured components
Pipelines->>Metrics: Pass engine_info
Metrics->>Metrics: Calculate evaluation metrics
Metrics-->>Pipelines: Return results
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
paopa
added
module/ai-service
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
wren-ai-service/eval/pipelines.py (2)
115-115: Confirm usage of ellipsis in abstract methodThe use of
...as the method body for_processin theEvalclass is acceptable in Python 3.5 and above for abstract methods. Ensure that the Python version used in the project supports this syntax. Alternatively, consider usingpassfor clarity.Apply this diff if you prefer using
pass:- def _process(self, prediction: dict, **_) -> dict: ... + def _process(self, prediction: dict, **_) -> dict: + pass
368-377: Update parameter names ininitfunctionIn the
initfunction, consider renaming the parametercomponentstopipe_componentsfor consistency with the rest of the codebase, as seen in the classes wherepipe_componentsis used. This enhances readability and reduces confusion.Apply this diff:
-def init( name: Literal["retrieval", "generation", "ask"], meta: dict, mdl: dict, components: Dict[str, Any], settings: EvalSettings, ) -> Eval: args = { "meta": meta, "mdl": mdl, - "components": components, + "pipe_components": components, "settings": settings, }Ensure that all calls to
initand usage ofargswithin the function are updated accordingly.wren-ai-service/eval/metrics/faithfulness.py (2)
10-13: Consider maintaining private attribute convention.The removal of the underscore prefix from
_engine_configsuggests this is now intended to be a public attribute. If this is meant to be an implementation detail, consider keeping it as a private attribute with the underscore prefix.- self.engine_info = engine_info + self._engine_info = engine_info
20-20: Update attribute reference to match constructor.If keeping the private attribute convention, update the attribute reference here as well.
- sql=test_case.actual_output, **self.engine_info + sql=test_case.actual_output, **self._engine_infowren-ai-service/eval/evaluation.py (1)
85-85: Consider adding error details to the result.The error handling could be improved by including error details in the test result.
- result = evaluate([test_case], self._metrics, ignore_errors=True).test_results[0] + try: + result = evaluate([test_case], self._metrics, ignore_errors=True).test_results[0] + except Exception as e: + result = TestResult(test_case=test_case, error=str(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ai-service/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
wren-ai-service/eval/.env.example(0 hunks)wren-ai-service/eval/.gitignore(1 hunks)wren-ai-service/eval/__init__.py(1 hunks)wren-ai-service/eval/evaluation.py(3 hunks)wren-ai-service/eval/metrics/accuracy.py(3 hunks)wren-ai-service/eval/metrics/answer_relevancy.py(1 hunks)wren-ai-service/eval/metrics/context_recall.py(1 hunks)wren-ai-service/eval/metrics/faithfulness.py(1 hunks)wren-ai-service/eval/pipelines.py(11 hunks)wren-ai-service/eval/prediction.py(3 hunks)wren-ai-service/eval/utils.py(8 hunks)wren-ai-service/pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- wren-ai-service/eval/.env.example
✅ Files skipped from review due to trivial changes (1)
- wren-ai-service/eval/.gitignore
🔇 Additional comments (18)
wren-ai-service/eval/prediction.py (3)
16-16: Ensure correct import path forEvalSettingsThe import statement
from eval import EvalSettingsmight not resolve if theevalmodule is not correctly recognized as a package. IfEvalSettingsis defined in the__init__.pyfile within theevaldirectory, ensure that theevaldirectory has an__init__.pyfile to make it a package. Alternatively, adjust the import statement to reflect the correct module structure.
24-25: Update calls togenerate_metato match new parameterThe
generate_metafunction has been updated to acceptsettings: EvalSettingsinstead ofllm_providerandembedder_provider. Ensure that all calls to this function are updated accordingly to pass thesettingsparameter. This change centralizes configuration management but requires careful updates to all function invocations.
28-43:⚠️ Potential issueVerify that all
settingsattributes exist inEvalSettingsThe code accesses several attributes from
settings, such as:
settings.langfuse_urlsettings.column_indexing_batch_sizesettings.table_retrieval_sizesettings.table_column_retrieval_sizesettings.batch_sizesettings.batch_intervalsettings.datasourceEnsure that all these attributes are defined within the
EvalSettingsclass or inherited from the baseSettingsclass. Missing attributes will result in anAttributeErrorat runtime.Run the following script to verify the existence of these attributes:
✅ Verification successful
All required settings attributes are properly defined
The code correctly accesses all required attributes:
- Direct attributes in
EvalSettings:batch_size,batch_interval,datasource- Property in
EvalSettings:langfuse_url- Inherited from base
Settingsclass:column_indexing_batch_size,table_retrieval_size,table_column_retrieval_size🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all required attributes exist in EvalSettings. # Test: List all attributes of EvalSettings and check for the required ones. ast-grep --pattern $'class EvalSettings(Settings) { $$$ }' wren-ai-service/eval/__init__.py | grep -E 'langfuse_url|column_indexing_batch_size|table_retrieval_size|table_column_retrieval_size|batch_size|batch_interval|datasource'Length of output: 232
Script:
#!/bin/bash # Find the relevant Python files echo "=== Finding relevant files ===" fd -e py "settings|__init__" eval # View the content of files in eval directory that might contain settings echo -e "\n=== Checking content of potential settings files ===" for f in $(fd -e py "settings|__init__" eval); do echo -e "\n=== Content of $f ==="; cat "$f"; done # Search for the attributes across all Python files in eval echo -e "\n=== Searching for settings attributes ===" rg -A 1 "class .*Settings" eval/ rg "(langfuse_url|column_indexing_batch_size|table_retrieval_size|table_column_retrieval_size|batch_size|batch_interval|datasource)\s*[:=]" eval/Length of output: 822
Script:
#!/bin/bash # Check settings class definitions echo "=== Settings Class Definitions ===" rg -A 5 "class .*Settings" wren-ai-service/ # Check settings attributes echo -e "\n=== Settings Attributes ===" rg "(langfuse_url|column_indexing_batch_size|table_retrieval_size|table_column_retrieval_size|batch_size|batch_interval|datasource)\s*[=:]" wren-ai-service/ # View the specific settings file content echo -e "\n=== Settings File Content ===" find wren-ai-service -type f -name "settings.py" -o -name "__init__.py" | while read -r file; do echo -e "\n--- $file ---" cat "$file" doneLength of output: 69957
wren-ai-service/eval/pipelines.py (3)
15-15: Verify correct import ofEvalSettingsSimilar to
prediction.py, ensure that the importfrom eval import EvalSettingscorrectly locates theEvalSettingsclass. Confirm that theevalpackage is properly recognized, and adjust the import path if necessary.
33-33: Update parameter type indeploy_modelfunctionThe
deploy_modelfunction's parameterpipehas changed fromindexing.Indexingtoindexing.DBSchema. Ensure that theDBSchemaclass provides all the methods and attributes used withindeploy_model. This change should be verified to prevent any unexpected behavior.
172-188:⚠️ Potential issueEnsure all
settingsattributes are defined and used correctlyWithin the
RetrievalPipelineclass, several attributes fromsettingsare used:
settings.column_indexing_batch_sizesettings.table_retrieval_sizesettings.table_column_retrieval_sizesettings.allow_using_db_schemas_without_pruningVerify that these attributes are defined in
EvalSettingsor inherited fromSettings. Missing attributes will cause runtime errors.wren-ai-service/eval/__init__.py (2)
5-9:⚠️ Potential issueEnsure all required settings are included in
EvalSettingsThe
EvalSettingsclass defines several attributes. Confirm that all required settings used throughout the codebase are included here or in the baseSettingsclass, including:
column_indexing_batch_sizetable_retrieval_sizetable_column_retrieval_sizeallow_using_db_schemas_without_pruningIf these attributes are missing, accessing them will result in
AttributeError.
12-15:⚠️ Potential issueVerify the existence of
langfuse_hostinEvalSettingsThe property
langfuse_urlusesself.langfuse_host, butlangfuse_hostis not defined withinEvalSettings. Ensure thatlangfuse_hostis defined in the baseSettingsclass or add it toEvalSettings.wren-ai-service/eval/metrics/context_recall.py (1)
10-13: Same changes as in FaithfulnessMetric.The changes mirror those in
FaithfulnessMetric. Consider applying the same private attribute convention here.Also applies to: 20-20
wren-ai-service/eval/metrics/answer_relevancy.py (1)
10-13: Same changes as in other metric classes.The changes mirror those in other metric classes. Consider applying the same private attribute convention here.
Also applies to: 20-20, 24-24
wren-ai-service/eval/evaluation.py (4)
16-18: LGTM! Clean import changes.The imports reflect the transition to the new settings loading mechanism and provider structure.
146-148: LGTM! Clean settings initialization.The initialization of settings and components aligns with the new architecture.
155-156: LGTM! Consistent use of engine_info.The use of
engine_infois consistent with the changes in metric classes.
160-172: Remove or update commented code.The commented-out code related to the training dataset functionality should either be:
- Removed if it's no longer needed
- Updated to use the new settings mechanism
- Tracked as a TODO item if it needs to be addressed later
Let's check if this code is referenced elsewhere:
wren-ai-service/eval/metrics/accuracy.py (2)
15-15: LGTM! Constructor parameter and instance variable names improved.The renaming from
engine_configtoengine_infoand removal of underscore prefixes improves naming consistency and clarity while maintaining the same functionality.Also applies to: 18-19
20-20: Verify semantics comparison behavior remains unchanged.The semantics comparison logic has been updated to use the new variable name. While the changes look correct, please ensure that all test cases for semantic comparison still pass.
Also applies to: 148-148
✅ Verification successful
Semantics comparison behavior verified to be unchanged
The semantics comparison remains an optional feature that acts as a fallback mechanism in accuracy metrics, only triggering when exact matches fail. The variable rename is consistent across the codebase and maintains the same behavior pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests that cover semantics comparison rg -l "enable_semantics_comparison.*True" -g "test_*.py"Length of output: 58
Script:
#!/bin/bash # Find test files related to accuracy and semantics echo "=== Test files with 'accuracy' ===" fd "test.*accuracy.*\.py$" echo -e "\n=== Any usage of semantics comparison in tests ===" rg "semantics" -g "test_*.py" -A 2 -B 2 echo -e "\n=== Implementation of semantics comparison ===" rg "enable_semantics_comparison" -A 5 -B 5Length of output: 18892
wren-ai-service/eval/utils.py (1)
217-219: Verify engine attributes are always set.The code assumes
_source,_endpoint, and_connection_infoattributes exist on the engine object. Add validation to prevent potential AttributeErrors.wren-ai-service/pyproject.toml (1)
59-60: LGTM! Dependencies properly organized.Moving
deepevalto eval dependencies and addingstreamlit-tagsaligns with the evaluation framework's scope. The versions are pinned appropriately.
71e57c9 to
4a0d2be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wren-ai-service/eval/prediction.py (1)
113-118: Consider adding type hints for pipe_components.While the code is functional, adding type hints for pipe_components would improve code maintainability and IDE support.
- components=pipe_components, + components: Dict[str, Any]=pipe_components,wren-ai-service/eval/evaluation.py (1)
159-172: Document the reason for commenting out training dataset code.The code block for handling training datasets has been commented out. Consider adding a TODO comment explaining why this functionality is temporarily disabled and when it will be re-enabled.
- # if args.training_dataset: + # TODO(TICKET-123): Training dataset functionality temporarily disabled pending dspy integration + # if args.training_dataset:wren-ai-service/eval/pipelines.py (1)
178-180: Consider extracting configuration constants.The column_batch_size configuration could be moved to a constants file for better maintainability.
+# In constants.py +DEFAULT_COLUMN_BATCH_SIZE = 100 + +# In current file _indexing = indexing.DBSchema( **pipe_components["db_schema_indexing"], - column_batch_size=settings.column_indexing_batch_size, + column_batch_size=settings.column_indexing_batch_size or DEFAULT_COLUMN_BATCH_SIZE, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ai-service/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
wren-ai-service/eval/.env.example(0 hunks)wren-ai-service/eval/.gitignore(1 hunks)wren-ai-service/eval/__init__.py(1 hunks)wren-ai-service/eval/evaluation.py(3 hunks)wren-ai-service/eval/metrics/accuracy.py(3 hunks)wren-ai-service/eval/metrics/answer_relevancy.py(1 hunks)wren-ai-service/eval/metrics/context_recall.py(1 hunks)wren-ai-service/eval/metrics/faithfulness.py(1 hunks)wren-ai-service/eval/pipelines.py(11 hunks)wren-ai-service/eval/prediction.py(4 hunks)wren-ai-service/eval/utils.py(8 hunks)wren-ai-service/pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- wren-ai-service/eval/.env.example
🚧 Files skipped from review as they are similar to previous changes (8)
- wren-ai-service/eval/.gitignore
- wren-ai-service/eval/init.py
- wren-ai-service/eval/metrics/context_recall.py
- wren-ai-service/eval/metrics/answer_relevancy.py
- wren-ai-service/eval/metrics/faithfulness.py
- wren-ai-service/pyproject.toml
- wren-ai-service/eval/utils.py
- wren-ai-service/eval/metrics/accuracy.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pytest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
wren-ai-service/eval/prediction.py (2)
111-111: LGTM! Clean parameter update.The generate_meta call has been correctly updated to use the settings parameter.
104-107: Verify error handling for settings initialization.The code initializes settings and generates components without any error handling. Consider adding try-catch blocks to handle potential initialization failures gracefully.
wren-ai-service/eval/evaluation.py (2)
146-148: LGTM! Clean initialization sequence.The settings initialization and langfuse setup follow a logical order.
155-156: LGTM! Proper configuration update.The engine configuration has been correctly updated to use pipe_components.
wren-ai-service/eval/pipelines.py (1)
185-188: LGTM! Comprehensive retrieval configuration.The retrieval configuration properly includes all necessary parameters from settings.
| _indexing = indexing.DBSchema( | ||
| **pipe_components["db_schema_indexing"], | ||
| column_batch_size=settings.column_indexing_batch_size, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication in pipeline initialization.
The initialization code in AskPipeline duplicates the pattern from RetrievalPipeline. Consider extracting this common initialization logic into a base class or utility function.
def init_db_schema(components: Dict[str, Any], settings: EvalSettings) -> indexing.DBSchema:
return indexing.DBSchema(
**components["db_schema_indexing"],
column_batch_size=settings.column_indexing_batch_size,
)
def init_retrieval(components: Dict[str, Any], settings: EvalSettings) -> retrieval.Retrieval:
return retrieval.Retrieval(
**components["db_schema_retrieval"],
table_retrieval_size=settings.table_retrieval_size,
table_column_retrieval_size=settings.table_column_retrieval_size,
allow_using_db_schemas_without_pruning=settings.allow_using_db_schemas_without_pruning,
)Also applies to: 298-301
4a0d2be to
cfb1d7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
wren-ai-service/eval/evaluation.py (1)
Add test coverage for EvalSettings and its integration
The codebase lacks test coverage for the
EvalSettingsclass and its integration with the evaluation pipeline. While the baseSettingsclass has tests, you should add tests for:
EvalSettingsconfiguration validation- Settings integration with component initialization
- Error handling for invalid settings
🔗 Analysis chain
Line range hint
1-177: Verify test coverage for the new settings mechanism.The changes look good overall, but ensure there are unit tests covering:
- The new settings loading mechanism
- Component initialization failures
- Various evaluation scenarios with the new pipeline structure
Let's check for existing test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test files fd -e py "test.*evaluation" . # Check for EvalSettings tests rg -i "class.*test.*evalsettings" .Length of output: 69
Script:
#!/bin/bash # Look for any test files in common test directories fd -e py . tests/ test/ unit_tests/ integration_tests/ 2>/dev/null # Search for any test files containing 'eval' or 'settings' rg -l "class.*Test" -g "*.py" | rg -i "eval|settings" # Search for imports or usage of EvalSettings rg "EvalSettings" -A 3 -B 3Length of output: 4240
Script:
#!/bin/bash # Check for tests of the parent Settings class rg "class.*Test.*Settings" -g "*.py" # Look for any test files importing Settings rg "from src.config import Settings" -g "*test*.py" # Check the base Settings class implementation rg "class Settings" -A 5 -B 5Length of output: 835
🧹 Nitpick comments (3)
wren-ai-service/eval/evaluation.py (1)
146-148: Consider adding error handling for component initialization.The new initialization sequence using
EvalSettingsand component generation looks good, but consider adding error handling for cases where component initialization fails.settings = EvalSettings() +try: pipe_components = provider.generate_components(settings.components) utils.init_langfuse(settings) +except Exception as e: + print(f"Failed to initialize components: {e}") + sys.exit(1)Also applies to: 155-156
wren-ai-service/eval/pipelines.py (2)
178-180: Extract common initialization logic.The initialization pattern is duplicated across pipeline classes. Consider extracting this into utility functions.
def init_db_schema(components: Dict[str, Any], settings: EvalSettings) -> indexing.DBSchema: return indexing.DBSchema( **components["db_schema_indexing"], column_batch_size=settings.column_indexing_batch_size, ) def init_retrieval(components: Dict[str, Any], settings: EvalSettings) -> retrieval.Retrieval: return retrieval.Retrieval( **components["db_schema_retrieval"], table_retrieval_size=settings.table_retrieval_size, table_column_retrieval_size=settings.table_column_retrieval_size, allow_using_db_schemas_without_pruning=settings.allow_using_db_schemas_without_pruning, )Also applies to: 185-188
390-392: Add type hints for engine_info parameter.The
engine_infoparameter lacks type hints in themetrics_initiatorfunction.- engine_info: dict, + engine_info: Dict[str, Any],Also applies to: 396-396, 398-398, 400-400
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ai-service/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
wren-ai-service/eval/.env.example(0 hunks)wren-ai-service/eval/.gitignore(1 hunks)wren-ai-service/eval/__init__.py(1 hunks)wren-ai-service/eval/evaluation.py(3 hunks)wren-ai-service/eval/metrics/accuracy.py(3 hunks)wren-ai-service/eval/metrics/answer_relevancy.py(1 hunks)wren-ai-service/eval/metrics/context_recall.py(1 hunks)wren-ai-service/eval/metrics/faithfulness.py(1 hunks)wren-ai-service/eval/pipelines.py(11 hunks)wren-ai-service/eval/prediction.py(4 hunks)wren-ai-service/eval/utils.py(8 hunks)wren-ai-service/pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- wren-ai-service/eval/.env.example
🚧 Files skipped from review as they are similar to previous changes (8)
- wren-ai-service/eval/.gitignore
- wren-ai-service/pyproject.toml
- wren-ai-service/eval/init.py
- wren-ai-service/eval/metrics/answer_relevancy.py
- wren-ai-service/eval/metrics/faithfulness.py
- wren-ai-service/eval/metrics/context_recall.py
- wren-ai-service/eval/utils.py
- wren-ai-service/eval/metrics/accuracy.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (python)
- GitHub Check: pytest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ai-service/eval/evaluation.py (3)
16-17: LGTM! Import changes align with the new settings mechanism.The transition from environment variables to the new
EvalSettingsclass is reflected in these import changes.
85-87: LGTM! Improved result handling with better object-oriented approach.The change to access
test_resultsattribute instead of direct index access is more robust and maintainable.
160-172: 🛠️ Refactor suggestionRemove commented-out code and track it properly.
Large blocks of commented code can lead to maintenance issues and confusion. Instead:
- If this is temporary, create a ticket to track the re-implementation of the dspy functionality
- If this code is no longer needed, remove it entirely
- If this is for reference, move it to documentation
Let's check if this dspy-related code is referenced elsewhere:
wren-ai-service/eval/prediction.py (2)
24-24: LGTM! Improved configuration management.The refactoring of
generate_metato useEvalSettingscentralizes configuration management and improves code maintainability.Also applies to: 28-43
111-111: LGTM! Clean pipeline initialization.The pipeline initialization is now more streamlined with the new settings-based approach.
Also applies to: 113-118
wren-ai-service/eval/pipelines.py (2)
371-377: LGTM! Clean argument packaging.The argument packaging for pipeline initialization is clean and maintainable.
224-225: Consider validating component configuration.The generation component initialization lacks validation for required configuration keys.
Also applies to: 228-229
| settings = EvalSettings() | ||
| # todo: handle duckdb as datasource | ||
| pipe_components = provider.generate_components(settings.components) | ||
| utils.init_langfuse(settings) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify error handling for settings initialization.
The settings initialization and component generation lack error handling. Consider adding try-catch blocks to handle potential configuration errors gracefully.
- settings = EvalSettings()
- # todo: handle duckdb as datasource
- pipe_components = provider.generate_components(settings.components)
- utils.init_langfuse(settings)
+ try:
+ settings = EvalSettings()
+ pipe_components = provider.generate_components(settings.components)
+ utils.init_langfuse(settings)
+ except Exception as e:
+ print(f"Failed to initialize settings: {e}")
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wren-ai-service/eval/__init__.py (1)
8-9: Add validation for batch processing parameters.Consider adding validation to ensure batch_size and batch_interval are within reasonable bounds to prevent performance issues.
- batch_size: int = 4 - batch_interval: int = 1 + batch_size: int = Field(default=4, gt=0, le=10) + batch_interval: int = Field(default=1, gt=0, le=5)wren-ai-service/eval/data_curation/app.py (3)
70-72: Improve error message for model mismatch.The assertion error message could be more helpful by including the actual model details.
- assert doc["mdl"] == st.session_state["mdl_json"], ( - "The model in the uploaded dataset is different from the deployed model" - ) + assert doc["mdl"] == st.session_state["mdl_json"], ( + f"Model mismatch - Uploaded: {doc['mdl'].get('name', 'unknown')} " + f"Deployed: {st.session_state['mdl_json'].get('name', 'unknown')}" + )
345-345: Standardize error message formatting.The error messages in the UI should follow a consistent format. Currently, some use f-strings directly while others use get() with default values.
Standardize the error handling:
- f"SQL is invalid: {st.session_state['llm_question_sql_pairs'][i]['error']}" + f"SQL is invalid: {st.session_state['llm_question_sql_pairs'][i].get('error', 'Unknown error')}"Also applies to: 424-424
482-482: Use UTC for timestamp in filename.When generating filenames with timestamps, it's better to use UTC to avoid timezone-related issues.
- f"eval_dataset_{datetime.today().strftime('%Y_%m_%d')}.toml", + f"eval_dataset_{datetime.utcnow().strftime('%Y_%m_%d')}.toml",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ai-service/eval/__init__.py(1 hunks)wren-ai-service/eval/data_curation/app.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ai-service/eval/__init__.py (2)
12-12: LGTM! Secure handling of API key.Good use of SecretStr for the API key and proper implementation of the getter method.
Also applies to: 20-21
7-7: Consider validation for langfuse_project_id.The empty string default for langfuse_project_id combined with the silent failure in langfuse_url might mask configuration issues. Consider adding validation or logging when the project ID is missing.
Also applies to: 16-18
wren-ai-service/eval/data_curation/app.py (1)
42-43: LGTM! Proper initialization of settings and client.Good integration of the new EvalSettings class for managing the OpenAI API key.
| sys.path.append(f"{Path().parent.resolve()}") | ||
| from eval import EvalSettings |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid modifying sys.path.
Modifying sys.path directly is not recommended. Consider restructuring the project to use proper Python packaging.
Consider one of these alternatives:
- Add an empty init.py in the parent directory to make it a package
- Install the package in development mode using pip
- Use PYTHONPATH environment variable instead
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ai-service/eval/pipelines.py (2)
178-180: Extract common initialization logic to reduce duplication.The initialization of
DBSchemais duplicated betweenRetrievalPipelineandAskPipeline. Consider extracting this into a utility function:def init_db_schema(components: Dict[str, Any], settings: EvalSettings) -> indexing.DBSchema: return indexing.DBSchema( **components["db_schema_indexing"], column_batch_size=settings.column_indexing_batch_size, )Also applies to: 291-293
185-188: Extract retrieval initialization logic to reduce duplication.Similar to the
DBSchemainitialization, theRetrievalinitialization is also duplicated. Consider extracting this into a utility function:def init_retrieval(components: Dict[str, Any], settings: EvalSettings) -> retrieval.Retrieval: return retrieval.Retrieval( **components["db_schema_retrieval"], table_retrieval_size=settings.table_retrieval_size, table_column_retrieval_size=settings.table_column_retrieval_size, allow_using_db_schemas_without_pruning=settings.allow_using_db_schemas_without_pruning, )Also applies to: 298-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)wren-ai-service/eval/pipelines.py(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (4)
wren-ai-service/eval/pipelines.py (4)
15-30: LGTM! Clean import organization.The imports are well-organized and properly reflect the transition to the new settings loading mechanism.
204-210: LGTM! Consistent metrics implementation.The metrics implementation is now more consistent across all pipeline classes, using
engine_infouniformly. The commented-out metrics for the spider dataset are properly documented.Also applies to: 266-274, 344-358
371-377: LGTM! Clean initialization structure.The common initialization structure using a dictionary of arguments is clean and maintainable.
321-322: Verify the impact of empty SQL samples.The
sql_samplesparameter is passed as an empty list. Verify if this is intentional and whether it affects the generation quality.✅ Verification successful
Empty SQL samples list is acceptable in evaluation context
The empty SQL samples list is intentionally used in the evaluation pipeline and the codebase is designed to handle this case properly. The generation pipelines accept this parameter as optional and include proper conditional handling in their templates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of sql_samples to understand the expected values rg -A 2 "sql_samples"Length of output: 4058
Overview
This Pull Request introduces significant updates to the evaluation framework, focusing on transitioning to a new settings loading mechanism and implementing a multi-pipeline component structure. Additionally, it includes modifications to accommodate changes in the
deepevalinterface.Key Changes
Settings Management:
LANGFUSE_PROJECT_ID,OPENAI_API_KEY,WREN_IBIS_ENDPOINT,WREN_ENGINE_ENDPOINT,WREN_IBIS_TIMEOUT,BATCH_SIZE,BATCH_INTERVAL,DATA_SOURCE,bigquery.project-id,bigquery.dataset-id, andbigquery.credentials-keyfrom the.env.examplefile.EvalSettingsclass in the__init__.pyfile to manage settings such as langfuse project ID, batch size, batch interval, datasource, config path, and langfuse URL.Multi-Pipeline Component Mechanism:
Interface Changes:
deepevalinterface to reflect the new settings and multi-pipeline component mechanism.Metric Classes Updates:
AccuracyMetric,AnswerRelevancyMetric,ContextualRecallMetric, andFaithfulnessMetricclasses to useengine_infoinstead ofengine_config.Code Cleanup:
Summary by CodeRabbit
Configuration Changes
.env.examplefile with environment variable configurations..gitignoreto includeconfig.yamlanddeepeval_telemtry.txt.Dependency Updates
deepevalfrom dev dependencies to evaluation dependencies.streamlit-tagsto evaluation dependencies.Evaluation Improvements
EvalSettingsclass for centralized configuration management.Code Refactoring
engine_configtoengine_info.