feat: cross‑platform Florence/YOLO startup via flash_attn guard - #22
feat: cross‑platform Florence/YOLO startup via flash_attn guard#22Santosh69 wants to merge 9 commits into
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.
There was a problem hiding this comment.
Pull request overview
This pull request adds cross-platform development tooling and fixes Florence model initialization failures on non-CUDA systems by patching the flash_attn dependency check. The changes enable the project to run on CPU and MPS devices.
Changes:
- Added cross-platform development launcher (
start-dev.py) and OmniParser weight setup script (scripts/setup_omniparser.py) - Implemented transformers patch to skip flash_attn checks on non-CUDA systems
- Updated YOLO model path from
best.pttomodel.ptand enhanced Florence model initialization with device selection - Pinned critical dependencies and added new requirements for the patch functionality
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| start-dev.py | New cross-platform development launcher that verifies weights and starts backend/Firebase services |
| scripts/setup_omniparser.py | New script that downloads and organizes OmniParser model weights from HuggingFace |
| app/utils/transformers_patches.py | New module that patches transformers to prevent false-positive flash_attn errors on CPU/MPS |
| app/services/omniparser_client.py | Updated to apply transformers patch, change YOLO path, add device selection, and enhance logging |
| requirements.txt | Pinned torch/transformers versions and added einops, huggingface-hub, and accelerate dependencies |
| .gitignore | Removed markdown/rst exclusions, added firebase config and weights folder to ignore list |
| README.md | Restructured with new quick start instructions and detailed model weight setup options |
💡 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 processor is being loaded from the remote repository "microsoft/Florence-2-base-ft" while the model is loaded from local weights at "weights/icon_caption_florence". This creates an inconsistency where the processor and model might be from different versions. The processor should be loaded from the same local path as the model to ensure compatibility. Consider changing this to load from "weights/icon_caption_florence" to match the model loading on line 229.
| 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) |
|
|
||
| if setup_script.exists(): | ||
| print("Verifying OmniParser V2 weights...") | ||
| subprocess.run([sys.executable, str(setup_script)], check=True) |
There was a problem hiding this comment.
The script checks subprocess.run() with check=True but doesn't handle the CalledProcessError that could be raised if the setup script fails. This will result in an unhandled exception. Consider wrapping this in a try-except block to provide a better error message to users when setup fails.
| subprocess.run([sys.executable, str(setup_script)], check=True) | |
| try: | |
| subprocess.run([sys.executable, str(setup_script)], check=True) | |
| except subprocess.CalledProcessError as e: | |
| print( | |
| f"Error: scripts/setup_omniparser.py failed with exit code {e.returncode}.", | |
| file=sys.stderr, | |
| ) | |
| sys.exit(e.returncode or 1) |
| try: | ||
| snapshot_download( | ||
| repo_id="microsoft/OmniParser-v2.0", | ||
| revision=None, |
There was a problem hiding this comment.
Setting revision=None explicitly might cause confusion. The default behavior of snapshot_download is to use the main/master branch when revision is not specified. Consider either removing this parameter entirely (to use the default) or setting it to a specific commit hash or tag for reproducibility. Using None here is equivalent to omitting the parameter.
| revision=None, |
| sys.exit(0) | ||
|
|
||
| signal.signal(signal.SIGINT, shutdown_handler) | ||
| signal.signal(signal.SIGTERM, shutdown_handler) |
There was a problem hiding this comment.
On Windows, SIGTERM is not available. This will cause an AttributeError when the script tries to register the signal handler. Consider wrapping this in a try-except block or checking for the signal's availability with hasattr(signal, 'SIGTERM') before registering.
| signal.signal(signal.SIGTERM, shutdown_handler) | |
| if hasattr(signal, "SIGTERM"): | |
| signal.signal(signal.SIGTERM, shutdown_handler) |
| # 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 script only checks for the existence of "model.safetensors" to determine if models are present, but according to the README and the code, it should also verify that the YOLO model at "weights/icon_detect/model.pt" exists. Currently, if the Florence model exists but the YOLO model is missing, the script will skip the download entirely, leading to runtime errors later.
| # 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 detection models exist, do nothing | |
| safetensors_path = os.path.join(target_caption_folder, "model.safetensors") | |
| yolo_model_path = os.path.join(weights_dir, "icon_detect", "model.pt") | |
| if os.path.exists(safetensors_path) and os.path.exists(yolo_model_path): | |
| print(f"✅ Models found in {target_caption_folder} and icon_detect. Skipping download.") |
|
|
||
| try: | ||
| from huggingface_hub import snapshot_download | ||
| except Exception: |
There was a problem hiding this comment.
The exception handling uses a bare "except Exception:" which is too broad and might catch unexpected errors like KeyboardInterrupt (though Python 3 handles this differently). Consider catching more specific exceptions like ImportError, ModuleNotFoundError to provide more accurate error messages to users.
| except Exception: | |
| except (ImportError, ModuleNotFoundError): |
| cwd=firebase_dir, | ||
| shell=(os.name == "nt") |
There was a problem hiding this comment.
Setting shell=True only on Windows (os.name == "nt") may cause inconsistent behavior. The "npm run serve" command should work without shell=True on all platforms since "npm" is a regular executable. Consider removing the shell parameter entirely, or if it's needed on Windows for some reason, add a comment explaining why.
| cwd=firebase_dir, | |
| shell=(os.name == "nt") | |
| cwd=firebase_dir |
| ## Quick Start | ||
|
|
||
| ### Using the Development Script | ||
| ### Quick Start (Recommended) |
There was a problem hiding this comment.
The README title shows "Quick Start" twice - first as "## Quick Start" (line 65) and then immediately as "### Quick Start (Recommended)" (line 67). This creates redundancy and an inconsistent heading hierarchy. Consider removing the "## Quick Start" header and keeping only the "### Quick Start (Recommended)" subsection, or restructure to have "## Quick Start" with multiple subsections beneath it.
| ### Quick Start (Recommended) | |
| ### Recommended |
Summary
start-dev.pyand OmniParser setup scriptsetup_omniparser.py.Changes
start-dev.pyandscripts/setup_omniparser.py.app/utils/transformers_patches.pyand apply it inomniparser_client.py.requirements.txt(torch/transformers pins +einops,huggingface-hub,accelerate)..gitignoreto ignore local weights and Firebase config.Testing
Scripts Fix