modify_parameters reads stale cached values after in-place edits, and Reform.modify_parameters deep-copies a tree the reform already owns
Maintainer thường phản hồi trong vòng 2 ngày
Chưa có ai nhận issue này.
Đánh giá
- Độ khó
- 4/5
- Thời gian dự kiến
- 3-5 ngày
- Mức phù hợp với người mới
- 22/100
Hướng nghiên cứu
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.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Mô tả
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_parametersdeep-copies the baseline's parameter tree, althoughReform.__init__has already given the reform its own copy. That deep copy also throws away any changeapply()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_parametersstarted deep-copying the baseline's tree. Only the tax-benefit system cached parameters at an instant, andmodify_parametersresets that cache. - January 2023 (
63a5be7c, "Add performance improvements"). EveryParameterNodegot its own cache of the node at each instant,_at_instant_cache.Parameter.updateclears that cache up the tree (clear_parent_cache), andParameterNode.clonestarts the clone with an empty cache.copy.deepcopycopies the cache along with the tree.- Neither
modify_parameterswas changed.
When it happens
Both conditions must hold:
- 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.
- The modifier changes the tree without
Parameter.update, for example by editingvalues_list, a scale'sbracketslist or a node'schildren.
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)
Reformno longer overridesmodify_parameters. It inheritsTaxBenefitSystem.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 copyReform.__init__made.TaxBenefitSystem.modify_parametersnow 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 successivemodify_parameterscalls build on each other. This is whatapply_reform_setalready does. - A modifier that edits the tree in place but returns something other than a
ParameterNodenow has its edits kept, again as withapply_reform_set. Before, they were lost with the copy.modify_parametersstill returns aValueErrorin this case instead of raising it; that is unchanged. - A reform instance can now pass the dict form to
modify_parameters, whichTaxBenefitSystemalready 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. Itsmodify_parametersoverrides (CountryTaxBenefitSystemand the shared-parameter policy inspm.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_dictor one modifier. - policyengine.py has no
modify_parameterscalls.
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.
- Ngôn ngữ chính
- Python
- Star
- 22
- Fork
- 30
- Merge trung bình
- 2 ngày 17 giờ
- Pull request đã merge (30 ngày)
- 26
Chuẩn bị môi trường
- Không có Dockerfile hay tệp Docker Compose
- Có mẫu pull request
- Đọc hướng dẫn đóng góp
Bắt đầu từ đâu
- Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
- Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
- Fork repository và làm thay đổi trên một nhánh.
- Mở pull request có tham chiếu số hiệu của issue.
Issue khác của PolicyEngine/policyengine-core
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 76/100
PolicyEngine/policyengine-core#605 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 2 ngày
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
PolicyEngine/policyengine-core#549 ·
Maintainer thường phản hồi trong vòng 2 ngày
-
`restore_simulation` silently drops memberless group entitiesCó thể đã có người làm @Adelagric đã nhận 37 ngày trước. Đang mở
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
PolicyEngine/policyengine-core#547 ·
Maintainer thường phản hồi trong vòng 2 ngày
-
Độ khó 5/5 Hơn một tuần Mức phù hợp với người mới 8/100
PolicyEngine/policyengine-core#621 ·
Maintainer thường phản hồi trong vòng 2 ngày
-
Độ khó 4/5 3-5 ngày Mức phù hợp với người mới 35/100
PolicyEngine/policyengine-core#617 ·
Maintainer thường phản hồi trong vòng 2 ngày
Tất cả issue của PolicyEngine/policyengine-core
Issue tương tự
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
-
[Bug] to_ical lexicographical/default sorting moves URL property between two X-WR- propertiesĐang mở
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 68/100
collective/icalendar#1968 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Deferred from #1869: Complete the companion pytest guardrail update noted in the PR description...Đang mở
Độ khó 1/5 Dưới một giờ Mức phù hợp với người mới 95/100
Maintainer thường phản hồi trong vòng 1 ngày
-
Custom provider API keys collide on one host: env slot keyed on host:port only, not URL pathĐang mở
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 88/100
NousResearch/hermes-agent#140958 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Bug triage/confirmed
Độ khó 2/5 Nửa ngày Mức phù hợp với người mới 85/100
agentscope-ai/agentscope#3355 ·
Maintainer thường phản hồi trong vòng 2 ngày