diff --git a/scripts/guardrails-baseline.json b/scripts/guardrails-baseline.json index c0f07f5..eda301b 100644 --- a/scripts/guardrails-baseline.json +++ b/scripts/guardrails-baseline.json @@ -20,7 +20,7 @@ "long_functions": 2 }, "src/matteloop/jobs/models/session.py": { - "lines": 439, + "lines": 437, "long_functions": 1 }, "src/matteloop/jobs/render.py": { diff --git a/src/matteloop/jobs/models/session.py b/src/matteloop/jobs/models/session.py index 37d777b..865aacd 100644 --- a/src/matteloop/jobs/models/session.py +++ b/src/matteloop/jobs/models/session.py @@ -220,14 +220,12 @@ def fetch( def remove(self, model_id: str) -> bool: with self._lock: spec = self._catalog.get(model_id) - if (self._active_spec is not None and self._active_spec.id == spec.id) or ( - spec.id in self._cleanup_ids - ): + if spec.id in self._cleanup_ids: raise AppError( ErrorCode.MODEL_IN_USE, "model-cache", "error.model.in-use", - f"model {spec.id!r} owns the active segmentation session", + f"model {spec.id!r} is pending segmentation cleanup", "close-or-switch-model", ) self._require_open() diff --git a/src/matteloop/ui/model_manager/_controller.py b/src/matteloop/ui/model_manager/_controller.py index 15bd60b..cadf648 100644 --- a/src/matteloop/ui/model_manager/_controller.py +++ b/src/matteloop/ui/model_manager/_controller.py @@ -139,8 +139,6 @@ def _removal_block_reason(self, entry: ModelEntry) -> str | None: return "Cannot remove a model while a job is running." if self._manager is None: return "Model removal is unavailable in this runtime." - if self._manager.active_id == entry.model_id: - return "Cannot remove the model used by the active session." return None def _download_requested(self, value: object) -> None: @@ -196,7 +194,9 @@ def _delete_outdated_requested(self) -> None: QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, QMessageBox.StandardButton.No, ) - if answer is not QMessageBox.StandardButton.Yes: + # QMessageBox.question returns an int, so identity with StandardButton + # does not hold. + if answer != QMessageBox.StandardButton.Yes: return self._start_obsolete_removal(manager) @@ -213,10 +213,7 @@ def _redownload_outdated_requested(self) -> None: return if self._remove_thread is not None: return - if ( - _confirm_redownload(self.dialog, entries) - is not QMessageBox.StandardButton.Yes - ): + if _confirm_redownload(self.dialog, entries) != QMessageBox.StandardButton.Yes: return cancel = CancellationState() self._batch = {entry.model_id: entry for entry in entries} @@ -373,7 +370,9 @@ def _obsolete_removal_failed(self, error: object) -> None: def _remove_requested(self, value: object) -> None: if not isinstance(value, ModelEntry) or not value.cached: return - if self._removal_block_reason(value) is not None: + reason = self._removal_block_reason(value) + if reason is not None: + self.dialog.set_message(reason) return if self._remove_thread is not None: return @@ -385,7 +384,7 @@ def _remove_requested(self, value: object) -> None: QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, QMessageBox.StandardButton.No, ) - if answer is not QMessageBox.StandardButton.Yes: + if answer != QMessageBox.StandardButton.Yes: return manager = self._manager if manager is None: diff --git a/src/matteloop/ui/workspace_controller.py b/src/matteloop/ui/workspace_controller.py index c75ad14..5868cda 100644 --- a/src/matteloop/ui/workspace_controller.py +++ b/src/matteloop/ui/workspace_controller.py @@ -85,4 +85,4 @@ def _confirm_pinned_delete(self) -> bool: QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, QMessageBox.StandardButton.No, ) - return answer is QMessageBox.StandardButton.Yes + return answer == QMessageBox.StandardButton.Yes diff --git a/tests/jobs/models/test_session.py b/tests/jobs/models/test_session.py index 3ae7c66..8324a29 100644 --- a/tests/jobs/models/test_session.py +++ b/tests/jobs/models/test_session.py @@ -413,20 +413,17 @@ def test_task9_rejects_custom_options_and_unimplemented_prompts( assert exc.value.code is ErrorCode.MODEL_PREPARATION_INVALID -def test_remove_rejects_active_model_then_removes_exact_inactive_artifact( - tmp_path: Path, -) -> None: +def test_remove_active_and_inactive_model_weights(tmp_path: Path) -> None: manager, _downloader, _clients, _events = _manager(tmp_path) - result = manager.prepare("u2net", {}) - assert result.artifact_path is not None + inactive = manager.prepare("u2net", {}) + active = manager.prepare("u2netp", {}) + assert inactive.artifact_path is not None + assert active.artifact_path is not None - with pytest.raises(AppError) as exc: - manager.remove("u2net") - assert exc.value.code is ErrorCode.MODEL_IN_USE - - manager.prepare("u2netp", {}) assert manager.remove("u2net") is True - assert not result.artifact_path.exists() + assert not inactive.artifact_path.exists() + assert manager.remove("u2netp") is True + assert not active.artifact_path.exists() assert manager.remove("u2net") is False diff --git a/tests/ui/test_model_manager.py b/tests/ui/test_model_manager.py index 4863c81..ece7f63 100644 --- a/tests/ui/test_model_manager.py +++ b/tests/ui/test_model_manager.py @@ -5,9 +5,9 @@ from threading import Event, get_ident import pytest -from PySide6.QtCore import QUrl +from PySide6.QtCore import QTimer, QUrl from PySide6.QtGui import QDesktopServices -from PySide6.QtWidgets import QMessageBox +from PySide6.QtWidgets import QApplication, QMessageBox from matteloop.core.errors import AppError, ErrorCode from matteloop.core.parameters import ParameterState @@ -192,7 +192,7 @@ def test_model_manager_removes_only_confirmed_selected_weight_in_background( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) reported: list[str] = [] monkeypatch.setattr( @@ -224,7 +224,73 @@ def test_model_manager_removes_only_confirmed_selected_weight_in_background( controller.close() -def test_model_manager_blocks_active_and_running_job_removal( +def test_model_manager_removes_weight_after_real_confirmation( + tmp_path: Path, qtbot +) -> None: + target = _model_path(tmp_path, "u2netp") + target.parent.mkdir(parents=True) + target.write_bytes(b"cached-weight") + manager = FakeModelRemovalService(targets={"u2netp": target}) + controller = ModelManagerController( + ReducerStore(AppState(model_available=True)), + catalog=ModelCatalog.load_resource(), + cache_root=tmp_path, + manager=manager, + ) + qtbot.addWidget(controller.dialog) + controller.open() + controller.dialog.model_list.setCurrentRow( + next( + index + for index, entry in enumerate(controller.dialog.entries) + if entry.model_id == "u2netp" + ) + ) + + def click_active_modal_button() -> None: + app = QApplication.instance() + assert isinstance(app, QApplication) + modal = app.activeModalWidget() + if not isinstance(modal, QMessageBox): + return + yes_button = next( + ( + button + for button in modal.buttons() + if modal.buttonRole(button) == QMessageBox.ButtonRole.YesRole + ), + None, + ) + if yes_button is not None: + yes_button.click() + return + if manager.removed != ["u2netp"]: + return + ok_button = next( + ( + button + for button in modal.buttons() + if modal.buttonRole(button) == QMessageBox.ButtonRole.AcceptRole + ), + None, + ) + if ok_button is not None: + ok_button.click() + + modal_timer = QTimer(controller.dialog) + modal_timer.setInterval(10) + modal_timer.timeout.connect(click_active_modal_button) + modal_timer.start() + try: + controller.dialog.remove_button.click() + qtbot.waitUntil(lambda: manager.removed == ["u2netp"], timeout=5000) + qtbot.waitUntil(lambda: not target.exists(), timeout=5000) + finally: + modal_timer.stop() + controller.close() + + +def test_model_manager_allows_active_and_blocks_running_job_removal( tmp_path: Path, qtbot ) -> None: target = _model_path(tmp_path, "u2netp") @@ -247,8 +313,7 @@ def test_model_manager_blocks_active_and_running_job_removal( ) ) - assert controller.dialog.remove_button.isEnabled() is False - assert "active session" in controller.dialog.remove_button.toolTip() + assert controller.dialog.remove_button.isEnabled() is True running = ReducerStore( AppState( @@ -283,6 +348,12 @@ def test_model_manager_blocks_active_and_running_job_removal( assert running_controller.dialog.remove_button.isEnabled() is False assert "job is running" in running_controller.dialog.remove_button.toolTip() + selected = running_controller.dialog.selected_entry + assert selected is not None + running_controller._remove_requested(selected) + assert running_controller.dialog._message.text() == ( + "Cannot remove a model while a job is running." + ) controller.close() running_controller.close() @@ -529,7 +600,7 @@ def test_model_manager_reports_and_deletes_outdated_copy_alongside_current_weigh QMessageBox, "question", lambda _parent, title, message, *_args: ( - confirmation.append((title, message)) or QMessageBox.StandardButton.Yes + confirmation.append((title, message)) or int(QMessageBox.StandardButton.Yes) ), ) controller.dialog.delete_outdated_button.click() @@ -570,7 +641,7 @@ def test_model_manager_deletes_outdated_directory_without_touching_other_cache( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.delete_outdated_button.click() @@ -644,7 +715,7 @@ def test_model_manager_confirms_the_download_size_before_re_downloading_outdated QMessageBox, "question", lambda _parent, title, body, *_args: ( - confirmation.append((title, body)) or QMessageBox.StandardButton.No + confirmation.append((title, body)) or int(QMessageBox.StandardButton.No) ), ) @@ -691,7 +762,7 @@ def test_model_manager_re_downloads_each_outdated_weight_then_deletes_its_old_co monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -757,7 +828,7 @@ def record(message: str) -> None: monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -792,7 +863,7 @@ def test_model_manager_cancel_stops_the_batch_and_keeps_the_rest_outdated( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -840,7 +911,7 @@ def test_model_manager_escape_cancels_a_running_download_instead_of_hiding_the_d monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() qtbot.waitUntil(lambda: manager.in_flight, timeout=5000) @@ -877,7 +948,7 @@ def test_model_manager_ignores_dismissal_while_a_removal_runs( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.delete_outdated_button.click() qtbot.waitUntil(lambda: manager.obsolete_calls == 1, timeout=5000) @@ -923,7 +994,7 @@ def test_model_manager_stops_the_batch_at_the_first_failed_download( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -974,7 +1045,7 @@ def test_model_manager_close_cancels_the_running_download_before_waiting( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() qtbot.waitUntil(lambda: manager.in_flight, timeout=5000) @@ -1089,7 +1160,7 @@ def test_model_manager_refuses_re_download_while_a_job_runs( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -1128,7 +1199,7 @@ def fail_after_external_removal() -> int: monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.delete_outdated_button.click()