-
Notifications
You must be signed in to change notification settings - Fork 0
Add EmbeddingClient to openai.py with detailed logging and retries #10
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: Mte90 <403283+Mte90@users.noreply.github.com>
|
@copilot il file di esempio non serve |
- Remove analyzer_embedding_usage_example.py (not needed as per feedback) - Import and instantiate EmbeddingClient in analyzer.py - Replace get_embedding_for_text calls with _embedding_client.embed_text - Pass file_path and chunk_index for detailed logging - EmbeddingClient now actively used for all embedding operations Co-authored-by: Mte90 <403283+Mte90@users.noreply.github.com>
|
@copilot così hai lasciato la funzione get_embedding_for_text dentro openai inutilizzata, non voglio un file per embedding voglio quel codice dentro il file openai.py |
- Move EmbeddingClient class and EmbeddingError from embedding_client.py to openai.py - Update analyzer.py to import EmbeddingClient from openai module - Delete ai/embedding_client.py (no longer needed) - All embedding functionality now consolidated in openai.py - get_embedding_for_text remains in openai.py (can be used for backward compatibility) Co-authored-by: Mte90 <403283+Mte90@users.noreply.github.com>
ai/analyzer.py
Outdated
| """ | ||
| Wrapper to acquire semaphore inside executor task to avoid deadlock. | ||
| The semaphore is acquired in the worker thread, not the main thread. | ||
| Now uses EmbeddingClient for better logging and error handling. |
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.
@copilot line useless
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.
Rimosso. Commit: c9310f1
Fatto. Ho spostato tutto il codice di EmbeddingClient dentro openai.py e rimosso il file embedding_client.py. Ora tutte le funzionalità di embedding sono consolidate in un unico modulo. Commit: e73edcd |
ai/openai.py
Outdated
| EMBEDDING_API_URL = os.getenv("PICOCODE_EMBEDDING_URL", CFG.get("api_url", "https://example.com/v1/embeddings")) | ||
| EMBEDDING_API_KEY = os.getenv("PICOCODE_EMBEDDING_API_KEY", CFG.get("api_key", "")) | ||
| DEFAULT_TIMEOUT = float(os.getenv("PICOCODE_EMBEDDING_TIMEOUT", "30")) # seconds per request | ||
| MAX_RETRIES = int(os.getenv("PICOCODE_EMBEDDING_RETRIES", "2")) | ||
| BACKOFF_FACTOR = float(os.getenv("PICOCODE_EMBEDDING_BACKOFF", "1.5")) | ||
| EMBEDDING_MODEL_NAME = os.getenv("PICOCODE_EMBEDDING_MODEL", DEFAULT_EMBEDDING_MODEL or "text-embedding-3-small") |
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.
@copilot this lines doesn't make any sense this settings already exits check the config files.
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.
Corretto. EmbeddingClient ora usa direttamente i valori da CFG (API_URL, API_KEY, EMBEDDING_MODEL) invece di duplicarli. Solo timeout, retries e backoff rimangono come override via environment. Commit: c9310f1
- Remove duplicate config settings (EMBEDDING_API_URL, EMBEDDING_API_KEY, EMBEDDING_MODEL_NAME) - EmbeddingClient now uses CFG values directly for api_url, api_key, and model - Remove unnecessary comments in analyzer.py - Keep only timeout, retries, and backoff as environment-configurable overrides Co-authored-by: Mte90 <403283+Mte90@users.noreply.github.com>
ai/openai.py
Outdated
| DEFAULT_TIMEOUT = float(os.getenv("PICOCODE_EMBEDDING_TIMEOUT", "30")) # seconds per request | ||
| MAX_RETRIES = int(os.getenv("PICOCODE_EMBEDDING_RETRIES", "2")) | ||
| BACKOFF_FACTOR = float(os.getenv("PICOCODE_EMBEDDING_BACKOFF", "1.5")) | ||
|
|
||
| # Optionally enable requests debug logging by setting PICOCODE_HTTP_DEBUG=true | ||
| if os.getenv("PICOCODE_HTTP_DEBUG", "").lower() in ("1", "true", "yes"): | ||
| logging.getLogger("requests").setLevel(logging.DEBUG) | ||
| logging.getLogger("urllib3").setLevel(logging.DEBUG) |
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.
@copilot I don't want this variables
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.
Rimosso. I parametri timeout, max_retries e backoff ora hanno valori predefiniti hard-coded (30.0, 2, 1.5) direttamente nel metodo init. Commit: f28a7ef
- Remove DEFAULT_TIMEOUT, MAX_RETRIES, BACKOFF_FACTOR constants - Remove PICOCODE_HTTP_DEBUG environment variable handling - Use hard-coded defaults directly in EmbeddingClient.__init__ (timeout=30.0, max_retries=2, backoff=1.5) - Simpler code with no extra configuration layer Co-authored-by: Mte90 <403283+Mte90@users.noreply.github.com>
|
@copilot get_embedding_for_text what is the purpose now of this function with this new class? there is stuff unused after that change? |
La funzione get_embedding_for_text è ancora utilizzata in ai/llama_integration.py (linea 31). Attualmente abbiamo due approcci:
Vuoi che converta anche llama_integration.py per usare EmbeddingClient e rimuova get_embedding_for_text? |
|
@copilot yes I want that you use that class for everything and remove the useless code |
…_text - Update llama_integration.py to use EmbeddingClient instead of get_embedding_for_text - Create module-level _embedding_client instance in llama_integration.py - Remove unused get_embedding_for_text function from openai.py - All embedding operations now use EmbeddingClient for consistent logging and retry behavior Co-authored-by: Mte90 <403283+Mte90@users.noreply.github.com>
Fatto. Ho convertito llama_integration.py per usare EmbeddingClient e rimosso get_embedding_for_text da openai.py. Ora tutte le operazioni di embedding (analyzer, search, llama_integration) usano EmbeddingClient con logging consistente e retry logic. Commit: d9ea51d |
|
@copilot check the code for the function you removed if they are unused that code can be removed, but has just a pass and doing nothing |
Ho verificato il codice:
|
|
@copilot I don't have want anymore get_embedding_for_text if there is a class that replaced. this is code duplication and you should be smart and check to organize better the code or I wlitllr ash this pr and start again |
Mi scuso per la confusione nella mia risposta precedente. Ho verificato nuovamente: get_embedding_for_text è già stata completamente rimossa nel commit d9ea51d. Non esiste più nel codice - ho confermato con grep che non ci sono riferimenti. Nella mia risposta precedente ho controllato erroneamente le funzioni helper (_retry_with_backoff, _check_rate_limit, etc) che get_embedding_for_text usava, ma quelle sono ancora necessarie per call_coding_api. Situazione attuale:
Il codice è pulito e organizzato come richiesto. |
Adds a synchronous requests-based EmbeddingClient class to
ai/openai.pywith structured logging, configurable timeouts, and retry logic with exponential backoff to improve debugging of embedding failures. The EmbeddingClient replaces all existing embedding implementations across the codebase for consistent behavior.Changes
Modified files:
ai/openai.py- Added EmbeddingClient class:embed_text()for single chunks,embed_multiple()for batch processingget_embedding_for_text()function (replaced by EmbeddingClient)ai/analyzer.py- Integrated EmbeddingClient:_get_embedding_with_semaphore()to use EmbeddingClient with file_path and chunk_index parameterssearch_semantic()to use EmbeddingClient for query embeddingsai/llama_integration.py- Integrated EmbeddingClient:llama_index_retrieve_documents()to use EmbeddingClient for query embeddingsBenefits
The integration provides:
Configuration
Uses standard .env configuration:
The EmbeddingClient uses sensible hard-coded defaults (timeout=30s, max_retries=2, backoff=1.5) that work well for most cases. These can be overridden by passing parameters when instantiating the client if needed.
All embedding operations (analyzer, search, llama_integration) benefit from enhanced logging and retry capabilities, using the standard configuration from .env.
Original prompt
Create a pull request that adds a robust EmbeddingClient wrapper and a small usage example to the repository to make embedding failures easier to debug and to provide configurable timeouts/retries.
Files to add (exact contents below):
Contents: