Skip to content

OnDiskStorage.delete(period) deletes only that exact period, not the periods within it #564

Description

@MaxGhenis

Holder.delete_arrays(period) is documented to remove "all values for any period included in period (e.g. if period is "2017", values for "2017-01", "2017-07", etc. would be removed)". InMemoryStorage.delete does that (period.contains(...)). OnDiskStorage.delete instead removes only the file whose key is exactly f"{branch_name}_{period}" (policyengine_core/data_storage/on_disk_storage.py, delete). So the same deletion removes monthly values from memory but leaves them on disk. A disk-backed simulation then keeps reading values the caller deleted.

Reproduction on 3.32.12 (master b78b0ba), country template only:

"""OnDiskStorage.delete(period) deletes only that exact period (policyengine-core 3.32.12)."""
import warnings
warnings.simplefilter("ignore")
from policyengine_core.country_template import CountryTaxBenefitSystem
from policyengine_core.experimental import MemoryConfig
from policyengine_core.simulations import SimulationBuilder


def simulation(on_disk):
    sim = SimulationBuilder().build_from_entities(
        CountryTaxBenefitSystem(),
        {"persons": {"a": {}}, "households": {"h": {"parents": ["a"]}}},
    )
    if on_disk:
        sim.memory_config = MemoryConfig(max_memory_occupation=0)
        holder = sim.get_holder("rent")
        holder._disk_storage = holder.create_disk_storage()
        holder._on_disk_storable = True
    sim.set_input("rent", "2025-01", [500.0])
    sim.set_input("rent", "2025-02", [600.0])
    sim.delete_arrays("rent", "2025")
    return [str(p) for p in sim.get_holder("rent").get_known_periods()]


print("in memory, after delete_arrays('rent', '2025'):", simulation(False))
print("on disk,   after delete_arrays('rent', '2025'):", simulation(True))

Output:

in memory, after delete_arrays('rent', '2025'): []
on disk,   after delete_arrays('rent', '2025'): ['2025-01', '2025-02']

Suggested fix: in OnDiskStorage.delete, when period is given, parse each key with the same split #552 introduces (key.rsplit("_", 1), since a period's string form never contains _), and delete the files whose branch is branch_name and whose period period.contains, as InMemoryStorage.delete does. Eternal storage keeps deleting its single ETERNITY key. Best done after #552 merges, since #552 adds that key parser and fixes the period-None branch's prefix match. Test exact, containing, excluding and eternal periods on both backends.

Found while fixing #559 (PR #561). #561 keeps _user_input_keys in step with whatever disk storage actually keeps, so it does not depend on this fix. The PolicyEngine-UK #2025 round-3 review (finding 5) hit the same behaviour.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions