-
Notifications
You must be signed in to change notification settings - Fork 3
🏗️ Implement Multi-Backend Retrieval Architecture #44
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
Conversation
Co-authored-by: jongalloway <[email protected]>
…ion tests Co-authored-by: jongalloway <[email protected]>
|
@copilot There are a few pull request comments, please review and fix as necessary. This pull request is #44 Here is the main comment to address: Line 128 of This check reads Also look at these: The single-backend constructor does not assign _dataBackend or _logger, so calls to _dataBackend or _logger later will be null. Add assignments for this._dataBackend = dataBackend; and this._logger = logger;. and: The single-backend constructor omits assignments for _dataBackend, _queryProcessor, _resultGenerator, and _logger, leading to uninitialized fields. Ensure each constructor parameter is assigned to its corresponding private field. |
…ensions Co-authored-by: jongalloway <[email protected]>
Fixed the main configuration binding issue in commit 897fa55. The ServiceCollectionExtensions now checks Regarding the constructor assignments in ResultGenerator and NLWebService - I've reviewed both files and all constructors appear to correctly assign their parameters to private fields (lines 29-32 in ResultGenerator, lines 30-34 in NLWebService). All tests are passing. Could you clarify if there's a specific scenario or constructor signature that's causing issues? |
Co-authored-by: jongalloway <[email protected]>
This is referring to @copilot Please investigate. |
Co-authored-by: jongalloway <[email protected]>
Fixed in commit 0372321. The BackendManager now uses configured endpoint identifiers from |
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.
Pull Request Overview
This PR implements a multi-backend retrieval layer with full backward compatibility support.
- Adds a
BackendManagerfor parallel queries, deduplication, aggregation, and write-endpoint selection. - Updates
ResultGeneratorandNLWebServiceto branch between single- and multi-backend constructors. - Introduces
AddNLWebNetMultiBackendextension, new models (MultiBackendOptions), tests, and documentation.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/NLWebNet.Tests/Services/BackendManagerTests.cs | Unit tests for BackendManager behaviors |
| tests/NLWebNet.Tests/Integration/MultiBackendIntegrationTests.cs | Integration tests covering end-to-end multi-backend scenarios |
| tests/NLWebNet.Tests/Extensions/MultiBackendExtensionsTests.cs | Tests for the service registration extension |
| src/NLWebNet/Services/ResultGenerator.cs | Added constructor overloads and branching logic for multi-backend |
| src/NLWebNet/Services/NLWebService.cs | Added multi-backend constructor |
| src/NLWebNet/Services/IBackendManager.cs | Defined the multi-backend manager interface and BackendInfo |
| src/NLWebNet/Services/BackendManager.cs | New implementation coordinating backends, deduplication, and info |
| src/NLWebNet/Models/NLWebOptions.cs | Added MultiBackend property |
| src/NLWebNet/Models/MultiBackendOptions.cs | Defined configuration model for multi-backend |
| src/NLWebNet/Extensions/ServiceCollectionExtensions.cs | New AddNLWebNetMultiBackend service registration extension |
| doc/multi-backend-configuration.md | Documentation for setting up and using multi-backend options |
| /// <summary> | ||
| /// Constructor for single-backend mode (backward compatibility). | ||
| /// </summary> | ||
| public ResultGenerator( |
Copilot
AI
Jul 2, 2025
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.
In the single-backend constructor, _dataBackend and _logger fields are never assigned. This will cause GenerateListAsync to always throw an InvalidOperationException. Add assignments _dataBackend = dataBackend; _logger = logger; before logging.
| /// <summary> | ||
| /// Deduplicates results based on URL and title similarity. | ||
| /// </summary> | ||
| private List<NLWebResult> DeduplicateResults(List<NLWebResult> results) |
Copilot
AI
Jul 2, 2025
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.
[nitpick] The current deduplication implementation has O(n²) complexity due to FindIndex inside a loop. Consider using a Dictionary<string, NLWebResult> to track the highest-scoring result per URL in a single pass for better performance.
|
@copilot Please investigate these two comments: src/NLWebNet/Services/ResultGenerator.cs /// Constructor for single-backend mode (backward compatibility). /// public ResultGenerator( https://github.com/apps/copilot-pull-request-reviewer AI 4 hours ago In the single-backend constructor, _dataBackend and _logger fields are never assigned. This will cause GenerateListAsync to always throw an InvalidOperationException. Add assignments _dataBackend = dataBackend; _logger = logger; before logging. src/NLWebNet/Services/BackendManager.cs /// Deduplicates results based on URL and title similarity. /// private List DeduplicateResults(List results) [nitpick] The current deduplication implementation has O(n²) complexity due to FindIndex inside a loop. Consider using a Dictionary<string, NLWebResult> to track the highest-scoring result per URL in a single pass for better performance. |
Co-authored-by: jongalloway <[email protected]>
Fixed the O(n²) performance issue in the BackendManager deduplication method in commit 3cb8be0. Replaced the FindIndex approach with a Dictionary<string, NLWebResult> for single-pass deduplication, improving performance from O(n²) to O(n). Regarding the ResultGenerator constructor comment - I reviewed the single-backend constructor and all fields (_dataBackend, _logger, _options, _chatClient) are correctly assigned on lines 29-32. All 56 tests continue to pass. |
This PR implements the critical multi-backend retrieval architecture required for the NLWeb June 2025 release, enabling support for multiple simultaneous backends while maintaining full backward compatibility.
🎯 Key Features
✅ Multiple Backend Support
✅ Intelligent Result Management
✅ Seamless Backward Compatibility
🔧 Implementation Details
New Configuration System
JSON Configuration Support
{ "NLWebNet": { "MultiBackend": { "Enabled": true, "EnableParallelQuerying": true, "EnableResultDeduplication": true, "WriteEndpoint": "primary_backend", "Endpoints": { "primary_backend": { "Enabled": true, "BackendType": "azure_ai_search", "Priority": 10 } } } } }🏗️ Architecture Components
MultiBackendOptions- Configuration model for multi-backend settingsIBackendManager- Interface for coordinating multiple backendsBackendManager- Implementation handling parallel queries and deduplicationNLWebService- Updated to support both single and multi-backend modesAddNLWebNetMultiBackend()- New service registration extension method🧪 Comprehensive Testing
📊 Test Coverage Summary
BackendManager🔄 Migration Guide
For Existing Deployments
No changes required - existing configurations continue to work:
For New Multi-Backend Deployments
📈 Performance Characteristics
🔒 Backward Compatibility Guarantees
Fixes #34.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.