Enhancement: Improve MVP Stability by Adding Configurable Florence Batching with OOM Fallback and Finalizing Singleton Service Wiring - #29
Conversation
- Update code to use model.pt (matches OmniParser release) - Add .gitignore patterns for model weight files - Remove *.rst from .gitignore - Add Prerequisites section to README with download instructions - Fix verify_model_load.py to use model.pt Sub-issue of MVP demo preparation. Fixes FileNotFoundError for contributors.
- generate captions per detected element - added validate_image size/type before inference
There was a problem hiding this comment.
Pull request overview
This PR improves MVP stability by moving heavyweight services (OmniParser, RAG KB, Heuristic Engine) to app-level singletons, and adds configurable Florence caption batching with adaptive retry on probable OOM errors. It also adds dev tooling to download required weights and start the stack more consistently across platforms.
Changes:
- Initialize and reuse OmniParser, RAGKnowledgeBase, and HeuristicEvaluationEngine via
app.statesingletons. - Add configurable Florence batching (
FLORENCE_BATCH_SIZE) with adaptive batch halving on probable OOM. - Add cross-platform dev launcher and OmniParser weight setup helper; update docs and ignores for local weights.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| start-dev.py | New cross-platform dev launcher that preps weights and starts backend (+ optional firebase-functions). |
| scripts/setup_omniparser.py | New helper to download/lay out OmniParser v2 weights into weights/. |
| requirements.txt | Pins/extends ML stack deps (torch/transformers/numpy/einops/huggingface-hub/accelerate). |
| main.py | Startup now wires OmniParser/RAG/Heuristic Engine as singletons with clearer logs. |
| app/utils/transformers_patches.py | Adds a transformers monkey-patch to avoid flash-attn import checks on non-CUDA. |
| app/services/omniparser_client.py | Adds Florence batching + adaptive OOM fallback; adjusts model initialization and validation. |
| app/services/heuristic_engine.py | Makes RAG KB a constructor dependency and avoids reinitialization when already initialized. |
| app/core/config.py | Adds FLORENCE_BATCH_SIZE setting sourced from env with validation. |
| app/api/routes/heuristic.py | Routes now reuse the singleton OmniParser client. |
| app/api/routes/evaluation.py | Routes now reuse singleton engine and pass upload content_type into detection. |
| README.md | Updates quick start and documents weight setup paths and options. |
| .gitignore | Stops ignoring docs, and starts ignoring weights + local firebase config files. |
| .env.example | Documents FLORENCE_BATCH_SIZE env var. |
Comments suppressed due to low confidence (2)
app/services/omniparser_client.py:9
- Unused imports:
asyncioandjsonare imported but not referenced anywhere in this module. Removing them avoids confusion and keeps dependencies clear.
import asyncio
from typing import List, Dict, Any, Optional, Tuple
from PIL import Image
import io
import json
app/services/omniparser_client.py:361
detect_elementsstill exposes animage_urlparameter, but the implementation only processesimage_databytes and never fetches/usesimage_url. As a result, callers likedetect_elements(image_url=...)will always fail validation (no image bytes). Either implement URL fetching insidedetect_elements(and validate content-type/size), or removeimage_urlfrom the API and update the routes accordingly.
async def detect_elements(
self,
image_data: bytes,
image_url: Optional[str] = None,
content_type: str = "image/jpeg"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ).to(device) | ||
|
|
||
| self.logger.info("✅ Florence model loaded") | ||
| self.processor = AutoProcessor.from_pretrained("microsoft/Florence-2-base-ft", trust_remote_code=True) |
There was a problem hiding this comment.
The caption model is loaded from the local snapshot (weights/icon_caption_florence), but the processor is loaded from Hugging Face ("microsoft/Florence-2-base-ft"). This introduces an unexpected runtime network dependency and risks processor/model mismatch. Consider loading the processor from the same local directory (or ensure setup_omniparser downloads the processor artifacts and point AutoProcessor to the local path).
| self.processor = AutoProcessor.from_pretrained("microsoft/Florence-2-base-ft", trust_remote_code=True) | |
| self.processor = AutoProcessor.from_pretrained( | |
| "weights/icon_caption_florence", | |
| trust_remote_code=True, | |
| ) |
| from ultralytics import YOLO | ||
| import torch | ||
| from transformers import AutoProcessor, AutoModelForCausalLM | ||
| import re # For cleaning Florence output |
There was a problem hiding this comment.
Unused import: re is imported but not used (caption cleaning is done with .replace(...)). Either remove it or use it for the intended cleaning logic.
| import re # For cleaning Florence output |
| # FLORENCE CAPTIONING - Makes content DYNAMIC | ||
| element_content = "" | ||
|
|
There was a problem hiding this comment.
element_content is assigned but never read (captions are written directly into elements[element_index].content inside the batch captioner). Removing this variable and related dead assignments will make the captioning flow easier to follow.
| client = request.app.state.omniparser_client | ||
|
|
||
| contents = await image.read() | ||
| result = await client.detect_elements(contents) |
There was a problem hiding this comment.
analyze_interface calls detect_elements(contents) without passing the upload content_type. Since detect_elements now validates content_type, this endpoint will always use the default ("image/jpeg"), which can incorrectly accept unsupported types (or reject supported ones if defaults change). Pass content_type=image.content_type (with a safe fallback) to keep validation consistent with /evaluation/evaluate.
| result = await client.detect_elements(contents) | |
| content_type = image.content_type or "image/jpeg" | |
| result = await client.detect_elements(contents, image.filename, content_type=content_type) |
| try: | ||
| uvicorn_process.wait() | ||
| except KeyboardInterrupt: | ||
| shutdown_handler(None, None) |
There was a problem hiding this comment.
The launcher only waits on the Uvicorn process. If Uvicorn exits/crashes, the script will exit without terminating firebase_process, leaving it running in the background. Consider wrapping the wait loop in a try/finally (or monitoring both processes) to ensure all child processes are terminated and to propagate the backend exit code.
| self.rag_kb = RAGKnowledgeBase() | ||
| await self.rag_kb.initialize() | ||
| if self.rag_kb is None: | ||
| raise RuntimeError("RAGKnowledgeBase dependency missing. Inject via startup.") |
There was a problem hiding this comment.
initialize() now raises if rag_kb wasn’t injected. This is a breaking behavior change for any direct HeuristicEvaluationEngine() usage (e.g., the existing integration test instantiates it without dependencies). If standalone construction should remain supported, consider defaulting to a new RAGKnowledgeBase() internally (or provide a clear factory/helper used by tests) rather than raising at runtime.
| raise RuntimeError("RAGKnowledgeBase dependency missing. Inject via startup.") | |
| self.logger.warning( | |
| "RAGKnowledgeBase dependency was not injected; creating a default instance." | |
| ) | |
| self.rag_kb = RAGKnowledgeBase() |
| await app.state.rag_knowledge_base.initialize() | ||
| logger.info("RAG knowledge base initialized (singleton)") | ||
|
|
||
| # Initilize Hueristic Evaluation Engine (Singleton) |
There was a problem hiding this comment.
Comment typo: "Initilize Hueristic" should be "Initialize Heuristic" for clarity/professionalism in startup logs/comments.
| # Initilize Hueristic Evaluation Engine (Singleton) | |
| # Initialize Heuristic Evaluation Engine (Singleton) |
| detection_client = request.app.state.omniparser_client | ||
| contents = await image.read() | ||
|
|
||
| detection_result = await detection_client.detect_elements(contents) | ||
|
|
||
| # Initialize evaluation engine and evaluate | ||
| evaluation_engine = HeuristicEvaluationEngine() | ||
| await evaluation_engine.initialize() | ||
| detection_result = await detection_client.detect_elements( | ||
| contents, | ||
| content_type=content_type | ||
| ) | ||
|
|
||
| # Use singleton evaluation engine and evaluate | ||
| evaluation_engine = request.app.state.heuristic_engine | ||
| evaluation_result = await evaluation_engine.evaluate_interface(detection_result) |
There was a problem hiding this comment.
detect_elements(..., content_type=content_type) can raise InvalidInputError (and OmniParser errors), but this handler currently falls through to the generic except Exception at the bottom of the endpoint, returning HTTP 500 for client errors. Add explicit except InvalidInputError / service-exception branches in this endpoint so invalid content types and validation failures return 400/422/503 as intended.
| @@ -0,0 +1,54 @@ | |||
| import os | |||
| import sys | |||
| import shutil | |||
There was a problem hiding this comment.
Unused import: shutil is imported but never used in this script. Removing it keeps the setup script minimal.
| import shutil |
| # if model exists, do nothing | ||
| safetensors_path = os.path.join(target_caption_folder, "model.safetensors") | ||
| if os.path.exists(safetensors_path): | ||
| print(f"✅ Models found in {target_caption_folder}. Skipping download.") |
There was a problem hiding this comment.
The early-exit check only verifies weights/icon_caption_florence/model.safetensors, so the script may skip even if weights/icon_detect/model.pt is missing (or the caption folder is incomplete). Consider validating both required weight sets (detector + caption) before skipping, so start-dev.py reliably produces a runnable environment.
| # if model exists, do nothing | |
| safetensors_path = os.path.join(target_caption_folder, "model.safetensors") | |
| if os.path.exists(safetensors_path): | |
| print(f"✅ Models found in {target_caption_folder}. Skipping download.") | |
| # if both caption and detector models exist, do nothing | |
| safetensors_path = os.path.join(target_caption_folder, "model.safetensors") | |
| detector_model_path = os.path.join(weights_dir, "icon_detect", "model.pt") | |
| if os.path.exists(safetensors_path) and os.path.exists(detector_model_path): | |
| print(f"✅ Models found in {weights_dir}. Skipping download.") |
Description
This PR keeps only the latest 3 commits for this branch and focuses on:
app.state) for OmniParser, RAG KB, and Heuristic Engine.Included Commits (this PR only)
8dcbb6d-refactor(singleton): wire OmniParser, RAG KB, and engine via app.state5227b47-feat(omniparser): add configurable Florence batching with adaptive OOM fallback1aa0033-chore(logging): label RAG and heuristic engine startup logs as singletonReview Focus (Suggested Order)
app/services/omniparser_client.py- batching + adaptive fallback logicapp/core/config.pyand.env.example- batch-size config contractmain.py,app/services/heuristic_engine.py, route files - singleton wiringKey File Changes
1. OmniParser batching + memory fallback (
app/services/omniparser_client.py)Commit:
5227b47Adds configurable Florence batching and safe fallback when memory is low.
This is the main logic change maintainers should review.
2. Batch size config from env (
app/core/config.py,.env.example)Commit:
5227b47Makes Florence batch size configurable and validated.
+ FLORENCE_BATCH_SIZE=33. Singleton service wiring in startup and routes (
main.py,app/services/heuristic_engine.py,app/api/routes/evaluation.py,app/api/routes/heuristic.py)Commit:
8dcbb6dEnsures startup creates shared services once, and routes reuse those instances.
4. Clear singleton startup logs (
main.py)Commit:
1aa0033Small log clarity update so startup clearly shows singleton usage.
Before / After Flow
Before
OmniParser,RAG KB, andHeuristic Enginecould be created in more than one place (startup + route-level paths), which could lead to inconsistent instance usage.After
request.app.state.*.FLORENCE_BATCH_SIZE.N -> N/2 -> ... -> 1) instead of failing the whole request.Proof of Work
Custom Florence-Batching (OmniParser)