Trace parameter reads in yearly formulas and scale reads; add dependency-map - #543
Trace parameter reads in yearly formulas and scale reads; add dependency-map#543PavelMakarchuk wants to merge 6 commits into
Conversation
…ncy-map Two tracer gaps made FullTracer record almost no parameter reads for real models. The yearly ParameterNodeAtInstant was cached before the lazy tracing recast in _run_formula ran (defined_for and adds/subtracts evaluation build it first), so every parameters(period).gov.* read in a yearly formula went unrecorded while monthly formulas looked fine. Fixes #541. And TracingParameterNodeAtInstant only recorded scalar and array leaves, so scale and bracket reads (p.base.calc(age), p.max[children]) were invisible. Fixes #542. The trace setter now recasts the parameter tree (ParameterNode.set_tracing) and clears its at-instant caches whenever tracing switches on or off; _run_formula only follows the current branch. Scale children are recorded at their node with no scalar value. policyengine-core dependency-map -c <country package> builds on the fixed tracer: it traces the package's YAML tests (one per newly covered output variable per file) and optionally a microdata subsample, and writes readers (parameter path -> variables) and consumers (variable -> variables) with the package version, a fingerprint over its model surface, and the core version. Country packages need no wrapper. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n check reads them, not the formula Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The tool patched FullTracer.record_calculation_result globally, which leaked into any later trace in the same process (seven tracer tests failed when run after the tool's). It now uses a FullTracer subclass on its own simulations, and ParameterNode.set_tracing invalidates the at-instant caches when the tracer instance changes, not only when the flag does: cached tracing nodes capture the tracer they were built with, which branch simulations (which swap in the parent's tracer) rely on too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Clearing the at-instant caches whenever tracing switched or the tracer changed rebuilt the whole parameter tree each time, about two seconds on policyengine-us. Branch simulations swap tracers per formula, and the API traces some household simulations and not others on one shared system, so that cost landed on every consumer of tracing. The cache now holds plain nodes only, and _get_at_instant wraps one in TracingParameterNodeAtInstant on the way out when tracing is on. The wrapper reads the tracer and branch name from the parameter root at access time, so nothing is ever invalidated: set_tracing is three attribute writes, and branch labelling of parameter reads is now the current branch rather than whichever built the cache. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
anth-volk
left a comment
There was a problem hiding this comment.
I am requesting changes because the direct fixes for #541 and #542 work, but the new dependency map still omits real reform dependencies and silently skips a supported YAML output form. I also reproduced incorrect branch metadata in serialized parameter traces.
Findings
1. High: parameter-backed adds and subtracts are absent from traces
When a variable uses a parameter path for adds or subtracts, _run_formula resolves the Parameter directly and calls it with period.start (simulation.py lines 1052–1064). That call bypasses TracingParameterNodeAtInstant.
I reproduced this with an actual US reform:
- Reform: set
gov.irs.credits.refundableto an empty list for 2026. - Baseline
income_tax_refundable_credits:5065.6220703125. - Reformed value:
0.0. - Parameters recorded on the reformed
income_tax_refundable_creditstrace node: onlygov.abolitions.income_tax_refundable_credits;gov.irs.credits.refundableis absent.
The same direct-access problem applies to parameter-backed subtracts, and variable-level uprating also resolves parameters outside the tracing wrapper. These are material parameter dependencies: changing the list changes which variables contribute to the result. A map intended to classify reforms needs to record them explicitly in the active trace node or document and compensate for their omission.
2. High: entity-scoped YAML outputs are silently skipped
trace_yaml_tests loops over the top-level keys of test["output"] and passes each key to simulation.calculate (dependency_map.py lines 136–150). The standard YAML runner also supports entity-scoped output structures such as:
output:
people:
person1:
al_ui: 3850For these cases the new command attempts to calculate a variable named people, suppresses the exception, and never calculates the nested variables. The deduplication logic remembers only the top-level people key, so it also treats later cases in the same file as already covered.
In the current policyengine-us repository, 206 of the 5,405 cases selected by the default dependency-map logic have entity-scoped outputs. Running the command on policyengine_us/tests/policy/baseline/gov/states/al/dol/unemployment_insurance/al_ui.yaml reported:
tests: 1
failed: 0
parameter paths: 0
consumed variables: 0
The file contains valid nested person outputs. Output-variable extraction should follow the same entity-aware and period-aware rules as YamlItem.check_output. Calculation exceptions should also increment failure statistics or be surfaced, otherwise a materially incomplete map appears successful.
3. Medium: a nested branch can relabel later parent parameter reads
TracingParameterNodeAtInstant.tracer and .branch_name read mutable state from the shared parameter root at access time (tracing_parameter_node_at_instant.py lines 41–51). A nested branch calculation changes that shared state in _run_formula, but the parent state is not restored when the nested calculation returns.
I reproduced this through the existing ctc_limiting_tax_liability formula. Its trace node had branch default, while its later read of gov.irs.credits.non_refundable was labeled no_salt, the branch that had just completed. The dependency map currently ignores branch names, but the serialized tracer output is incorrect.
Now that _at_instant_cache contains only plain nodes, a wrapper returned for a particular formula can capture that formula's tracer and branch instead of consulting mutable root state. Alternatively, the active tracing state needs scoped restoration after nested calculations.
4. Low: the release note and source comment describe removed cache clearing
The final implementation deliberately keeps plain at-instant nodes cached, but the changelog fragment still says the caches are cleared (fragment). The comment in the Simulation.trace setter says “Recast (and clear)” as well. These should describe the final wrap-on-access design.
Successful verification
I tested PR head de2b4090533608c640900e31596199ac6de9198a against current policyengine-us 1.823.0 on Python 3.14.6.
- A reform setting
gov.irs.credits.ctc.amount.base[0].amountto$2,500produced[0, 2500]forctc_child_individual_maximum.get_serialized_flat_trace()recordedgov.irs.credits.ctc.amount.basewithnull, the intended representation for a scale. - A reform setting
gov.irs.credits.ctc.refundable.fully_refundabletotrueproduced a$2,500refundable CTC and recorded that yearly boolean parameter with valuetrue. - A vectorized federal-poverty-guideline calculation recorded
gov.hhs.fpg.first_personandgov.hhs.fpg.additional_personwith their selected array values. - A Social Security PIA calculation recorded the marginal-rate scale
gov.ssa.social_security.pia.formula_factorswithnull. - A CTC-only dependency-map run completed over 28 selected YAML cases, producing 319 parameter paths and 877 consumed variables. It included both
gov.irs.credits.ctc.amount.baseandgov.irs.credits.ctc.refundable.fully_refundable.
Commands run:
uv run --frozen pytest -q tests/core/test_parameters.py tests/core/parameters_fancy_indexing tests/core/test_reforms.py tests/core/test_reform_parameter_isolation.py tests/core/test_reform_period_keys.py tests/core/test_simulations.py tests/core/test_simulation_builder.py tests/core/test_tracers.py tests/core/tools/test_dependency_map.py
# 170 passed
uv run --frozen --extra dev ruff check <changed Python files and tests>
# passed
uv run --frozen --extra dev ruff format --check <changed Python files and tests>
# 7 files already formatted
policyengine-core dependency-map -c policyengine_us --tests-root policyengine_us/tests/policy/baseline/gov/irs/credits/ctc --output /tmp/ctc-dependency-map.json
# 28 tests, 0 reported failures, 319 parameter paths, 877 consumed variables
The GitHub checks are also currently successful across the reported Python and operating-system matrix. I did not run the full US microdata map because the deterministic omissions above already establish that its output can be incomplete.
Overall assessment and documentation review
Keeping plain at-instant nodes cached and wrapping them on access is a good direction: it fixes the yearly-read timing problem without the measured cache-rebuild cost. Recording tax-scale objects at their parameter path with a null value also matches the existing vectorized-access granularity.
The tracer changes are close, but the branch context needs to remain associated with the formula that obtained the wrapper. The dependency-map command should not be treated as complete until it handles core-managed parameter dependencies and the YAML runner's supported output structures. Splitting the tracer correction from the dependency-map command would also let the narrower #541/#542 work proceed independently.
Documentation observed: module documentation, command-line help, and two changelog fragments. Additional documentation is needed for the output schema and for how partial calculation failures affect completeness, and the stale cache-clearing statements need correction. Impact: high. Confidence: high, based on direct US reform and command execution. Known gap: I did not run the full microdata population after reproducing the deterministic correctness problems above.
…keep branch labels with their formula Review follow-up on #543. - Parameter-backed adds/subtracts lists and uprating factors are resolved on the parameter tree directly, bypassing the formula's tracing wrapper. Record them on the variable's trace node (Simulation._record_parameter_read). - TracingParameterNodeAtInstant captures its tracer and branch name when created instead of reading the tree's mutable state at access time; _run_formula restores the tree's tracing state after the formula, and get_branch hands the tree back to the caller after the branch's trace setter re-pointed it. A nested branch calculation no longer relabels the parameter reads its caller makes afterwards. - The dependency map reads test outputs the way the YAML runner does: entity singular and plural keys and period-keyed values, so entity-scoped cases are traced instead of silently skipped. Output calculations that raise are counted (outputErrors) and printed, and the module documents the output schema and how the counts bound completeness. - Changelog fragment and trace-setter comment describe the wrap-on-access design rather than cache clearing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the review. All four findings reproduced against the code and are addressed in b916dc5. 1. Parameter-backed 2. Entity-scoped YAML outputs (high). Confirmed. 3. Nested branch relabelling (medium). Confirmed via your 4. Stale cache-clearing wording (low). Changelog fragment and the Checks: full core suite 711 passed / 4 skipped / 1 xfailed; your focused set 175 passed; ruff check and format clean on the changed files; On splitting the tracer fix from the command: happy to do that if you'd prefer it for the #541/#542 release; leaving it as one PR for now so the command's tests keep exercising the tracer changes. |
anth-volk
left a comment
There was a problem hiding this comment.
I re-reviewed b916dc51 and confirmed that the four findings from my previous review are addressed. I also tested the proposed changes combined with the current master: 718 tests passed, with one skipped and one expected failure.
Medium: parameter-node structure reads emit invalid parameter paths and can bypass tracing
TracingParameterNodeAtInstant.get_traced_child treats every object that is neither a parameter node nor an allowed scalar, list, or NumPy array as though it were a tax schedule. It constructs a path from the accessed attribute and records None as its value.
PolicyEngine US has four formulas with five reads of the internal _children dictionary:
is_in_snap_abawd_waived_areaut_ccap_income_group(two reads)co_ccap_fpg_eligibleslcsp_rating_area_la_county
I reproduced the resulting dependency-map entry:
"gov.usda.snap.work_requirements.abawd.waived_counties._children": [
"is_in_snap_abawd_waived_area"
]_children is an implementation attribute, not a resolvable parameter path. Consequently, the map can expose a key that downstream code cannot resolve as a parameter. There is also a deeper loss in ut_ccap_income_group: p.income_limits._children.values() returns an unwrapped ParameterNodeAtInstant; the later size_node._children read therefore happens outside the tracing wrapper and is not recorded.
Please provide tracing-aware parameter-node iteration or mapping access, or otherwise special-case structural reads so that:
- the trace records a canonical parameter-node path rather than a path ending in
._children; - child names used by the formula are represented;
- parameter nodes obtained from the mapping remain wrapped; and
- the dependency map contains only documented, resolvable parameter paths.
A regression test should cover iteration over child names and retrieving a nested node through the child mapping.
Representation note: US tax schedules retain only their path
I also inventoried the current PolicyEngine US parameter tree. Scalar numbers, booleans, lists, and vectorized NumPy selections retain their values. The tree contains 651 SingleAmountTaxScale, 212 MarginalRateTaxScale, and 2 MarginalAmountTaxScale objects; all are recorded as None, so their concrete class, thresholds, rates, and amounts are absent from serialized traces.
That is sufficient for the dependency map because it consumes only parameter paths, but it is not a complete value representation for FullTracer. If complete serialized parameter traces are part of the intended contract, the general tracer should retain a JSON-safe schedule description while _QuietFullTracer can continue discarding values for dependency-map generation.
Review of #543, second round. Formulas read a node's structure in four places in policyengine-us: iterating p.node._children, taking its keys, taking its values and reading a nested node, and len(). The wrapper treated the _children dict like a scale, recording a path ending in ._children with no value, and handed out unwrapped nodes whose later reads went unrecorded. TracingChildren is a mapping view that records a read of the node's structure at the node's canonical path, with the child names as the value, and returns children through the parent's tracing so nested nodes stay wrapped. The wrapper also iterates and answers membership the same way. On the four formulas the map now records gov.usda.snap.work_requirements.abawd.waived_counties and its state children, gov.states.ut.dwf.ccap.copay.income_limits and the nested household-size node, gov.states.co.ccap.entry.fpg_rate, and gov.aca.la_county_rating_area, and nothing ending in ._children. Tax scales were recorded with a None value, which is enough for the dependency map but leaves serialized traces without the schedule. A scale read now records its class, thresholds, and rates or amounts as plain lists, so get_serialized_flat_trace carries the brackets. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the second pass. Both points are addressed in f140bc2, verified against policyengine-us 1.823.0. Structural reads (medium).
Regression tests cover iterating child names, retrieving a nested node through the mapping and reading under it, and iterating the node directly. On the four formulas you listed, the map now records Scale representation. Taken as part of the contract. A scale read now records Full suite: 684 passed, 4 skipped, 1 xfailed, ruff clean. |
|
@anth-volk re-requesting your review on f140bc2, which addresses the second round (structural One question that could shorten this: in your first review you suggested splitting the tracer corrections (#541, #542, plus the branch and structural-read fixes) from the |
|
Let's keep this as one PR. My earlier note about splitting it was only an optional suggestion, not a required change. I'll review the updated implementation together at f140bc2. |
Fixes #541. Fixes #542.
The two tracer gaps
With
simulation.trace = True,TraceNode.parameterswas almost always empty for real models:_run_formularecast the parameter tree for tracing lazily, but by thendefined_forandadds/subtractsevaluation had already built and cached the yearlyParameterNodeAtInstantas a plain node. Everyparameters(period).gov.*read for that instant bypassedTracingParameterNodeAtInstant. Monthly instants were created later and traced fine, which hid the bug.TracingParameterNodeAtInstant.get_traced_childonly recorded scalar and array leaves, so scale and bracket reads (p.base.calc(age),p.max[children], every bracket schedule) were never recorded.The fix
ParameterNode.set_tracing(tracer, branch_name)recasts the tree and clears every at-instant cache in the subtree whenever tracing switches on or off (clear_at_instant_caches).Simulation.tracesetter calls it (and is now the only placeself.traceris built; the duplicate assignment in__init__is gone)._run_formulaonly re-points the tree at the current branch, since branch simulations share the system. Systems with no parameter tree are skipped.value=None, matching how vectorial reads already record at the parent.Three regression tests in
test_tracers.pyfail on master and pass here: the yearly read after the node was cached untraced, the scale read, and trace off restoring plain nodes.policyengine-core dependency-mapBuilds on the fixed tracer.
policyengine-core dependency-map -c policyengine_ustraces which variables read each parameter and which variables feed which, and writes them asreadersandconsumerswith the package version, a sha256 fingerprint over its model surface (entities, parameters, system, variables), and the core version. Downstream: validation matching in policyengine-app-v2 (PolicyEngine/policyengine-app-v2#1180 consumes this shape), the calibration dashboard's model-coverage page (which derives the same edges by grepping source today), reform classifiers.Populations:
tests(default) traces the package's YAML tests, one per newly covered output variable per file;--every-testtraces all;microdatatraces every variable over a subsample of the package's default dataset;bothunions them. Reads ofgov.abolitions.*are skipped: core's neutralisation check reads them before the formula, so they are a switch per variable, not a dependency.Country packages need no wrapper. The release job that publishes the map per version belongs in each country repo and is a follow-up.
Design note: no cache clearing
The first version of this branch cleared the parameter at-instant caches whenever tracing switched or the tracer changed. Profiling against policyengine-us showed each clear rebuilds the whole parameter tree (~2 s), and branch simulations swap tracers per formula, so a traced microsim run went from 11 s to 29 s and a traced household calculation from 0.3 s to 4 s. The cache now holds plain nodes only;
_get_at_instantwraps one inTracingParameterNodeAtInstanton the way out when tracing is on, and the wrapper reads the tracer and branch name from the parameter root at access time. Nothing is ever invalidated,set_tracingis three attribute writes, and parameter reads are now labelled with the current branch rather than whichever built the cache.Benchmark (policyengine-us 1.822.4, 2,000-household subsample, 19 output variables; household = one CA family,
household_net_income+income_tax)Same trace node count (120,947) and flat-trace entries (6,993) on both, so the tracer's variable-level output is unchanged; the only difference is that parameter reads are now present.
get_flat_trace,get_serialized_flat_trace, andcomputation_log.lines()all run on the traced household.Downstream consumers
policyengine-api (its own pins: policyengine-us 1.764.6), running
PolicyEngineCountry.calculate, which setstrace = Trueand returnscomputation_log.lines(aggregate=False, max_depth=10), on a two-person California household requesting income tax, refundable CTC, EITC, SNAP, SPM net income, and household net income:The API's household-calculation unit tests (
test_household_calculation_service,test_calculate_*,test_household_traces: 17 tests) pass on this branch.policyengine-us 1.822.4 on this branch: the Python suite (
tests/minuspolicy/andmicrosimulation/) passes except five tests intest_run_selective_tests.py, which fail identically on released core (they shell out to git and were run from site-packages). Its YAML tests forgov/irs,gov/states/ca, andgov/usdathrough this branch'spolicyengine-core testrunner: 2,504 passed in 6m46s. Itemisation branching (simulation.get_branch) is exercised by every income-tax calculation above.Structural reads and scale values (second review)
Formulas sometimes read a node's structure —
for state in p.waived_counties._children,set(p.zips._children.keys()),next(iter(p.limits._children.values())),len(node._children). The wrapper now hands out aTracingChildrenmapping for_children(and iterates / answersinitself the same way): structure reads are recorded at the node's canonical path with the child names as the value, and children come back through the parent's tracing so nested nodes stay wrapped. No._childrenpath ever reaches a trace or the map.Scale reads record
{"type", "thresholds", "rates" | "amounts"}as plain lists instead ofNone, so a serialized trace keeps the schedule.Verified
test_release_tagging.pyfailures are pre-existing on master (same result with this diff stashed).policyengine-core dependency-map -c policyengine_us --tests-root <ctc tests>recordsgov.irs.credits.ctc.amount.base → ctc_child_individual_maximum(scale read) andgov.irs.credits.ctc.refundable.fully_refundable → refundable_ctc(yearly read), with no monkeypatches.🤖 Generated with Claude Code