Conversation
- Updated the model checker to verify dependencies for the IQA-based best-shot pipeline. - Removed legacy model paths and introduced a new dependency catalog. - Simplified the check for required modules using importlib. - Enhanced error messages for missing dependencies. - Added compatibility helpers for NumPy < 2.0 to prevent crashes in third-party packages. - Introduced runtime path helpers for PyInstaller to manage resource locations. - Improved application cache clearing logic in the AppController. - Added tests for best-shot model checking and quality rating computations. - Enhanced the AI rating worker to emit status messages during processing. - Updated tests to reflect changes in model checking and dependency management.
…g in BestPhotoSelector
…stimation in BestPhotoSelector and BestShotPipeline
…llel ranking in LocalBestShotStrategy
…LocalBestShotStrategy
There was a problem hiding this comment.
Pull Request Overview
This pull request modernizes the AI best-shot ranking pipeline by transitioning from a multi-model system requiring manual model downloads to an automated, no-reference IQA approach using MUSIQ, MANIQA, and LIQE via the pyiqa package. The changes improve compatibility with NumPy 2.0, enhance runtime resource handling for frozen builds, and add ETA displays to progress indicators.
Key Changes:
- Replaced manual face detector/eye classifier/aesthetic predictor models with automated IQA metrics (MUSIQ/MANIQA/LIQE)
- Added NumPy 2.0 compatibility layer by reintroducing
np.sctypes - Introduced runtime path helpers for PyInstaller/frozen builds
- Enhanced progress reporting with ETA calculations in worker threads
Reviewed Changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/ai/best_photo_selector.py |
Complete rewrite replacing multi-model pipeline with IQA-based ranking using pyiqa |
src/core/ai/best_shot_pipeline.py |
Added heuristic prefiltering, quality rating computation, and parallel IQA scoring |
src/core/ai/model_checker.py |
Simplified dependency checking to verify torch and pyiqa presence |
src/core/numpy_compat.py |
New compatibility module restoring np.sctypes for NumPy 2.0+ |
src/core/runtime_paths.py |
New helper module for PyInstaller resource resolution |
src/core/app_settings.py |
Added get_preferred_torch_device() for automatic device selection (cuda/mps/cpu) |
src/core/similarity_utils.py |
Extracted embedding normalization and adaptive DBSCAN logic |
src/core/similarity_engine.py |
Integrated normalization helpers and adaptive clustering |
workers/best_shot_worker.py |
Added ETA calculation and enhanced progress messages |
src/workers/ai_rating_worker.py |
Added ETA formatting to rating progress |
src/ui/app_controller.py |
Added logic to skip already-rated images and clear analysis cache |
src/ui/ui_components.py |
Updated CUDA detection to report device name instead of boolean |
src/ui/dialog_manager.py |
Enhanced device label to show cuda/mps/cpu with friendly names |
src/main.py |
Integrated runtime path helpers and local model cache setup |
requirements.txt, requirements-cuda.txt |
Added pyiqa and mediapipe dependencies |
.github/workflows/release-build.yml |
Updated build config to bundle pyiqa and mediapipe data |
README.md |
Updated documentation to reflect new IQA pipeline |
| Test files | New/updated tests for IQA pipeline, similarity helpers, cache clearing, and dependency checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/core/ai/best_photo_selector.py
Outdated
| from PIL import Image, ImageOps | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| from src.core.numpy_compat import ensure_numpy_sctypes |
There was a problem hiding this comment.
Incorrect import path: This should be from core.numpy_compat import ensure_numpy_sctypes rather than from src.core.numpy_compat import ensure_numpy_sctypes. The src prefix is inconsistent with other imports in the file (e.g., line 22 imports from core.app_settings without the src prefix).
| from src.core.numpy_compat import ensure_numpy_sctypes | |
| from core.numpy_compat import ensure_numpy_sctypes |
src/core/ai/best_photo_selector.py
Outdated
| from core.app_settings import get_local_best_shot_constants | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
Debug logging left enabled in production: logger.setLevel(logging.DEBUG) is set unconditionally. This will generate excessive logs in production environments. Either remove this line to respect the configured logging level, or wrap it in a conditional check.
| logger.setLevel(logging.DEBUG) |
src/core/ai/best_shot_pipeline.py
Outdated
| thresholds = [0.22, 0.42, 0.62, 0.8] | ||
| for idx, threshold in enumerate(thresholds, start=1): | ||
| if normalized_score < threshold: | ||
| return idx | ||
| return 5 | ||
|
|
||
|
|
||
| def _compute_quality_rating(result) -> Tuple[int, float]: | ||
| def _is_number(value: object) -> bool: | ||
| return isinstance(value, (int, float)) and not isinstance(value, bool) | ||
|
|
||
| quality_score: Optional[float] = None | ||
|
|
||
| composite = getattr(result, "composite_score", None) | ||
| if _is_number(composite): | ||
| composite_value = float(composite) | ||
| if math.isfinite(composite_value): | ||
| quality_score = composite_value | ||
|
|
||
| if quality_score is None: | ||
| metrics = getattr(result, "metrics", {}) or {} | ||
| metric_values = [ | ||
| float(value) for value in metrics.values() if _is_number(value) | ||
| ] | ||
| if metric_values: | ||
| quality_score = sum(metric_values) / len(metric_values) | ||
|
|
||
| if quality_score is None: | ||
| samples: List[float] = [] | ||
| raw = getattr(result, "raw_metrics", {}) or {} | ||
| musiq_raw = raw.get("musiq_raw") | ||
| if _is_number(musiq_raw): | ||
| samples.append(_normalize_for_rating(musiq_raw, lower=25.0, upper=85.0)) | ||
| liqe_raw = raw.get("liqe_raw") | ||
| if _is_number(liqe_raw): | ||
| samples.append(_normalize_for_rating(liqe_raw, lower=30.0, upper=90.0)) | ||
| maniqa_raw = raw.get("maniqa_raw") | ||
| if _is_number(maniqa_raw): | ||
| samples.append(_normalize_for_rating(maniqa_raw, lower=0.25, upper=0.85)) | ||
| if samples: |
There was a problem hiding this comment.
Magic numbers should be documented or extracted as constants: The thresholds [0.22, 0.42, 0.62, 0.8] and the normalization bounds in lines 477, 479, and 483 are undocumented magic numbers. Consider extracting them as named constants (e.g., RATING_THRESHOLDS, MUSIQ_EXPECTED_RANGE, etc.) with explanatory comments to improve maintainability.
| try: | ||
| prepared = _downscale_image(preview, self._preview_max_edge) | ||
| if prepared is preview: | ||
| # Ensure caller gets a live image even if no resize was needed. | ||
| prepared = prepared.copy() | ||
| return prepared | ||
| finally: | ||
| try: | ||
| preview.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Potential resource leak: The preview image is closed in the finally block, but if _downscale_image creates a copy and the original is still preview, the copy (prepared) may not be closed when an exception occurs after downscaling but before the function returns. Consider ensuring all allocated images are properly tracked and closed.
src/core/ai/best_photo_selector.py
Outdated
| tensor = _pil_to_tensor(image).to(device) | ||
| with metric_lock: | ||
| try: | ||
| value = _run_metric(tensor) | ||
| except Exception as exc: | ||
| if ( | ||
| self.spec.name != "maniqa" | ||
| or "list index out of range" not in str(exc).lower() | ||
| ): | ||
| raise | ||
| logger.debug( | ||
| "MANIQA failed on %s; retrying with %dx%d center crop", | ||
| image.info.get("source_path", "<unknown>"), | ||
| MANIQA_SAFE_INPUT, | ||
| MANIQA_SAFE_INPUT, | ||
| ) | ||
| safe_image = ImageOps.fit( | ||
| image, | ||
| (MANIQA_SAFE_INPUT, MANIQA_SAFE_INPUT), | ||
| method=_RESAMPLE_LANCZOS, | ||
| centering=(0.5, 0.5), | ||
| ) | ||
| tensor = _pil_to_tensor(safe_image).to(device) | ||
| value = _run_metric(tensor) |
There was a problem hiding this comment.
[nitpick] Inconsistent handling of tensor device placement: The code creates a tensor and moves it to device on line 274, but after the MANIQA fallback on line 296, the new tensor is also moved to the same device. While this is correct, it would be clearer to extract the device placement logic to avoid duplication and ensure consistency.
| with _METRIC_CACHE_LOCK: | ||
| cached = _METRIC_CACHE.get(cache_key) | ||
| if cached is None: | ||
|
|
||
| @staticmethod | ||
| def _iou(box: np.ndarray, others: np.ndarray) -> np.ndarray: | ||
| ymin = np.maximum(box[0], others[:, 0]) | ||
| xmin = np.maximum(box[1], others[:, 1]) | ||
| ymax = np.minimum(box[2], others[:, 2]) | ||
| xmax = np.minimum(box[3], others[:, 3]) | ||
| def _factory(): | ||
| return pyiqa.create_metric( | ||
| self.spec.name, | ||
| device=device, | ||
| as_loss=False, | ||
| ) | ||
|
|
||
| inter = np.maximum(0.0, ymax - ymin) * np.maximum(0.0, xmax - xmin) | ||
| box_area = (box[2] - box[0]) * (box[3] - box[1]) | ||
| other_area = (others[:, 2] - others[:, 0]) * (others[:, 3] - others[:, 1]) | ||
| union = box_area + other_area - inter + 1e-6 | ||
| return inter / union | ||
| if self.status_callback is None: | ||
| metric = _factory() | ||
| else: | ||
| with _PYIQA_DOWNLOAD_LOCK: | ||
| metric = self._with_download_notifications( | ||
| download_util, _factory | ||
| ) | ||
| metric.eval() | ||
| cached = (metric, threading.Lock()) | ||
| _METRIC_CACHE[cache_key] = cached |
There was a problem hiding this comment.
Potential race condition with module-level cache: _METRIC_CACHE is accessed with _METRIC_CACHE_LOCK on line 243, but the nested metric_lock from the cached tuple is used later on line 275. If multiple threads are creating metrics for the same key concurrently, the factory might be called multiple times before the cache is populated, leading to redundant metric initialization. Consider ensuring the factory is only called once per cache key.
src/core/ai/best_photo_selector.py
Outdated
| def _with_download_notifications(self, download_util, factory): | ||
| original_loader = download_util.load_file_from_url | ||
|
|
||
| def wrapped_loader(url, model_dir=None, progress=True, file_name=None): | ||
| target_dir = model_dir or download_util.DEFAULT_CACHE_DIR | ||
| filename = file_name or os.path.basename(urlparse(url).path) | ||
| destination = os.path.abspath(os.path.join(target_dir, filename)) | ||
| should_notify = not os.path.exists(destination) | ||
| if should_notify: | ||
| self._report_download_status("start", destination) | ||
| try: | ||
| return original_loader( | ||
| url, | ||
| model_dir=model_dir, | ||
| progress=progress, | ||
| file_name=file_name, | ||
| ) | ||
| finally: | ||
| if should_notify: | ||
| self._report_download_status("done", destination) | ||
|
|
||
| def _ensure_ready(self): | ||
| if self._model is not None: | ||
| return | ||
| download_util.load_file_from_url = wrapped_loader | ||
| try: | ||
| import torch # type: ignore | ||
| from transformers import ( # type: ignore | ||
| AutoImageProcessor, | ||
| MobileNetV2ForImageClassification, | ||
| ) | ||
| except ImportError as exc: # pragma: no cover | ||
| raise RuntimeError( | ||
| "transformers and torch are required for the eye-state classifier" | ||
| ) from exc | ||
|
|
||
| self._device = torch.device("cuda" if torch.cuda.is_available() else "cpu") | ||
| self._processor = AutoImageProcessor.from_pretrained( | ||
| self.model_dir, local_files_only=True | ||
| ) | ||
| self._model = MobileNetV2ForImageClassification.from_pretrained( | ||
| self.model_dir, local_files_only=True | ||
| ) | ||
| self._model.to(self._device) | ||
| self._model.eval() | ||
|
|
||
| def predict_open_probability( | ||
| self, eye_image: Image.Image, image_path: Optional[str] = None | ||
| ) -> float: | ||
| import torch # type: ignore | ||
|
|
||
| assert ( | ||
| self._processor is not None | ||
| and self._model is not None | ||
| and self._device is not None | ||
| ) | ||
| inputs = self._processor(images=eye_image, return_tensors="pt") | ||
| inputs = {k: v.to(self._device) for k, v in inputs.items()} | ||
| with torch.no_grad(): | ||
| logits = self._model(**inputs).logits | ||
| probs = torch.softmax(logits, dim=-1) | ||
| # Class index 1 == eyes open | ||
| return float(probs[0, 1].item()) | ||
|
|
||
| return factory() | ||
| finally: | ||
| download_util.load_file_from_url = original_loader |
There was a problem hiding this comment.
Monkeypatching global state without cleanup: The function modifies download_util.load_file_from_url on line 323 and only restores it in the finally block. If an exception occurs during the factory call (line 325) but the module is used elsewhere concurrently, other threads may see the patched version unexpectedly. Consider using a context manager or ensuring thread-safety if concurrent metric creation is possible.
| def _compute_ratio( | ||
| landmarks, width: int, height: int, indices: Dict[str, int] | ||
| ) -> Optional[float]: | ||
| try: | ||
| upper = landmarks[indices["upper"]] | ||
| lower = landmarks[indices["lower"]] | ||
| outer = landmarks[indices["outer"]] | ||
| inner = landmarks[indices["inner"]] | ||
| except (IndexError, KeyError): # pragma: no cover - defensive guard | ||
| return None | ||
| vertical = abs(upper.y - lower.y) | ||
| horizontal = abs(outer.x - inner.x) | ||
| if horizontal <= 0: | ||
| return None | ||
| return vertical / horizontal |
There was a problem hiding this comment.
[nitpick] Missing input validation for eye landmarks: The _compute_ratio method accesses landmark indices without validating the landmarks object has sufficient elements. While there's a try-except on line 110, accessing an invalid index directly could raise an IndexError before the defensive guard. Consider checking len(landmarks) or the presence of required indices before access.
| try: | ||
| if mps_backend.is_available(): # type: ignore[attr-defined] | ||
| return "mps" | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # If an error occurs while checking MPS availability, fall back to CPU. |
src/core/ai/best_photo_selector.py
Outdated
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| logger.debug( | |
| "Failed to close disposable image resource: %s", exc, exc_info=True | |
| ) |
… in best shot pipeline
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workers/best_shot_worker.py
Outdated
| def _format_duration(seconds: float) -> str: | ||
| """Return a compact human readable duration.""" | ||
| if not math.isfinite(seconds): | ||
| return "" | ||
| seconds = max(0, int(round(seconds))) | ||
| hours, remainder = divmod(seconds, 3600) | ||
| minutes, secs = divmod(remainder, 60) | ||
| parts: List[str] = [] | ||
| if hours: | ||
| parts.append(f"{hours}h") | ||
| if minutes or hours: | ||
| parts.append(f"{minutes}m") | ||
| if secs or not parts: | ||
| parts.append(f"{secs}s") | ||
| return " ".join(parts) | ||
|
|
||
|
|
There was a problem hiding this comment.
The _format_duration function is duplicated in both workers/best_shot_worker.py and src/workers/ai_rating_worker.py with identical implementations. Consider extracting this to a shared utility module to avoid code duplication and ensure consistency.
| def _format_duration(seconds: float) -> str: | |
| """Return a compact human readable duration.""" | |
| if not math.isfinite(seconds): | |
| return "" | |
| seconds = max(0, int(round(seconds))) | |
| hours, remainder = divmod(seconds, 3600) | |
| minutes, secs = divmod(remainder, 60) | |
| parts: List[str] = [] | |
| if hours: | |
| parts.append(f"{hours}h") | |
| if minutes or hours: | |
| parts.append(f"{minutes}m") | |
| if secs or not parts: | |
| parts.append(f"{secs}s") | |
| return " ".join(parts) | |
| from core.utils.time_utils import format_duration |
src/workers/ai_rating_worker.py
Outdated
| def _format_duration(seconds: float) -> str: | ||
| if not math.isfinite(seconds): | ||
| return "" | ||
| seconds = max(0, int(round(seconds))) | ||
| hours, remainder = divmod(seconds, 3600) | ||
| minutes, secs = divmod(remainder, 60) | ||
| parts: list[str] = [] | ||
| if hours: | ||
| parts.append(f"{hours}h") | ||
| if minutes or hours: | ||
| parts.append(f"{minutes}m") | ||
| if secs or not parts: | ||
| parts.append(f"{secs}s") | ||
| return " ".join(parts) |
There was a problem hiding this comment.
The _format_duration function is duplicated from workers/best_shot_worker.py. Extract this common utility to a shared module to maintain DRY principles.
workers/best_shot_worker.py
Outdated
| seconds = max(0, int(round(seconds))) | ||
| hours, remainder = divmod(seconds, 3600) | ||
| minutes, secs = divmod(remainder, 60) | ||
| parts: List[str] = [] |
There was a problem hiding this comment.
Inconsistent type hint style: workers/best_shot_worker.py uses List[str] (capital L), while src/workers/ai_rating_worker.py uses list[str] (lowercase). The lowercase syntax is the modern Python 3.9+ style. Consider using lowercase list[str] consistently throughout the codebase.
| parts: List[str] = [] | |
| parts: list[str] = [] |
src/ui/app_controller.py
Outdated
| def _get_existing_rating_for_path(self, image_path: str) -> Optional[int]: | ||
| normalized_path = os.path.normpath(image_path) | ||
| cached_rating = self.app_state.rating_cache.get(normalized_path) | ||
| if cached_rating is not None: | ||
| return int(cached_rating) | ||
|
|
||
| disk_cache = getattr(self.app_state, "rating_disk_cache", None) | ||
| if disk_cache: | ||
| disk_rating = disk_cache.get(normalized_path) | ||
| if disk_rating is not None: | ||
| rating_int = int(disk_rating) | ||
| self.app_state.rating_cache[normalized_path] = rating_int | ||
| return rating_int | ||
|
|
||
| try: | ||
| metadata_rating = PyExiv2Operations.get_rating(normalized_path) | ||
| except Exception: | ||
| logger.debug( | ||
| "Failed to read rating metadata for %s", | ||
| normalized_path, | ||
| exc_info=True, | ||
| ) | ||
| return None | ||
|
|
||
| if metadata_rating is None: | ||
| self.app_state.rating_cache.setdefault(normalized_path, 0) | ||
| if disk_cache: | ||
| disk_cache.set(normalized_path, 0) | ||
| return None | ||
|
|
||
| try: | ||
| rating_int = int(round(float(metadata_rating))) | ||
| except (TypeError, ValueError): | ||
| logger.debug( | ||
| "Unexpected rating value for %s: %s", | ||
| normalized_path, | ||
| metadata_rating, | ||
| ) | ||
| return None | ||
|
|
||
| rating_int = max(0, min(5, rating_int)) | ||
| self.app_state.rating_cache[normalized_path] = rating_int | ||
| if disk_cache: | ||
| disk_cache.set(normalized_path, rating_int) | ||
| return rating_int |
There was a problem hiding this comment.
[nitpick] The method _get_existing_rating_for_path is quite long (45 lines) and handles multiple responsibilities: checking multiple cache layers, reading from disk, validating data, and updating caches. Consider breaking this into smaller helper methods such as _check_memory_cache, _check_disk_cache, and _read_metadata_rating to improve readability and testability.
… path normalization and preview handling
This pull request introduces major updates to the AI best-shot ranking pipeline, transitioning from a multi-model system requiring manual downloads to a streamlined, no-reference IQA (Image Quality Assessment) approach using MUSIQ, MANIQA, and LIQE via the
pyiqapackage. It also improves compatibility with recent NumPy releases, refines dependency checking, and enhances runtime resource handling for bundled/frozen builds. Documentation and packaging are updated to reflect these changes.AI Best-Shot Ranking Pipeline Modernization
pyiqa, eliminating the need for manual model downloads. Updated documentation and code to reflect this new workflow. [1] [2] [3]Dependency Management and Packaging
pyiqaandmediapipeto both requirements and build scripts, ensuring these packages are included in frozen releases and available at runtime. [1] [2] [3] [4] [5]NumPy Compatibility Fixes
src/core/numpy_compat.pyto reintroduce thenp.sctypesattribute removed in NumPy 2.0, preventing crashes in third-party dependencies such asimgaug(used bypyiqa).Runtime Resource and Device Handling
src/core/runtime_paths.pywith helpers for frozen resource location, and refactored model path resolution to use these utilities for better PyInstaller support. [1] [2] [3]cuda,mps, orcpu) for inference, improving performance on supported hardware.Constants and Caching Improvements
These changes collectively modernize the best-shot ranking system, simplify setup for users, and improve robustness and compatibility across platforms.