Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

modify_parameters reads stale cached values after in-place edits, and Reform.modify_parameters deep-copies a tree the reform already owns

Aperta
  • Qualcosa non va?
#618 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 2 giorni

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
22/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
python
Ambito
backend

Direzione di ricerca

Start in TaxBenefitSystem.modify_parameters and Reform.modify_parameters, and in ParameterNode._at_instant_cache, clone and Parameter.update. Reproduce the stale read with the modify_social_security_taxation snippet in the body: read the baseline at 2017 first, then apply the reform. Done when that reform's thresholds show its own values and the test_modify_parameters and test_tracing_parameter_isolation.py tests pass. The body describes a change already under way in #616, so check that PR before starting.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

Summary

modify_parameters has two problems:

  • Stale values. A reform can read the values the system had cached instead of the changes its own modifier made.
  • A redundant copy. Reform.modify_parameters deep-copies the baseline's parameter tree, although Reform.__init__ has already given the reform its own copy. That deep copy also throws away any change apply() made before it.

Both come from the same deep copy, which predates the per-node parameter cache.

Two ways a reform runs

A Reform subclass's apply() runs in one of two ways, and self.modify_parameters means a different method in each:

How the reform runs modify_parameters used What the modifier gets
As an instance: MyReform(baseline). The YAML test runner and Reform.from_dict(...)(system) use this. Reform.modify_parameters A deep copy of baseline.parameters
In place: system.apply_reform_set(MyReform), which calls MyReform.apply(system). Simulation(reform=...) and policyengine-us's CountryTaxBenefitSystem(reform=...) use this. TaxBenefitSystem.modify_parameters The system's own tree

Problem 1: in-place edits can read stale values

History
  • 2015, OpenFisca (ab498658). modify_parameters started deep-copying the baseline's tree. Only the tax-benefit system cached parameters at an instant, and modify_parameters resets that cache.
  • January 2023 (63a5be7c, "Add performance improvements"). Every ParameterNode got its own cache of the node at each instant, _at_instant_cache.
    • Parameter.update clears that cache up the tree (clear_parent_cache), and ParameterNode.clone starts the clone with an empty cache.
    • copy.deepcopy copies the cache along with the tree.
    • Neither modify_parameters was changed.
When it happens

Both conditions must hold:

  1. The tree has already been read at an instant. On the instance path that means the baseline; on the in-place path, the system itself.
  2. The modifier changes the tree without Parameter.update, for example by editing values_list, a scale's brackets list or a node's children.

The country template's own example reform, modify_social_security_taxation, does the second. On master:

from policyengine_core.country_template import CountryTaxBenefitSystem
from policyengine_core.country_template.reforms.modify_social_security_taxation import (
    modify_social_security_taxation,
)

scale = lambda system: system.get_parameters_at_instant("2017-01-01").taxes.social_security_contribution

scale(modify_social_security_taxation(CountryTaxBenefitSystem())).thresholds
# [6000.0, 12400.0, 40000]  (the reform)

baseline = CountryTaxBenefitSystem()
scale(baseline)  # the baseline reads 2017
scale(modify_social_security_taxation(baseline)).thresholds
# [0.0, 6000.0, 12400.0]  (the baseline's values)

Reform.from_dict reforms are not affected: their modifier changes values with Parameter.update, which clears the cache. Core's own test_modify_parameters clears _at_instant_cache by hand after editing children.

This came up in #616. The YAML test runner there builds reforms on the reform-free system that earlier cases have read, and 3 of the country template's 39 YAML cases (its modify_social_security_taxation cases) then failed.

Problem 2: the deep copy is redundant and drops earlier changes

Until #459 (merged 2026-04-17), Reform.__init__ used the baseline's own tree (self.parameters = baseline.parameters). The deep copy was the only thing that kept a modifier's edits out of the baseline. #459 made Reform.__init__ clone the tree, so modify_parameters now makes a second full copy.

Measured on policyengine-us's tree (136,361 nodes), on a loaded macOS host:

Operation Time
copy.deepcopy of the tree as loaded 5.7 s
copy.deepcopy after reading it at 32 instants 6.9 s
clone(), which Reform.__init__ already does 3.9 s
A walk clearing every node's _at_instant_cache 0.03 s

Because each call starts again from baseline.parameters, a reform instance also loses any change apply() made earlier. That includes an update or set_parameter, and an earlier modify_parameters call. The same reform applied with apply_reform_set keeps them:

class update_then_two_modifiers(Reform):
    def apply(self):
        self.parameters.taxes.income_tax_rate.update(period="year:2017:1", value=0.2)
        self.modify_parameters(double_basic_income)
        self.modify_parameters(double_basic_income)

# Reform instance, master:            income_tax_rate 0.15, basic_income 1200
# apply_reform_set on master:         income_tax_rate 0.2,  basic_income 2400
# Either way, with #616:             income_tax_rate 0.2,  basic_income 2400

Change (in #616)

  • Reform no longer overrides modify_parameters. It inherits TaxBenefitSystem.modify_parameters, which gives the modifier the reform's own tree, so both ways of running a reform behave the same. The baseline is still left unchanged, because the reform's tree is the copy Reform.__init__ made.
  • TaxBenefitSystem.modify_parameters now clears caches. After the modifier runs, it clears every node's _at_instant_cache, plus the system's cache, in the tree it handed over and in the tree the modifier returned.

These are behaviour changes for reforms built as instances:

  • Changes made earlier in apply() are kept, and successive modify_parameters calls build on each other. This is what apply_reform_set already does.
  • A modifier that edits the tree in place but returns something other than a ParameterNode now has its edits kept, again as with apply_reform_set. Before, they were lost with the copy. modify_parameters still returns a ValueError in this case instead of raising it; that is unchanged.
  • A reform instance can now pass the dict form to modify_parameters, which TaxBenefitSystem already accepted.
Downstream usage

This comes from searching the current checkouts:

  • policyengine-us applies reforms with apply_reform_set, so its reforms already behave this way. Its modify_parameters overrides (CountryTaxBenefitSystem and the shared-parameter policy in spm.py) are on the system-level method.
  • The Public API builds one modifier that changes values with update, and calls it once.
  • policyengine-uk builds reform instances only in tests, each with one Reform.from_dict or one modifier.
  • policyengine.py has no modify_parameters calls.

Two of core's tests described the deep copy: #584's test_reform_that_modifies_parameters_keeps_the_cached_vectorial_node and a docstring in test_tracing_parameter_isolation.py. #616 updates both. The deep-copy behaviour they guarded is still covered by direct copy.deepcopy tests.

Lingua principale
Python
Stelle
22
Fork
30
Merge medio
2g 17h
PR unite (30g)
26

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di PolicyEngine/policyengine-core

Tutte le issue di PolicyEngine/policyengine-core

Issue simili

Altre issue su Python

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.