Rest api #20
Merged
Rest api #20
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Enhances data loading by including additional documentation paths and correcting source URIs for improved context. Refines the agent's system role to emphasize the use of the GitHub Knowledgebase and external tools for accurate information retrieval, preventing fabricated responses. Adds chat history to the agent for persistent conversations. Addresses embedding request failures with retry mechanism and rate limit handling. Moves memgraph.sh to the scripts directory and updates its path resolution.
Introduces a configuration file for data loaders to improve maintainability and reduce code duplication. Moves hardcoded paths and URI replacements into configurable objects, making the scripts more flexible and easier to manage. Adds tests for the new configuration structure and loading logic.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors loader scripts to use centralized configuration classes, improves embedding retry logic, enhances agent session history handling, and adds corresponding tests.
- Introduces
LoaderConfigandWebLoaderConfiginscripts/loader_config.pyand updatesdoc_loader.py/url_loader.pyto use them. - Updates
_make_embedding_requestretry logic to split the 60s wait into two intervals and adds cumulative retry error handling. - Enhances
src/agent.pywith query history tracking and prints history; adds tests for loader configurations.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/loader_config.py | New dataclasses for loader configuration and constants. |
| scripts/doc_loader.py | Uses DOC_LOADER_CONFIGS and applies uri_replacement. |
| scripts/url_loader.py | Uses WEB_LOADER_CONFIGS and applies uri_replacement. |
| src/core/rag/embedder/init.py | Refactored _make_embedding_request retry/backoff logic. |
| src/core/rag/embedder/text_embedding_3_small.py | Removed unused uuid import. |
| src/agent.py | Added history state, updated process_query to use it. |
| scripts/memgraph.sh | Adjusted working directory path. |
| tests/test_scripts_loader_config.py | New tests for loader config classes and URI logic. |
| tests/test_core_rag_embedder.py | Updated tests to validate the new retry sleep calls. |
Comments suppressed due to low confidence (2)
src/agent.py:82
- assistant_message could be None if there are no choices in the response, leading to an AttributeError. Add a guard to ensure assistant_message is not None before calling get().
result = assistant_message.get("content", "")
scripts/memgraph.sh:10
- This cd command assumes a 'docker' directory at PROJECT_ROOT, but memgraph.sh was moved to 'scripts'. Confirm the 'docker' folder's location or update this path to match the current layout.
cd "$PROJECT_ROOT/docker"
| Today is {date.today().strftime("%d %B %Y")}. | ||
| """ | ||
|
|
||
| self.history = [ |
There was a problem hiding this comment.
The new history tracking in process_query isn't covered by existing tests. Please add unit tests to verify that history is correctly updated when requests succeed or fail.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces several enhancements and refactorings across multiple files to improve configurability, error handling, and code organization. Key changes include the addition of configuration classes for loader scripts, updates to retry logic in embedding requests, and improvements to the agent's query handling. New tests have also been added to validate the configuration and logic changes.
Configuration Management Enhancements:
LoaderConfigandWebLoaderConfigdataclasses inscripts/loader_config.pyto centralize configuration for document and web loaders. IntroducedDOC_LOADER_CONFIGSandWEB_LOADER_CONFIGSto store predefined configurations.scripts/doc_loader.pyandscripts/url_loader.pyto use the new configuration classes, enabling dynamic loading paths and URI replacements. [1] [2]Embedding Retry Logic Improvements:
async def _make_embedding_requestwithinsrc/core/rag/embedder/__init__.pyto handle retries more robustly. Reduced initial wait time to 5 seconds and added a cumulative wait for 429 errors.tests/test_core_rag_embedder.pyto validate the new retry behavior, ensuring proper handling of multiple sleep intervals.Agent Behavior Refinements:
src/agent.pyto maintain a query history for better context management during interactions. Added logic to append user and assistant messages to the history and print it for debugging.Testing Additions:
tests/test_scripts_loader_config.pyto validate the structure and logic ofDOC_LOADER_CONFIGSandWEB_LOADER_CONFIGS, including URI replacement behavior and configuration integrity.File Renaming and Directory Adjustments:
doc-loader.py→doc_loader.py,url-loader.py→url_loader.py,docker/memgraph.sh→scripts/memgraph.sh) and updated paths accordingly. [1] [2] [3]