Skip to content

Fix cache preservation and uprating order#491

Merged
MaxGhenis merged 1 commit into
masterfrom
fix/cache-uprating-tdd
May 14, 2026
Merged

Fix cache preservation and uprating order#491
MaxGhenis merged 1 commit into
masterfrom
fix/cache-uprating-tdd

Conversation

@MaxGhenis
Copy link
Copy Markdown
Contributor

@MaxGhenis MaxGhenis commented May 14, 2026

Summary

  • preserve actual set_input storage writes, including dispatched subperiod writes, branch-specific writes, and on-disk inputs, across apply_reform cache invalidation
  • make holder branch fallback and dispatched input conflict checks branch-aware across both memory and disk storage
  • topologically sort chained parameter uprating dependencies, reject cycles, and make parameter child traversal deterministic
  • add regression tests and Towncrier fragments for both fixes

Fixes #484.
Fixes #490.

Testing

  • uvx ruff format --check policyengine_core/holders/holder.py policyengine_core/holders/helpers.py policyengine_core/simulations/simulation.py policyengine_core/parameters/parameter_node.py policyengine_core/parameters/parameter_scale_bracket.py policyengine_core/parameters/operations/uprate_parameters.py tests/core/test_apply_reform_preserves_user_inputs.py tests/core/test_holder_branch_fallback.py tests/core/parameters/operations/test_uprating.py
  • uvx ruff check policyengine_core/holders/holder.py policyengine_core/holders/helpers.py policyengine_core/simulations/simulation.py policyengine_core/parameters/parameter_node.py policyengine_core/parameters/parameter_scale_bracket.py policyengine_core/parameters/operations/uprate_parameters.py tests/core/test_apply_reform_preserves_user_inputs.py tests/core/test_holder_branch_fallback.py tests/core/parameters/operations/test_uprating.py
  • uv run --no-sync pytest tests/core/test_apply_reform_preserves_user_inputs.py tests/core/test_apply_reform_invalidates_cache.py tests/core/test_holders.py tests/core/test_holder_branch_fallback.py tests/core/parameters/operations/test_uprating.py -q
  • uv run --no-sync pytest -q (536 passed, 5 skipped, 1 xfailed)

@MaxGhenis MaxGhenis force-pushed the fix/cache-uprating-tdd branch 2 times, most recently from f9f04af to d188cd0 Compare May 14, 2026 22:13
@anth-volk
Copy link
Copy Markdown
Collaborator

I validated the updated PR against the specific filesystem-order bug we saw in the household API.

What looks good:

  • The focused uprating test file passes locally for me: 19 passed.
  • The minimal chained dependency repro now works: target -> middle -> base correctly produces middle("2026-01-01") == 110 and target("2026-01-01") == 110.
  • The real policyengine-us CA repro works when forcing the Modal-bad CA directory order (tax before cpi.yaml):
    • ca_cpi_2026 = 369.3222178951283
    • exemption_amount_2026 = 156.46742564975085
    • dependent_amount_2026 = 485.7648835531481

So for the specific Modal/GCP CA uprating incident, this appears to accomplish what we need.

One remaining core correctness concern: the dependency sorter keys by parameter.name for parameter_by_name, visited, and visiting. That can skip uprating cloned subtrees whose child parameters preserve the same .name.

I reproduced this shape locally:

from policyengine_core.parameters import ParameterNode, uprate_parameters

root = ParameterNode(data={
    "target": {"values": {"2025-01-01": 100}, "metadata": {"uprating": "middle"}},
    "middle": {"values": {"2025-01-01": 100}, "metadata": {"uprating": "base"}},
    "base": {"values": {"2025-01-01": 100, "2026-01-01": 110}},
})
root.add_child("baseline", root.clone())
uprate_parameters(root)

print(root.middle("2026-01-01"))          # 110
print(root.target("2026-01-01"))          # 110
print(root.baseline.middle("2026-01-01")) # 100
print(root.baseline.target("2026-01-01")) # 100

If cloned/baseline duplicate-name subtrees are expected to be uprated, I think the graph bookkeeping should key by object identity rather than parameter.name. If those subtrees are intentionally out of scope, it would be useful to add a regression test or comment making that explicit.

@MaxGhenis MaxGhenis force-pushed the fix/cache-uprating-tdd branch from d188cd0 to d7c8255 Compare May 14, 2026 22:18
@MaxGhenis MaxGhenis merged commit 811d78e into master May 14, 2026
45 of 46 checks passed
@MaxGhenis MaxGhenis deleted the fix/cache-uprating-tdd branch May 14, 2026 22:32
@MaxGhenis
Copy link
Copy Markdown
Contributor Author

Thanks, Anthony - I reproduced this locally and opened follow-up #492 to fix cloned subtree uprating by tracking dependency-sort traversal by object identity. Added the cloned-subtree regression you sketched there as well.

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.

Make parameter uprating independent of traversal order Dispatched set_input values are dropped after cache invalidation

2 participants