-
Notifications
You must be signed in to change notification settings - Fork 4
fix: Resolve reranking variable mismatch and enable config overrides (Issue #465) #476
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
…(Issue #465) This commit fixes three critical issues preventing reranking from working: ## 1. Fix Variable Name Mismatch in Reranker (PRIMARY FIX) - **Problem**: Reranking scores always 0.0 because LLM prompts had unfilled placeholders - **Root cause**: Reranker used "document" variable, WatsonX provider expected "context" - **Fix**: Changed variable name from "document" to "context" in reranker.py - **Impact**: Reranking now generates valid scores (0.0-1.0 range) - **Files**: backend/rag_solution/retrieval/reranker.py:140,174 ## 2. Fix Backend Startup Error (BLOCKING BUG) - **Problem**: Backend failed to start with "'dict' object has no attribute 'model_dump'" - **Root cause**: system_initialization_service.py passed dict instead of Pydantic model - **Fix**: Convert LLMProviderInput to LLMProviderUpdate before passing to update_provider() - **Impact**: Backend starts successfully, hot-reload works - **Files**: backend/rag_solution/services/system_initialization_service.py:106-107 ## 3. Enable Per-Request Reranking Config (FEATURE ENHANCEMENT) - **Problem**: No way to enable/disable reranking per search request - **Root cause**: Config metadata not passed to _apply_reranking method - **Fix**: Added config_metadata parameter to enable per-request reranking control - **Impact**: Tests can now compare with/without reranking using config_metadata - **Files**: backend/rag_solution/services/search_service.py:239-262,695-700,939-944 ## Test Results **Before Fix:** - Reranking scores: always 0.0 - Backend startup: failed with Pydantic error - Config override: not supported **After Fix:** - Reranking scores: 0.0-1.0 range (e.g., top score changed from 0.6278 to 0.8000) - Backend startup: successful with hot-reload - Config override: enabled via config_metadata.enable_reranking ## Test Artifacts Added two test scripts: 1. **test_reranking_impact.py**: Demonstrates reranking effect by comparing scores 2. **create_reranking_template.py**: Creates RERANKING template in database ## Database Change Created reranking template directly in database: ```sql INSERT INTO prompt_templates (...) VALUES ('Default Reranking Template', 'RERANKING', ...) ``` ## Known Limitation Reranking can only improve ranking of chunks that are already retrieved. If the embedding model doesn't retrieve relevant chunks in initial top_k, reranking cannot bring them in. This is a separate embedding model quality issue. ## Related Issues Closes #465 (reranking not working, scores always 0.0) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🚀 Development Environment OptionsThis repository supports Dev Containers for a consistent development environment. Option 1: GitHub Codespaces (Recommended)Create a cloud-based development environment:
Option 2: VS Code Dev Containers (Local)Use Dev Containers on your local machine:
Option 3: Traditional Local SetupSet up the development environment manually: # Clone the repository
git clone https://github.com/manavgup/rag_modulo.git
cd rag_modulo
git checkout fix/issue-465-reranking-fix
# Initialize development environment
make dev-init
make dev-build
make dev-up
make dev-validateAvailable CommandsOnce in your development environment: make help # Show all available commands
make dev-validate # Validate environment setup
make test-atomic # Run atomic tests
make test-unit # Run unit tests
make lint # Run lintingServices AvailableWhen running
This automated message helps reviewers quickly set up the development environment. |
Code Review for PR #476Thank you for this well-documented PR! The fixes address critical issues in the reranking functionality. Strengths
Issues & Recommendations1. Critical: Hardcoded User Path in Test ScriptFile: backend/dev_tests/test_reranking_impact.py:20 Hardcoded personal directory path that won't work for other developers. Fix: Make path relative or remove since TEST_FILE is unused. 2. Missing Unit Tests for New FunctionalityFile: backend/rag_solution/services/search_service.py:239-262 The new config_metadata parameter in _apply_reranking() is not covered by unit tests. Add tests for:
3. Import Placement Anti-PatternFile: backend/rag_solution/services/system_initialization_service.py:106 Import statement inside a function rather than at module level. Violates PEP 8 and impacts performance. Move import to top of file with other schema imports. 4. Hardcoded User UUID in Test ScriptsFiles: backend/dev_tests/create_reranking_template.py:7, backend/dev_tests/test_reranking_impact.py:26 Use environment variable with fallback or add comment explaining this is a specific test user. Architecture & DesignPositive:
SecurityNo security concerns identified - no credential leaks, proper UUID validation, localhost only. PerformanceNo performance issues - minimal overhead, simple dict.get() lookup. Action Items Before Merge (Required)
SummaryOverall Assessment: Good work with minor issues This PR successfully fixes the critical reranking bug. The code changes are correct and minimal. Fix the 3 items above, then approve and merge. The fixes are solid and address the root cause effectively! 🤖 Review by Claude Code |
Summary
This PR fixes Issue #465 by resolving three critical issues that prevented reranking from working:
Changes
1. Fix Variable Name Mismatch in Reranker
{document}placeholder"document"variable, WatsonX provider expected"context""document"to"context"inreranker.pybackend/rag_solution/retrieval/reranker.py:140,1742. Fix Backend Startup Error
'dict' object has no attribute 'model_dump'system_initialization_service.pypassed dict instead of Pydantic modelLLMProviderInputtoLLMProviderUpdatebefore passing toupdate_provider()backend/rag_solution/services/system_initialization_service.py:106-1073. Enable Per-Request Reranking Config
_apply_rerankingmethodconfig_metadataparameter to enable per-request reranking controlconfig_metadatabackend/rag_solution/services/search_service.py:239-262,695-700,939-944Test Results
Before Fix:
After Fix:
config_metadata.enable_rerankingEvidence:
Test Artifacts
Added two test scripts:
Database Change
Created reranking template directly in database:
Known Limitation
Reranking can only improve the ranking of chunks that are already retrieved. If the embedding model doesn't retrieve relevant chunks in the initial
top_k, reranking cannot bring them in. This is a separate embedding model quality issue that will be addressed in subsequent issues.Related Issues
Closes #465
Checklist
🤖 Generated with Claude Code