From 4a32d712bafc6b1350f2e4205b4eff8780375810 Mon Sep 17 00:00:00 2001 From: Bruno Domingues Date: Mon, 20 Oct 2025 17:53:49 -0300 Subject: [PATCH] SIENTIAPDE-1255: Implement object destructors for Activities and ExperimentTracking to prevent AttributeError during garbage collection, and mock workflow logger in tests to avoid RuntimeWarning. --- model_manager/activities/activities.py | 22 +++++++++++++++++++ .../activities/experiment_tracking.py | 17 ++++++++++++++ tests/workflows/test_train_model.py | 18 +++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/model_manager/activities/activities.py b/model_manager/activities/activities.py index b0aa0ff..b810512 100644 --- a/model_manager/activities/activities.py +++ b/model_manager/activities/activities.py @@ -102,6 +102,26 @@ class Activities(ExperimentTracking, MLFlow, MinIO, Training): Training.__init__(self, logger=logger, notification_handler=notification_handler) + def __del__(self): + """ + Destructor to safely handle cleanup during garbage collection. + + This prevents AttributeError when the parent Postgres.__del__ tries to access + self.engine in objects with multiple inheritance. Only attempts cleanup if + the engine attribute exists. + """ + # Only call parent __del__ if engine attribute exists + # This prevents AttributeError in multiple inheritance scenarios + if hasattr(self, 'engine'): + try: + # Call parent class __del__ if it exists + if hasattr(super(), '__del__'): + super().__del__() + except Exception: # noqa: S110, BLE001 + # Silently ignore errors during garbage collection + # Logging here could cause issues if logger is already destroyed + pass + async def shutdown(self): """ Gracefully shutdown all activities and clean up resources. @@ -112,5 +132,7 @@ class Activities(ExperimentTracking, MLFlow, MinIO, Training): The method should be called before the application terminates to ensure proper resource cleanup and prevent resource leaks. + + Prefer calling this method explicitly rather than relying on __del__. """ ExperimentTracking.close(self) diff --git a/model_manager/activities/experiment_tracking.py b/model_manager/activities/experiment_tracking.py index 56ebff3..a5c354a 100644 --- a/model_manager/activities/experiment_tracking.py +++ b/model_manager/activities/experiment_tracking.py @@ -88,6 +88,23 @@ class ExperimentTracking(Postgres): self.logger = logger self.notification_handler = notification_handler + def __del__(self): + """ + Destructor to safely handle cleanup during garbage collection. + + This prevents AttributeError when used in multiple inheritance scenarios + where the parent Postgres.__del__ might be called on objects without + the engine attribute. + """ + # Only call parent __del__ if engine attribute exists + if hasattr(self, 'engine'): + try: + if hasattr(super(), '__del__'): + super().__del__() + except Exception: # noqa: S110, BLE001 + # Silently ignore errors during garbage collection + pass + @activity.defn(name='update_experiment_run') async def update_experiment_run(self, input_data: dict[str, Any]) -> None: """ diff --git a/tests/workflows/test_train_model.py b/tests/workflows/test_train_model.py index 561e863..5feee99 100644 --- a/tests/workflows/test_train_model.py +++ b/tests/workflows/test_train_model.py @@ -89,6 +89,9 @@ async def test_run_success_complete_flow( 'removed_intervals': [], } + # Mock workflow.logger to avoid RuntimeWarning about unawaited coroutines + workflow_mock.logger = MagicMock() + # Mock activity responses workflow_mock.execute_activity_method = AsyncMock( side_effect=[ @@ -264,6 +267,9 @@ async def test_validate_training_parameters_validation_error( input_data = {'experiment_run_id': 123} metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + # Mock workflow.logger to avoid RuntimeWarning about unawaited coroutines + workflow_mock.logger = MagicMock() + workflow_mock.execute_activity_method = AsyncMock( side_effect=[ ValueError('Missing required field'), # validate_train_params fails @@ -318,6 +324,9 @@ async def test_download_and_train_model_download_error( """Test download error is handled correctly.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + # Mock workflow.logger to avoid RuntimeWarning about unawaited coroutines + workflow_mock.logger = MagicMock() + workflow_mock.execute_activity_method = AsyncMock( side_effect=[ Exception('MinIO connection failed'), # fetch_file_from_minio fails @@ -342,6 +351,9 @@ async def test_download_and_train_model_training_error( """Test training error is handled correctly.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + # Mock workflow.logger to avoid RuntimeWarning about unawaited coroutines + workflow_mock.logger = MagicMock() + workflow_mock.execute_activity_method = AsyncMock( side_effect=[ b'file_content', # fetch_file_from_minio succeeds @@ -396,6 +408,7 @@ async def test_download_and_train_model_closes_bytesio_on_error( """Test that BytesIO is closed in finally block even on error.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + workflow_mock.logger = MagicMock() # Create a mock BytesIO with close method mock_file = MagicMock() mock_file.close = MagicMock() @@ -425,6 +438,7 @@ async def test_download_and_train_model_handles_file_without_close( """Test that workflow handles file objects without close method gracefully.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + workflow_mock.logger = MagicMock() # Create a mock file without close method mock_file = MagicMock(spec=[]) @@ -457,6 +471,7 @@ async def test_save_model_to_mlflow_success( """Test successful model saving to MLFlow.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + workflow_mock.logger = MagicMock() workflow_mock.execute_activity_method = AsyncMock( side_effect=[ mock_train_result, # save_model @@ -480,6 +495,7 @@ async def test_save_model_to_mlflow_error( """Test MLFlow save error is handled correctly.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + workflow_mock.logger = MagicMock() workflow_mock.execute_activity_method = AsyncMock( side_effect=[ Exception('MLFlow connection failed'), # save_model fails @@ -511,6 +527,7 @@ async def test_cleanup_resources_success( """Test successful resource cleanup.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + workflow_mock.logger = MagicMock() workflow_mock.execute_activity_method = AsyncMock( side_effect=[ None, # cleanup_run_directory @@ -537,6 +554,7 @@ async def test_cleanup_resources_delete_error( """Test cleanup handles delete errors correctly.""" metadata = {'metadata': {'experiment_run_id': 123, 'workflow_name': 'train_model'}} + workflow_mock.logger = MagicMock() workflow_mock.execute_activity_method = AsyncMock( side_effect=[ None, # cleanup_run_directory succeeds