diff --git a/apps/predbat/compare.py b/apps/predbat/compare.py index d9c242aa7..fa4291d30 100644 --- a/apps/predbat/compare.py +++ b/apps/predbat/compare.py @@ -86,6 +86,60 @@ def valid_compare_list(compare_list, log=None): return valid +# What Compare.run_all() saves before comparing tariffs and restores afterwards: every tariff fetches, plans and +# predicts on the live instance. The plan state is what calculate_plan() writes that outlives the run - leaving +# plan_preclip as the last tariff's plan once made the first re-plan after midnight score the wrong plan as the +# plan in force. Add any new plan state calculate_plan() keeps across runs here. +COMPARE_PLAN_STATE = ( + "charge_window_best", + "charge_limit_best", + "export_window_best", + "export_limits_best", + "plan_preclip", + "end_record", + "plan_valid", + "plan_last_updated", + "plan_last_updated_minutes", + "rate_best_cost_threshold_charge", + "rate_best_cost_threshold_export", +) +# Battery and inverter values a tariff can override, reset after each tariff as well as at the end +COMPARE_HARDWARE_STATE = ("soc_max", "battery_rate_max_charge", "battery_rate_max_charge_dc", "battery_rate_max_discharge", "battery_rate_max_export", "inverter_limit") +COMPARE_RESTORED_STATE = ( + COMPARE_PLAN_STATE + + COMPARE_HARDWARE_STATE + + ( + "forecast_plan_hours", + "forecast_minutes", + "forecast_days", + "manual_charge_times", + "manual_export_times", + "manual_freeze_charge_times", + "manual_freeze_export_times", + "manual_demand_times", + "manual_all_times", + "cost_today_sofar", + "carbon_today_sofar", + "iboost_today", + "iboost_plan", + "import_today_now", + "export_today_now", + "octopus_intelligent_charging", + "soc_kw", + ) +) +# Car state the comparison rebuilds per tariff, deep-copied as the lists are changed in place +COMPARE_RESTORED_COPIES = ( + "car_charging_plan_smart", + "car_charging_limit", + "car_charging_limit_model", + "car_charging_soc", + "car_charging_battery_size", + "car_charging_slots", + "car_charging_now_slots", +) + + class Compare: """Tariff comparison engine. @@ -667,40 +721,9 @@ def run_all(self, debug=False, fetch_sensor=True): my_predbat = self.pb - save_forecast_plan_hours = my_predbat.forecast_plan_hours - save_forecast_minutes = my_predbat.forecast_minutes - save_forecast_days = my_predbat.forecast_days - save_manual_charge_times = my_predbat.manual_charge_times - save_manual_export_times = my_predbat.manual_export_times - save_manual_freeze_charge_times = my_predbat.manual_freeze_charge_times - save_manual_freeze_export_times = my_predbat.manual_freeze_export_times - save_manual_demand_times = my_predbat.manual_demand_times - save_manual_all_times = my_predbat.manual_all_times - save_charge_window_best = my_predbat.charge_window_best - save_export_window_best = my_predbat.export_window_best - save_export_limits_best = my_predbat.export_limits_best - save_charge_limit_best = my_predbat.charge_limit_best - save_cost_today_sofar = my_predbat.cost_today_sofar - save_carbon_today_sofar = my_predbat.carbon_today_sofar - save_iboost_today = my_predbat.iboost_today - save_iboost_plan = my_predbat.iboost_plan - save_import_today_now = my_predbat.import_today_now - save_export_today_now = my_predbat.export_today_now - save_octopus_intelligent_charging = my_predbat.octopus_intelligent_charging - save_car_charging_plan_smart = copy.deepcopy(my_predbat.car_charging_plan_smart) - save_car_charging_limit = copy.deepcopy(my_predbat.car_charging_limit) - save_car_charging_limit_model = copy.deepcopy(my_predbat.car_charging_limit_model) - save_car_charging_soc = copy.deepcopy(my_predbat.car_charging_soc) - save_car_charging_battery_size = copy.deepcopy(my_predbat.car_charging_battery_size) - save_car_charging_slots = copy.deepcopy(my_predbat.car_charging_slots) - save_car_charging_now_slots = copy.deepcopy(my_predbat.car_charging_now_slots) - save_soc_kw = my_predbat.soc_kw - save_soc_max = my_predbat.soc_max - save_battery_rate_max_charge = my_predbat.battery_rate_max_charge - save_battery_rate_max_charge_dc = my_predbat.battery_rate_max_charge_dc - save_battery_rate_max_discharge = my_predbat.battery_rate_max_discharge - save_battery_rate_max_export = my_predbat.battery_rate_max_export - save_inverter_limit = my_predbat.inverter_limit + # Everything the comparison changes on the live instance, saved here and put back in the finally below + saved = {name: getattr(my_predbat, name) for name in COMPARE_RESTORED_STATE} + saved.update({name: copy.deepcopy(getattr(my_predbat, name)) for name in COMPARE_RESTORED_COPIES}) # Final reports, cut end_record back to 24 hours to ignore the dump at end of day end_record = int((my_predbat.minutes_now + 24 * 60 + 29) / 30) * 30 - my_predbat.minutes_now @@ -748,19 +771,18 @@ def run_all(self, debug=False, fetch_sensor=True): # First ever run for this tariff: start from actual midnight SoC start_soc = soc_midnight_fallback self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff_id, start_soc)) + # Each tariff plans from scratch: scored against the previous tariff's plan, the keep-or-switch threshold + # in calculate_plan() could leave one tariff's result as another tariff's plan + my_predbat.plan_valid = False try: - result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=save_car_charging_slots, start_soc=start_soc, io_adjusted_base=io_adjusted_base) + result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=saved["car_charging_slots"], start_soc=start_soc, io_adjusted_base=io_adjusted_base) if result_data is not None: results[tariff_id] = result_data finally: # Restore hardware settings after each tariff so overrides don't bleed into the next tariff; # in a finally so a raised run_single() can't leave compare values applied to the live instance - my_predbat.soc_max = save_soc_max - my_predbat.battery_rate_max_charge = save_battery_rate_max_charge - my_predbat.battery_rate_max_charge_dc = save_battery_rate_max_charge_dc - my_predbat.battery_rate_max_discharge = save_battery_rate_max_discharge - my_predbat.battery_rate_max_export = save_battery_rate_max_export - my_predbat.inverter_limit = save_inverter_limit + for name in COMPARE_HARDWARE_STATE: + setattr(my_predbat, name, saved[name]) # Restore config: overrides after each tariff too, for the same reason - otherwise a # tariff's config override stays applied to config_index for every subsequent tariff, # even ones with no config block of their own (issue #4156) @@ -776,47 +798,18 @@ def run_all(self, debug=False, fetch_sensor=True): self.save_yaml() self.publish_data() finally: - # Restore config values overridden by any tariff's fetch_config() call - if config_snapshot: - for key, orig_value in config_snapshot.items(): - item = my_predbat.config_index.get(key) - if item is not None: - item["value"] = orig_value - my_predbat.fetch_config_options() - - # Restore original settings - my_predbat.forecast_plan_hours = save_forecast_plan_hours - my_predbat.forecast_minutes = save_forecast_minutes - my_predbat.forecast_days = save_forecast_days - my_predbat.manual_charge_times = save_manual_charge_times - my_predbat.manual_export_times = save_manual_export_times - my_predbat.manual_freeze_charge_times = save_manual_freeze_charge_times - my_predbat.manual_freeze_export_times = save_manual_freeze_export_times - my_predbat.manual_demand_times = save_manual_demand_times - my_predbat.manual_all_times = save_manual_all_times - my_predbat.charge_window_best = save_charge_window_best - my_predbat.export_window_best = save_export_window_best - my_predbat.export_limits_best = save_export_limits_best - my_predbat.charge_limit_best = save_charge_limit_best - my_predbat.cost_today_sofar = save_cost_today_sofar - my_predbat.carbon_today_sofar = save_carbon_today_sofar - my_predbat.iboost_today = save_iboost_today - my_predbat.iboost_plan = save_iboost_plan - my_predbat.import_today_now = save_import_today_now - my_predbat.export_today_now = save_export_today_now - my_predbat.octopus_intelligent_charging = save_octopus_intelligent_charging - my_predbat.car_charging_limit = save_car_charging_limit - my_predbat.car_charging_limit_model = save_car_charging_limit_model - my_predbat.car_charging_soc = save_car_charging_soc - my_predbat.car_charging_battery_size = save_car_charging_battery_size - my_predbat.car_charging_slots = save_car_charging_slots - my_predbat.car_charging_now_slots = save_car_charging_now_slots - my_predbat.car_charging_plan_smart = save_car_charging_plan_smart - my_predbat.soc_kw = save_soc_kw - my_predbat.soc_max = save_soc_max - my_predbat.battery_rate_max_charge = save_battery_rate_max_charge - my_predbat.battery_rate_max_charge_dc = save_battery_rate_max_charge_dc - my_predbat.battery_rate_max_discharge = save_battery_rate_max_discharge - my_predbat.battery_rate_max_export = save_battery_rate_max_export - my_predbat.inverter_limit = save_inverter_limit - my_predbat.io_adjusted = io_adjusted_base + try: + # Restore config values overridden by any tariff's fetch_config() call + if config_snapshot: + for key, orig_value in config_snapshot.items(): + item = my_predbat.config_index.get(key) + if item is not None: + item["value"] = orig_value + my_predbat.fetch_config_options() + finally: + # Restore original settings, last so they win over fetch_config_options(), and even if it raised + for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES: + setattr(my_predbat, name, saved[name]) + # A re-plan requested while the comparison ran (a setting or override changed) must still happen + my_predbat.plan_valid = saved["plan_valid"] and not my_predbat.update_pending + my_predbat.io_adjusted = io_adjusted_base diff --git a/apps/predbat/tests/test_compare.py b/apps/predbat/tests/test_compare.py index 99d8ca640..2e78b8360 100644 --- a/apps/predbat/tests/test_compare.py +++ b/apps/predbat/tests/test_compare.py @@ -16,6 +16,11 @@ - hardware isolation between tariffs: overrides applied for tariff N must not bleed into tariff N+1 in run_all() (regression for the bug where agile_fixed reported a final SOC larger than the normal battery size) + - run_all() restores every attribute it saves, the plan state calculate_plan() keeps across runs + included (regression for plan_preclip left as the last tariff's plan), and plans each tariff from scratch + - every attribute calculate_plan() and its optimise passes assign is either restored by run_all() or rebuilt + on every run + - a re-plan requested during the comparison survives it, and the state is restored even if the config restore raises """ import inspect @@ -23,7 +28,7 @@ import tempfile from unittest.mock import MagicMock, patch -from compare import Compare, check_compare_list, valid_compare_list +from compare import Compare, check_compare_list, valid_compare_list, COMPARE_HARDWARE_STATE, COMPARE_PLAN_STATE, COMPARE_RESTORED_STATE, COMPARE_RESTORED_COPIES from const import MINUTE_WATT @@ -71,10 +76,199 @@ def _make_compare(): return cmp, pb +# Assigned by calculate_plan() but rebuilt on every run before use, so the comparison need not restore them +CALCULATE_PLAN_REBUILT = ( + "load_minutes_step", + "load_minutes_step10", + "load_minutes_step90", + "pv_forecast_minute_step", + "pv_forecast_minute10_step", + "pv_forecast_minute90_step", + "prediction", + "metric_cloud_coverage", + "metric_load_divergence", + "plan_last_duration_seconds", +) + + +def test_run_all_restores_state(my_predbat): + """run_all() puts back everything it saves however a tariff's run changed it, resets the hardware values between + tariffs, and plans each tariff from scratch. + + Each tariff fetches, plans and predicts on the live instance. Leaving plan_preclip as the last tariff's 48-hour + plan made the first re-plan after the midnight comparison score that plan as the plan in force, so it always + switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026: 300.97p against 30.71p). + """ + names = COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES + saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending + # A valid plan with no re-plan pending, so plan_valid must come back True whatever earlier tests left behind + my_predbat.plan_valid, my_predbat.update_pending = True, False + original = {name: getattr(my_predbat, name) for name in names} + saved_compare_list = my_predbat.args.get("compare_list") + my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}, {"id": "tariff_b", "name": "Tariff B"}] + comparison = Compare.__new__(Compare) + comparison.pb = my_predbat + comparison.log = my_predbat.log + comparison.comparisons = {} + seen_at_start = [] + + def run_single(*args, **kwargs): + """Stand in for a tariff's run, changing every saved attribute as fetching and planning a tariff can.""" + seen_at_start.append((my_predbat.plan_valid, all(getattr(my_predbat, name) == original[name] for name in COMPARE_HARDWARE_STATE))) + for name in names: + setattr(my_predbat, name, ("changed by", name)) + my_predbat.plan_valid = True + return None + + comparison.run_single = run_single + comparison.select_best = lambda *args, **kwargs: None + comparison.save_yaml = lambda *args, **kwargs: None + comparison.publish_data = lambda *args, **kwargs: None + try: + comparison.run_all(fetch_sensor=False) + failed = 0 + not_restored = [name for name in names if getattr(my_predbat, name) != original[name]] + if not_restored: + print("ERROR: run_all() did not restore {}".format(not_restored)) + failed = 1 + if seen_at_start != [(False, True), (False, True)]: + print("ERROR: each tariff should start with plan_valid False and the real hardware values, got {}".format(seen_at_start)) + failed = 1 + if not failed: + print("PASS: run_all() restores all {} saved attributes, resets hardware between tariffs and plans each from scratch".format(len(names))) + return failed + finally: + for name, value in original.items(): + setattr(my_predbat, name, value) + my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending + if saved_compare_list is None: + my_predbat.args.pop("compare_list", None) + else: + my_predbat.args["compare_list"] = saved_compare_list + + +def test_plan_state_covered(my_predbat): + """Every attribute calculate_plan() and the optimise passes it runs assign is restored by the comparison or + rebuilt on every run. + + A new piece of plan state that outlives a run (as plan_preclip did) must be added to COMPARE_PLAN_STATE, or the + comparison will leave the last tariff's value behind. Read from the source with ast, so plain, augmented, + annotated and tuple assignments to self. are all found. + """ + import ast + import textwrap + + plan_class = type(my_predbat) + methods = ["calculate_plan"] + sorted(name for name in dir(plan_class) if name.startswith("optimise")) + assigned = set() + for name in methods: + tree = ast.parse(textwrap.dedent(inspect.getsource(getattr(plan_class, name)))) + for node in ast.walk(tree): + targets = node.targets if isinstance(node, ast.Assign) else [node.target] if isinstance(node, (ast.AugAssign, ast.AnnAssign)) else [] + for target in targets: + for part in ast.walk(target): + if isinstance(part, ast.Attribute) and isinstance(part.value, ast.Name) and part.value.id == "self": + assigned.add(part.attr) + uncovered = sorted(assigned - set(COMPARE_PLAN_STATE) - set(CALCULATE_PLAN_REBUILT)) + if uncovered: + print("ERROR: calculate_plan() or an optimise pass assigns {} - add each to COMPARE_PLAN_STATE (compare.py) if it outlives the run, or to CALCULATE_PLAN_REBUILT here if it is rebuilt before use".format(uncovered)) + return 1 + print("PASS: every attribute calculate_plan() and its {} optimise passes assign is restored or rebuilt".format(len(methods) - 1)) + return 0 + + +def test_run_all_keeps_pending_replan(my_predbat): + """A re-plan requested while the comparison runs (a setting or override changed) still invalidates the plan.""" + saved_compare_list = my_predbat.args.get("compare_list") + saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending + my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}] + my_predbat.plan_valid, my_predbat.update_pending = True, False + comparison = Compare.__new__(Compare) + comparison.pb = my_predbat + comparison.log = my_predbat.log + comparison.comparisons = {} + + def run_single(*args, **kwargs): + """Stand in for a tariff's run during which the user changes a setting.""" + my_predbat.update_pending = True + return None + + comparison.run_single = run_single + comparison.select_best = lambda *args, **kwargs: None + comparison.save_yaml = lambda *args, **kwargs: None + comparison.publish_data = lambda *args, **kwargs: None + try: + comparison.run_all(fetch_sensor=False) + if my_predbat.plan_valid: + print("ERROR: a re-plan requested during the comparison should leave the plan invalid") + return 1 + print("PASS: run_all() keeps a re-plan requested during the comparison") + return 0 + finally: + my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending + if saved_compare_list is None: + my_predbat.args.pop("compare_list", None) + else: + my_predbat.args["compare_list"] = saved_compare_list + + +def test_run_all_restores_when_config_restore_raises(my_predbat): + """The saved state is put back even when restoring a tariff's config raises.""" + item = my_predbat.config_index.get("metric_battery_cycle") + saved_compare_list = my_predbat.args.get("compare_list") + saved_preclip, saved_value = my_predbat.plan_preclip, item["value"] + saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending + plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], []) + my_predbat.plan_preclip = plan_in_force + my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A", "config": {"metric_battery_cycle": 9.0}}] + comparison = Compare.__new__(Compare) + comparison.pb = my_predbat + comparison.log = my_predbat.log + comparison.comparisons = {} + + def run_single(*args, **kwargs): + """Stand in for a tariff's run, which replaces the plan.""" + my_predbat.plan_preclip = ("tariff plan",) + return None + + def fetch_config_options(): + """Fail as a broken config refresh would.""" + raise ValueError("config refresh failed") + + comparison.run_single = run_single + comparison.select_best = lambda *args, **kwargs: None + comparison.save_yaml = lambda *args, **kwargs: None + comparison.publish_data = lambda *args, **kwargs: None + my_predbat.fetch_config_options = fetch_config_options + try: + try: + comparison.run_all(fetch_sensor=False) + except ValueError: + pass + if my_predbat.plan_preclip is not plan_in_force: + print("ERROR: run_all() should restore the plan even when the config restore raises, got {}".format(my_predbat.plan_preclip)) + return 1 + print("PASS: run_all() restores the saved state when the config restore raises") + return 0 + finally: + del my_predbat.fetch_config_options + my_predbat.plan_preclip = saved_preclip + my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending + item["value"] = saved_value + if saved_compare_list is None: + my_predbat.args.pop("compare_list", None) + else: + my_predbat.args["compare_list"] = saved_compare_list + + def test_compare(my_predbat): """Run all compare unit tests.""" failed = 0 print("**** Running compare tests ****\n") + failed += test_run_all_restores_state(my_predbat) + failed += test_plan_state_covered(my_predbat) + failed += test_run_all_keeps_pending_replan(my_predbat) + failed += test_run_all_restores_when_config_restore_raises(my_predbat) # ------------------------------------------------------------------ # T1: apply_hardware_overrides – soc_max override @@ -547,23 +741,13 @@ def _mock_plan_iboost_smart(): print("PASS T18: bad export override value falls back to the discharge override") # ------------------------------------------------------------------ - # T19: run_all() actually saves and restores battery_rate_max_export, - # both mid-loop and at the end, so an export override can't bleed - # into the next tariff (T7 only proves the pattern in isolation). - # Positional checks (T12-style) so the snapshot must sit before the - # tariff loop, not just anywhere in the source. + # T19: run_all() saves and restores battery_rate_max_export, after each + # tariff and at the end, so an export override can't bleed into the + # next tariff (T7 only proves the pattern in isolation; the restore + # itself is exercised by test_run_all_restores_state) # ------------------------------------------------------------------ - run_all_source = inspect.getsource(Compare.run_all) - loop_idx = run_all_source.find("for tariff in compare_list:") - snapshot_idx = run_all_source.find("save_battery_rate_max_export = my_predbat.battery_rate_max_export") - if loop_idx < 0 or snapshot_idx < 0: - print("ERROR T19: could not locate the tariff loop or the battery_rate_max_export snapshot in run_all() source - test may need updating") - failed += 1 - elif not (snapshot_idx < loop_idx): - print("ERROR T19: battery_rate_max_export snapshot must appear before the tariff loop in run_all()") - failed += 1 - elif run_all_source.count("my_predbat.battery_rate_max_export = save_battery_rate_max_export") < 2: - print("ERROR T19: run_all() must restore battery_rate_max_export both inside the tariff loop and at the end") + if "battery_rate_max_export" not in COMPARE_HARDWARE_STATE or "battery_rate_max_export" not in COMPARE_RESTORED_STATE: + print("ERROR T19: battery_rate_max_export must be reset after each tariff (COMPARE_HARDWARE_STATE) and at the end") failed += 1 else: print("PASS T19: run_all() snapshots and restores battery_rate_max_export") @@ -667,12 +851,8 @@ def _mock_plan_iboost_smart(): # restores it after it, as it does car_charging_slots, so the live plan # gets its modelled slots back after a comparison (#5245 review) # ------------------------------------------------------------------ - snapshot_idx = run_all_source.find("save_car_charging_now_slots = copy.deepcopy(my_predbat.car_charging_now_slots)") - if snapshot_idx < 0 or not (snapshot_idx < loop_idx): - print("ERROR T25: car_charging_now_slots must be snapshotted before the tariff loop in run_all()") - failed += 1 - elif run_all_source.find("my_predbat.car_charging_now_slots = save_car_charging_now_slots") < loop_idx: - print("ERROR T25: run_all() must restore car_charging_now_slots after the tariff loop") + if "car_charging_now_slots" not in COMPARE_RESTORED_COPIES: + print("ERROR T25: run_all() must snapshot (deep copy) and restore car_charging_now_slots") failed += 1 else: print("PASS T25: run_all() snapshots and restores car_charging_now_slots")