-
Notifications
You must be signed in to change notification settings - Fork 0
Working version radu #1
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
…stable one) to not have 10K+ changes
- add parsing for special chars - add links to actual PDP page - format response in a friendly UI
…al endpoints - Add nlweb site configuration, disable tool_selection and who_endpoint - Add nlweb site definition
…pping tool selection, auto-select search tool with score 100
…hen who_endpoint_enabled is false, use sites from config
…web, extract fields from source_url and schema_org
-Set default site to 'nlweb', mode to 'summarize', db to 'qdrant_url' -Update default values in state initialization
|
From any of this changes do you see anything that can cause problems when deploying to an app service on azure ? |
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 configures the application for furniture search functionality using the nlweb site with Qdrant vector database, defaulting to "summarize" mode for improved user experience with formatted responses and product links.
- Switches default configuration from Azure/Bing to Qdrant URL endpoint for nlweb furniture database
- Updates UI to default to "summarize" mode with enhanced formatting for product summaries
- Adds content formatter for cleaning HTML entities and extracting product links from search results
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| static/index.html | Sets default parameters for nlweb site with summarize mode |
| static/common-chat-styles.css | Adds CSS for formatted summaries and product links display |
| static/chat-ui-common.js | Implements markdown parsing and product links rendering |
| static/chat-interface-unified.js | Changes defaults to nlweb/summarize and adds mode change notifications |
| start_server_debug.py | Fixes server class name capitalization |
| config/sites.xml | Adds nlweb site configuration for furniture database |
| config/config_retrieval.yaml | Switches from local to URL-based Qdrant endpoint |
| config/config_nlweb.yaml | Configures nlweb site and disables tool/who endpoints |
| config/config_llm.yaml | Updates API version and model references |
| config/config_embedding.yaml | Updates API version to use environment variable |
| code/python/webserver/routes/api.py | Maps UI "mode" parameter to backend "generate_mode" |
| code/python/testing/connectivity/check_connectivity.py | Adds dotenv import (commented out) |
| code/python/testing/check_connectivity.py | Adds dotenv import (commented out) |
| code/python/retrieval_providers/qdrant.py | Updates result formatting for nlweb data structure |
| code/python/methods/multi_site_query.py | Adds support for disabled who endpoint with direct site configuration |
| code/python/core/utils/content_formatter.py | New formatter for cleaning HTML entities and extracting product links |
| code/python/core/router.py | Auto-selects search tool for summarize/generate modes |
| code/python/core/retriever.py | Adds "retrieval" parameter support for endpoint selection |
| code/python/core/ranking.py | Skips individual result sending in summarize/generate modes |
| code/python/core/post_ranking.py | Enhances summary formatting with product links |
| code/python/core/baseHandler.py | Improves mode parameter handling from UI |
| code/python/app-file.py | Adds commented dotenv import |
| code/python/app-aiohttp.py | Adds commented dotenv import |
| UI_MOCKUP.md | Documentation of UI improvements |
| SUMMARY_UI_IMPROVEMENTS.md | Detailed summary of UI enhancement changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
config/config_retrieval.yaml
Outdated
| api_key_env: QDRANT_API_KEY | ||
| # Set the collection name to use | ||
| index_name: nlweb_collection | ||
| index_name: QDRANT_COLLECTION |
Copilot
AI
Oct 28, 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.
The index_name is set to 'QDRANT_COLLECTION' which appears to be an environment variable placeholder rather than an actual collection name. This should either reference an environment variable using the index_name_env key, or be set to a literal collection name like 'nlweb_collection'.
| index_name: QDRANT_COLLECTION | |
| index_name_env: QDRANT_COLLECTION |
| schema_org = payload.get("schema_org", {}) | ||
| if isinstance(schema_org, dict): | ||
| # If it's already a dict, convert to JSON string | ||
| import json |
Copilot
AI
Oct 28, 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.
Import statements should be placed at the top of the file, not inside a method. Move this import to the module-level imports section.
| import json |
| @@ -0,0 +1,136 @@ | |||
| import re | |||
Copilot
AI
Oct 28, 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.
The file contains a UTF-8 BOM (Byte Order Mark) character at the beginning. This invisible character can cause syntax errors in Python. Remove the BOM by re-saving the file with UTF-8 encoding without BOM.
| import re | |
| import re |
| 'query': self.query | ||
| } | ||
|
|
||
|
|
Copilot
AI
Oct 28, 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.
Extra blank line should be removed to maintain consistent code formatting.
code/python/core/post_ranking.py
Outdated
| @@ -21,12 +21,15 @@ async def do(self): | |||
| await self.check_and_send_map_message() | |||
|
|
|||
| if (self.handler.generate_mode == "none"): | |||
Copilot
AI
Oct 28, 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 comment '# nothing to do' was removed. While the code is clear, the comment provided useful documentation about the intent when generate_mode is 'none'. Consider restoring it for clarity.
| if (self.handler.generate_mode == "none"): | |
| if (self.handler.generate_mode == "none"): | |
| # nothing to do |
|
From any of this changes do you see anything that can cause problems when deploying to an app service on azure ? |
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
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
start_server_debug.py
Outdated
| """Run server with debug output.""" | ||
| print("Starting server with debug output...") | ||
| server = AioHttpServer(CONFIG) | ||
| server = AioHTTPServer() |
Copilot
AI
Oct 28, 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.
The AioHTTPServer constructor is being called without arguments, but the original code passed CONFIG as a parameter. This change may cause initialization issues if the server class expects configuration to be provided during instantiation.
| server = AioHTTPServer() | |
| server = AioHTTPServer(CONFIG) |
| schema_org = payload.get("schema_org", {}) | ||
| if isinstance(schema_org, dict): | ||
| # If it's already a dict, convert to JSON string | ||
| import json |
Copilot
AI
Oct 28, 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.
The import json statement is placed inside a method rather than at the module level. This should be moved to the top of the file with other imports for better performance and code organization.
| import json |
| else: | ||
| # If it's already a JSON string, use as-is | ||
| schema = str(schema_org) | ||
| name = payload.get("text", "").split("|")[0].replace("Name: ", "").strip() if payload.get("text") else "" |
Copilot
AI
Oct 28, 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.
This complex one-liner with chained string operations and conditionals is difficult to read and maintain. Consider breaking this into multiple lines with intermediate variables for clarity.
| name = payload.get("text", "").split("|")[0].replace("Name: ", "").strip() if payload.get("text") else "" | |
| text_value = payload.get("text", "") | |
| if text_value: | |
| name_part = text_value.split("|")[0] | |
| name = name_part.replace("Name: ", "").strip() | |
| else: | |
| name = "" |
|
|
||
|
|
Copilot
AI
Oct 28, 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] Empty line serves no purpose in the code flow. Remove this blank line to improve code cleanliness.
…lt formatting product → schema_json (with JSON conversion)
No description provided.