-
-
Notifications
You must be signed in to change notification settings - Fork 9
feat: optimize LLM batch processing and improve async handling #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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,19 @@ | ||
| import requests | ||
| import logging | ||
| import asyncio | ||
| from functools import partial | ||
| from generator.src.config import OLLAMA_MODEL | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # π₯ Limit concurrency | ||
| semaphore = asyncio.Semaphore(5) | ||
|
|
||
|
|
||
| def generate_text(prompt, model=None, url_override=None): | ||
| """Generate text from prompt using local Ollama.""" | ||
| target_model = model or OLLAMA_MODEL | ||
|
|
||
| # Use centralized config unless overridden | ||
|
|
||
| if url_override: | ||
| url = url_override | ||
| else: | ||
|
|
@@ -24,27 +29,28 @@ def generate_text(prompt, model=None, url_override=None): | |
| "num_ctx": 8192 | ||
| } | ||
| } | ||
|
|
||
| logger.info(f"Sending request to Ollama (model: {target_model}, prompt length: {len(prompt)} chars)") | ||
|
|
||
|
|
||
| try: | ||
| resp = requests.post(url, json=payload, timeout=600) | ||
| resp.raise_for_status() | ||
| except Exception as e: | ||
| logger.error(f"Error calling Ollama at {url}: {e}", exc_info=True) | ||
| return "" | ||
|
|
||
| try: | ||
| data = resp.json() | ||
| response_text = data.get("response", "") | ||
|
|
||
| logger.info(f"Received response from Ollama (length: {len(response_text)} chars)") | ||
| logger.info(f"Response done: {data.get('done', False)}") | ||
|
|
||
| if not response_text: | ||
| logger.warning(f"Empty response from Ollama. Full data: {data}") | ||
|
|
||
| return response_text | ||
| return data.get("response", "") | ||
| except Exception as e: | ||
| logger.error(f"Error parsing Ollama response: {e}", exc_info=True) | ||
| logger.error(f"LLM error: {e}", exc_info=True) | ||
| return "" | ||
|
|
||
|
|
||
| async def generate_text_async(prompt, model=None, url_override=None): | ||
| loop = asyncio.get_event_loop() | ||
| func = partial(generate_text, prompt, model, url_override) | ||
| return await loop.run_in_executor(None, func) | ||
|
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π§© Analysis chainπ Web query:
π‘ Result: Yes, asyncio.get_event_loop is deprecated in Python 3.10 when called from coroutines or callbacks without a running event loop, or in certain contexts where no current event loop exists. Instead, use asyncio.get_running_loop inside async functions (coroutines or callbacks), as it raises RuntimeError if no loop is running, ensuring safer usage. asyncio.get_running_loop (added in Python 3.7) returns the running event loop in the current OS thread and can only be called from a coroutine or callback. For top-level entry points, prefer asyncio.run(main), which handles loop creation and cleanup automatically, avoiding manual loop management altogether. Example migration: - Old (deprecated in async context): loop = asyncio.get_event_loop - New: loop = asyncio.get_running_loop Deprecation warnings appear in Python 3.10+ when get_event_loop is used improperly, and in later versions (e.g., 3.14+), it may raise RuntimeError if no current loop exists. Multiple projects (Langchain, Kingpin) have updated codebases to replace it with get_running_loop. Citations:
π Script executed: # Find project configuration files to determine Python version requirement
git ls-files | grep -E "(setup\.py|pyproject\.toml|requirements|tox\.ini|\.python-version)" | head -20Repository: AOSSIE-Org/LibrEd Length of output: 87 π Script executed: # Search for usages of generate_text_async to understand the calling context
rg "generate_text_async" --type py -B 2 -A 2Repository: AOSSIE-Org/LibrEd Length of output: 625 π Script executed: # Check the context around the function to see if there are any comments or tests
cat -n generator/src/llm_utils.py | head -60Repository: AOSSIE-Org/LibrEd Length of output: 1956 π Script executed: cat generator/requirements.txtRepository: AOSSIE-Org/LibrEd Length of output: 221 π Script executed: git ls-files | grep -E "setup\.(py|cfg)|pyproject\.toml|\.python-version"Repository: AOSSIE-Org/LibrEd Length of output: 43 π Script executed: git ls-files | grep -E "workflow|github|\.yml|\.yaml" | head -10Repository: AOSSIE-Org/LibrEd Length of output: 397 π Script executed: cat .github/workflows/ci.yml | head -50Repository: AOSSIE-Org/LibrEd Length of output: 1162 Use
Proposed fix async def generate_text_async(prompt, model=None, url_override=None):
- loop = asyncio.get_event_loop()
+ loop = asyncio.get_running_loop()
func = partial(generate_text, prompt, model, url_override)
return await loop.run_in_executor(None, func)π€ Prompt for AI Agents |
||
|
|
||
|
|
||
| async def controlled_generate_text(prompt): | ||
| async with semaphore: | ||
| return await generate_text_async(prompt) | ||
|
|
||
|
|
||
| async def process_batch(prompts): | ||
| tasks = [controlled_generate_text(p) for p in prompts] | ||
| return await asyncio.gather(*tasks, return_exceptions=True) | ||
|
Comment on lines
+54
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add docstring clarifying that exceptions are returned as values. With π Proposed fix with docstring async def process_batch(prompts):
+ """Process multiple prompts concurrently with limited concurrency.
+
+ Args:
+ prompts: List of prompt strings to process.
+
+ Returns:
+ List of results where each element is either:
+ - str: The generated text response
+ - Exception: If the request failed
+
+ Callers should check isinstance(result, Exception) to handle errors.
+ """
tasks = [controlled_generate_text(p) for p in prompts]
return await asyncio.gather(*tasks, return_exceptions=True)π€ Prompt for AI Agents |
||
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 | π΅ Trivial
Use
logger.exception()instead oflogger.error()withexc_info=True.Per static analysis hint,
logger.exception()is the idiomatic way to log exceptions with traceback. Also consider using%sformatting for lazy evaluation.β»οΈ Proposed fix
π Committable suggestion
π§° Tools
πͺ Ruff (0.15.6)
[warning] 39-39: Logging
.exception(...)should be used instead of.error(..., exc_info=True)(G201)
[warning] 39-39: Logging statement uses f-string
(G004)
π€ Prompt for AI Agents