From c2a84d3ee8a0a6d083286a5b93960e498e1241cc Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 24 Jun 2026 11:07:34 +0000 Subject: [PATCH] =?UTF-8?q?per-property=20failure=20fails=20child=20SubTas?= =?UTF-8?q?k=20without=20raising=20from=20handler=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- applications/modelling_e2e/handler.py | 46 +------- .../modelling_e2e/test_handler.py | 109 +----------------- 2 files changed, 12 insertions(+), 143 deletions(-) diff --git a/applications/modelling_e2e/handler.py b/applications/modelling_e2e/handler.py index c06d4b4d..a715c7bf 100644 --- a/applications/modelling_e2e/handler.py +++ b/applications/modelling_e2e/handler.py @@ -294,8 +294,6 @@ def handler(body: dict[str, Any], context: Any, orchestrator: TaskOrchestrator, products = catalogue_with_off_catalogue_overrides(read_session) solar_reader = SolarPostgresRepository(read_session) - failures: list[dict[str, Any]] = [] - for property_id in property_ids: child = orchestrator.create_child_subtask( task_id, inputs={"property_id": property_id} @@ -446,20 +444,8 @@ def handler(body: dict[str, Any], context: Any, orchestrator: TaskOrchestrator, try: orchestrator.run_subtask(child.id, work=_work) - except Exception as error: # noqa: BLE001 - logger.error( - f"property={property_id} uprn={uprns.get(property_id)}: " - f"{type(error).__name__}: {error}", - exc_info=True, - ) - failures.append( - { - "property_id": property_id, - "uprn": uprns.get(property_id), - "error_type": type(error).__name__, - "error": str(error), - } - ) + except Exception: # noqa: BLE001 + pass # Cohort certs the mapper could not consume were skipped (not aborted on) # so prediction could proceed; surface them — with cert numbers — in the @@ -474,29 +460,9 @@ def handler(body: dict[str, Any], context: Any, orchestrator: TaskOrchestrator, f"{[s['certificate_number'] for s in skipped_certs]}" ) - # A property that errored AND a cohort cert the mapper could not consume - # are both surfaced as failures, so the subtask is marked failed and - # shows up for debugging. The whole batch has already run by this point — - # every property that could be modelled was written to DB above — so - # failing here flags the run without discarding the work done so far. - if failures or skipped_certs: - parts: list[str] = [] - if failures: - failed_ids = [f["property_id"] for f in failures] - # Persisted verbatim into the subtask's outputs.error (via - # SubTask.fail): include each property's error type + message, - # not just the IDs, so failed runs are diagnosable without - # cross-referencing CloudWatch. - parts.append( - f"failed property_ids: {failed_ids}; " - f"details: {json.dumps(failures)}" - ) - if skipped_certs: - parts.append( - f"skipped_unmappable_cohort_certs: {json.dumps(skipped_certs)}" - ) - raise RuntimeError("; ".join(parts)) - - return None + if skipped_certs: + raise RuntimeError( + f"skipped_unmappable_cohort_certs: {json.dumps(skipped_certs)}" + ) finally: read_session.close() diff --git a/tests/applications/modelling_e2e/test_handler.py b/tests/applications/modelling_e2e/test_handler.py index a07f7346..511fc3d6 100644 --- a/tests/applications/modelling_e2e/test_handler.py +++ b/tests/applications/modelling_e2e/test_handler.py @@ -513,9 +513,10 @@ def test_prediction_path_saves_predicted_epc_plan_and_baseline( # --------------------------------------------------------------------------- -def test_empty_cohort_gates_property_out_and_raises() -> None: - """When candidates_for returns an empty list the property cannot be - predicted; the handler records it as an error and raises RuntimeError.""" +def test_empty_cohort_gates_property_out_without_saving() -> None: + """When candidates_for returns an empty list the property cannot be predicted; + the failure is recorded in the child SubTask and the handler returns without + raising — no plan is saved.""" # Arrange mock_engine = _engine_mock([PROPERTY_ID], [UPRN], [POSTCODE]) @@ -585,9 +586,8 @@ def test_empty_cohort_gates_property_out_and_raises() -> None: patch("applications.modelling_e2e.handler.PostgresUnitOfWork") ) - # Act - with pytest.raises(RuntimeError, match=str(PROPERTY_ID)): - _call_handler(_BODY) + # Act — no raise; child SubTask is failed, handler continues + _call_handler(_BODY) # UoW never entered — the property errored before the write block MockUoW.return_value.__enter__.assert_not_called() @@ -717,103 +717,6 @@ def test_empty_own_postcode_broadens_to_nearby_and_predicts() -> None: mock_uow.commit.assert_called_once() -# --------------------------------------------------------------------------- -# Partial batch failure -# --------------------------------------------------------------------------- - - -def test_partial_batch_failure_raises_runtime_error_listing_failed_ids() -> None: - """Two properties: property 1 succeeds, property 2 raises during modelling. - Handler raises RuntimeError naming only the failed property; property 1's - UoW was committed.""" - # Arrange - pid1, pid2 = 111, 222 - mock_engine = _engine_mock([pid1, pid2], [1001, 1002], [POSTCODE, POSTCODE]) - mock_plan = _plan_mock() - mock_uow = MagicMock() - - def _run_modelling_side_effect(*args: Any, **kwargs: Any) -> Any: - # Fail on second call (pid2) - if not hasattr(_run_modelling_side_effect, "_calls"): - _run_modelling_side_effect._calls = 0 # type: ignore[attr-defined] - _run_modelling_side_effect._calls += 1 # type: ignore[attr-defined] - if _run_modelling_side_effect._calls == 2: # type: ignore[attr-defined] - raise ValueError("modelling exploded") - return mock_plan - - with ExitStack() as stack: - stack.enter_context( - patch("applications.modelling_e2e.handler.os.environ", _ENV) - ) - stack.enter_context( - patch( - "applications.modelling_e2e.handler._get_engine", - return_value=mock_engine, - ) - ) - stack.enter_context( - patch("applications.modelling_e2e.handler.EpcClientService") - ).return_value.get_by_uprn.return_value = MagicMock() # lodged EPC - stack.enter_context( - patch("applications.modelling_e2e.handler.GeospatialS3Repository") - ) - stack.enter_context( - patch("applications.modelling_e2e.handler.GoogleSolarApiClient") - ) - stack.enter_context( - patch( - "applications.modelling_e2e.handler._spatial_for", return_value=None - ) - ) - stack.enter_context( - patch( - "applications.modelling_e2e.handler._solar_insights_for", - return_value=None, - ) - ) - stack.enter_context( - patch( - "applications.modelling_e2e.handler.overlays_from", return_value=[] - ) - ) - stack.enter_context( - patch("applications.modelling_e2e.handler.PropertyOverridesPostgresReader") - ) - stack.enter_context( - patch("applications.modelling_e2e.handler.ScenarioPostgresRepository") - ).return_value.get_many.return_value = [MagicMock()] - stack.enter_context( - patch("applications.modelling_e2e.handler.catalogue_with_off_catalogue_overrides") - ) - stack.enter_context(patch("applications.modelling_e2e.handler.Session")) - stack.enter_context( - patch( - "applications.modelling_e2e.handler.run_modelling", - side_effect=_run_modelling_side_effect, - ) - ) - MockUoW = stack.enter_context( - patch("applications.modelling_e2e.handler.PostgresUnitOfWork") - ) - MockUoW.return_value.__enter__.return_value = mock_uow - MockUoW.return_value.__exit__.return_value = False - - # Act - with pytest.raises(RuntimeError, match=str(pid2)): - _call_handler( - { - "property_ids": [pid1, pid2], - "portfolio_id": PORTFOLIO_ID, - "scenario_id": SCENARIO_ID, - "no_solar": True, - "dry_run": False, - } - ) - - # Property 1 succeeded — its UoW was committed - mock_uow.commit.assert_called_once() - - # --------------------------------------------------------------------------- # Partial batch failure — per-property isolation # ---------------------------------------------------------------------------