Skip to content

Delete the periods within a deleted period from disk storage - #565

Draft
MaxGhenis wants to merge 3 commits into
masterfrom
fix-disk-delete-contained-periods
Draft

MaxGhenis wants to merge 3 commits into
masterfrom
fix-disk-delete-contained-periods

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #564.

Stacked on #552. This branch carries #552's two commits; review only the last commit, bfa78cb. I'll rebase onto master once #552 merges. It uses #552's _split_key (split on the last _) instead of a second key parser.

What was wrong

Holder.delete_arrays(period) documents that it removes "all values for any period included in period". InMemoryStorage.delete does that. OnDiskStorage.delete(period, branch_name) removed only the file keyed exactly f"{branch_name}_{period}". So a disk-backed simulation (one with a MemoryConfig) kept values the caller had deleted. Here is the issue's reproduction with delete_arrays("rent", "2025"):

in memory on disk
before (#552 head) [] ['2025-01', '2025-02']
this PR [] []

Change

OnDiskStorage.delete(period, branch_name) now deletes each file key whose parsed branch is branch_name and whose period period.contains(...). This is the same rule as InMemoryStorage.delete. Eternal storage still deletes the branch's single ETERNITY key, and delete(None, branch_name) keeps #552's whole-branch-name match. As before, delete only forgets the mapping and leaves the .npy file in place.

Invariants (tested)

  1. Memory and disk agree. For any sequence of put, delete(period, branch) and delete(None, branch), eternal or not, both storages hold the same (branch, period) keys with the same values. This is a differential test, checked after every step.
  2. Deletion follows the documented rule. After delete(p, b), branch b has no key whose period p contains, and every other key is unchanged. After delete(None, b), branch b has no keys. Both storages are checked against a reference model of this rule after every step.
  3. Branches are matched by their whole name. A branch name that contains _, or that starts with another branch's name plus _ (pre / pre_tcja / pre_tcja_ctc, y / y_2019, trailing_), never loses or keeps another branch's keys.

Tests

  • tests/core/test_disk_storage_delete_contained_periods.py runs without dev extras. It covers:

    Without the fix, 29 of the 59 fail: every disk case where a period contains another.

  • tests/core/test_disk_storage_delete_property.py holds the Hypothesis differential and model test for invariants 1–3. It starts with pytest.importorskip("hypothesis") because the smoke job installs no dev extras. It runs 200 examples in CI; I also ran 5,000 locally with up to 40 steps. Without the fix it shrinks to put(2023, "default") followed by delete(ETERNITY, "default"): disk keeps 2023.

  • On Windows, the cases whose period string contains : (multi-unit and rolling-year periods) are skipped. A Windows file name cannot hold that period's key, which predates this PR. Keep branch period reads, dumps, and disk deletion consistent #552's round-trip test now skips them for the same reason (5fe8b61; that was Keep branch period reads, dumps, and disk deletion consistent #552's Windows failure).

  • Full suite on master + Keep branch period reads, dumps, and disk deletion consistent #552 + this PR: 1,735 passed, 4 skipped, 1 xfailed. Country-template YAML: 39 passed. ruff format --check and ruff check are clean.

Open PRs that touch the same code

Each was merged with this branch on top of master and the full suite run:

PR merge full suite
#561 (579c375) clean 1,766 passed
#558 (a9a0b40) clean (on_disk_storage.py composes) 1,767 passed
#562 (912a4ac) simulation_dumper.py conflicts with #552, not this PR. Resolved as #562's body says (#552's branch passed to is_derived) 1,792 passed; its _derived.intersection_update sits after this PR's comprehension
#557 (97d4582) clean 1,979 passed

#561: its disk tests compare the record with what storage still holds, so they pass before and after this fix, and its Holder._forget_deleted_inputs drops an entry only when neither storage still holds the value. One sentence in that method's docstring goes stale once both are merged: "disk storage deletes only the period it is given, not the periods within it (policyengine-core#564), so a value deleted from memory can survive on disk, and its entry stays." Whichever of #561 and this PR merges second edits it.

Impact

Holders get disk storage only when simulation.memory_config is set (Holder.__init__). policyengine-us (4e4999a3), policyengine.py (6a9c878), policyengine-canada, -il and -ng never set it, and policyengine-uk (c7e826ea) sets it to None. So this changes no country-package or policyengine.py result.

Not run: make documentation, and Windows locally (CI covers Windows).

axiom: n/a: core storage fix, no policy encoded.

🤖 Generated with Claude Code

MaxGhenis and others added 3 commits September 27, 2026 15:50
…g over

Holder.get_known_periods() lists the periods of every stored key, with the
branch name stripped, but Holder.get_array() reads only the requested branch,
its parent_branch ancestors and "default". Simulation._calculate took the
latest known period from the unscoped list, so a period stored only under an
unrelated branch read back as None: uprating raised TypeError and
auto-carry-over cached NaN.

- Holder._readable_branch_names() is the one definition of what a branch can
  read; get_array() and the new get_known_periods(branch_name) both use it.
- _calculate uses get_known_periods(self.branch_name).
- OnDiskStorage splits "<branch>_<period>" keys on the last "_": branch names
  like "no_salt" raised ValueError, "y_2019" listed the wrong period, and
  delete(None, "pre_tcja") also wiped "pre_tcja_ctc".
- dump_simulation saves the values the dumped branch reads instead of reading
  every period under "default" and saving None.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The string form of a multi-unit or rolling-year period contains ':',
which a Windows file name cannot. On windows-latest, numpy.save raised
OSError for month:2025-01:3 and year:2024:2, and year:2025-03 read back
until restore() found no file for it. Disk storage has never kept these
periods on Windows; this PR does not change that, so the test skips them
there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
OnDiskStorage.delete(period, branch_name) removed only the file keyed
exactly f"{branch_name}_{period}". InMemoryStorage.delete and the
Holder.delete_arrays docstring remove every period the given period
contains, so a disk-backed holder kept values the caller had deleted
(deleting "2025" left "2025-01" and "2025-02"). Parse each key with
_split_key and delete those whose branch is branch_name and whose period
the deleted period contains. Eternal storage still deletes its one
ETERNITY key.

Fixes #564.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis added a commit that referenced this pull request Oct 7, 2026
Preserve input/derived provenance and #585 storage directory ownership. Resolve the dumper conflict using branch-scoped reads and the existing derived_periods.txt marks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis added a commit that referenced this pull request Oct 7, 2026
Fixes the two findings of #565's review (REQUEST_CHANGES at bfa78cb):

1. An input for a month or year anchored mid-month (month:2025-03-15)
   was accepted on disk under the calendar month's key, since its string
   form drops the day; deleting its own period, which does not contain
   that calendar month, then left it. In-memory storage already rejected
   it. Both storages now run one check, storage_keys.check_storable, so
   whether an input is accepted no longer depends on which storage
   Holder._set picks by memory use; disk storage rejects branch names
   containing ':' too.
2. restore() read back every .npy file. A name that is not a key put
   writes (stray.npy, default_garbage.npy, default_2025-3.npy) made
   delete and get_known_periods raise, so a valid deletion left its
   value. restore now leaves such files out, with a warning naming them.

Tests: examples on both backends and through Simulation for each finding;
the memory/disk Hypothesis differential now draws mid-month periods and a
':' branch name and checks both storages accept or reject each put alike;
a new Hypothesis property restores a directory of random keys and non-key
names and checks restore reads back exactly the keys, warns about the
rest, and then deletes like memory.

Folds #565's changelog fragment into this PR's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis added a commit that referenced this pull request Oct 7, 2026
Cover the remaining #565 review witness, periods.period(999), on both string-key storage backends. Preserve supported twelve-month aliases and existing derived values after rejected writes. Extend the differential property across the early-year boundary and twelve-unit periods.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant