From 4cd7a4c22ee0acd8691a219221736941a69bf221 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 16 Jul 2026 19:31:32 -0400 Subject: [PATCH] Round-2 verification fixes: nested SVAR errors, contract hardening, population-claims sweep From Sol's post-merge verification pass over the night's merged work. Not-resolved items closed: the real multi-parameter Policy constructor now runs in the nightly contract job; boe_var contracts fail (not skip) under MACROMOD_REQUIRE_PE=1; SVAR shock names are pinned semantically (a demand/supply label swap now fails the contract test); the static- catalogue shape test uses resolve=False so the fast suite never pays the PE import; hosted population scoring is stated consistently across the homepage card, pe page, connect page, and README roadmap. New findings closed: 'macromod summary' errors actionably when a configured fallback checkout has no parseable results (the round-1 fix only caught the top-level error shape), and partial section errors print visibly; the integration README dev command now installs the [models] extra its own examples need, with OG-UK as the optional add; modal_app's stale hardcoded-path claim corrected; wiring tests pin the closure tri-state through the CLI and the []/{}-reform clean errors; the remote inventory asserts all ten tools. Co-Authored-By: Claude Fable 5 --- .github/workflows/contract.yml | 2 +- .gitignore | 6 +++++ README.md | 2 +- connect/index.html | 6 ++--- index.html | 4 ++-- integration/README.md | 11 +++++---- integration/modal_app.py | 5 ++-- integration/src/macromod/cli.py | 12 +++++++++- integration/tests/test_contract.py | 25 +++++++++++++++++-- integration/tests/test_core.py | 16 ++++++++++++- integration/tests/test_remote_mcp.py | 4 +++- integration/tests/test_wiring.py | 36 ++++++++++++++++++++++++++++ pe/index.html | 2 +- 13 files changed, 112 insertions(+), 19 deletions(-) diff --git a/.github/workflows/contract.yml b/.github/workflows/contract.yml index c5d13ae..8f9fca4 100644 --- a/.github/workflows/contract.yml +++ b/.github/workflows/contract.yml @@ -23,6 +23,6 @@ jobs: - name: Install with full models run: uv pip install --system -e ".[models]" pytest - name: Contract tests (failures fail, never skip) - run: pytest tests/test_contract.py --runslow -q + run: pytest tests/test_contract.py tests/test_og.py::test_og_build_policy_real --runslow -q env: MACROMOD_REQUIRE_PE: "1" diff --git a/.gitignore b/.gitignore index 6f5f2e7..d86d144 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,9 @@ integration/src/*.egg-info/ __pycache__/ *.pyc .DS_Store + +# Local env and deployment link files — never commit (a Vercel OIDC token +# was nearly published this way) +.env*.local +.env.local +.vercel/ diff --git a/README.md b/README.md index 0a29806..cd2da10 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,7 @@ non-real numbers as illustrative. - [x] Local MCP server (`python -m macromod.mcp_server`) - [x] Hosted MCP server (`https://policyengine--macromod-mcp-serve.modal.run/mcp`, auto-deployed by CI) - [x] OG-UK steady-state scoring (`macromod score --model og` / `macromod og-score`, local only) -- [ ] Population-level PolicyEngine reform scoring +- [x] Population-level PolicyEngine reform scoring (`population_reform_impact`, hosted and local) - [ ] Additional macroeconomic model classes - See [#1](https://github.com/PolicyEngine/macro/issues/1) — Rust port of the solver core diff --git a/connect/index.html b/connect/index.html index 8deacf8..8e77db5 100644 --- a/connect/index.html +++ b/connect/index.html @@ -750,9 +750,9 @@

Sector output, long-run

tax, National Insurance, Universal Credit, SNAP, EITC, and the rest — for the household you describe. Population-level analysis over representative microdata is in the same package; population-level - reform scoring is available locally (it needs large private microdata), - but is not on the hosted MCP server — the hosted PolicyEngine tools are - household-level only. + reform scoring runs on the hosted MCP server too (the private + microdata credential is provisioned server-side) and locally with a + HUGGING_FACE_TOKEN.

diff --git a/index.html b/index.html index ec86362..f4fd2ca 100644 --- a/index.html +++ b/index.html @@ -137,14 +137,14 @@

Bank of England structural VAR model

read the model → -
pe-microsimhosted: householdpopulation planned
+
pe-microsimhosted: householdhosted: population

PolicyEngine tax-benefit microsimulation

The micro member of the suite: PolicyEngine's tax-benefit microsimulation package for the UK and US — the same engine behind policyengine.org. Computes a specific family's taxes, benefits, and net income under current law or a reform, at person and household - resolution; population-level reform scoring is planned. + resolution; population-level reform scoring runs hosted and locally.

read the model → diff --git a/integration/README.md b/integration/README.md index 6f72243..2731f3a 100644 --- a/integration/README.md +++ b/integration/README.md @@ -13,7 +13,7 @@ one CLI and one MCP server: microdata (UK data is private: set `HUGGING_FACE_TOKEN`; the hosted deployment provisions it server-side). - **OG-UK** (`oguk`, optional/local-only): overlapping-generations - steady-state scoring through `score_reform --model og`. + steady-state scoring through `macromod score --model og`. `score_reform` is the one reform vocabulary across the suite: the same flat `{parameter_path: value}` dict as the microsimulation tools, dispatched to a @@ -36,11 +36,14 @@ pip install "macromod[models] @ git+https://github.com/PolicyEngine/macro#subdir A shorter `pip install macromod` will come with PyPI publication. -For development, install local checkouts editable into a fresh env: +For development, install with the full model set (policyengine included via +the `[models]` extra), overriding the two model packages with local editable +checkouts; OG-UK is optional and local-only: ```bash -uv venv && uv pip install -e ./integration \ - -e ../obr-macroeconomic-model -e ../boe-var-model pytest +uv venv && uv pip install -e "./integration[models]" pytest \ + -e ../obr-macroeconomic-model -e ../boe-var-model +uv pip install "oguk @ git+https://github.com/PSLmodels/OG-UK" # optional, for --model og ``` ## CLI diff --git a/integration/modal_app.py b/integration/modal_app.py index 47dd322..704ac18 100644 --- a/integration/modal_app.py +++ b/integration/modal_app.py @@ -9,8 +9,9 @@ ASGI app at https://policyengine--macromod-mcp-serve.modal.run/mcp Both model repos resolve their data files relative to their own repo root -(`Path(__file__)`-relative), and `macromod.core` hardcodes -/Users/janansadeqian/boe-var-model. We therefore bake the repos into the +(`Path(__file__)`-relative); `macromod.core`'s svar_summary falls back to a +checkout via MACROMOD_BOE_VAR_REPO only when boe_var is absent (it is +installed here, so the fallback never fires). We bake the repos into the image at the SAME absolute paths and `pip install -e` them, so every path resolves in the container with zero patching: - obr_macro: /Users/janansadeqian/obr-macroeconomic-model (+ data/) diff --git a/integration/src/macromod/cli.py b/integration/src/macromod/cli.py index 60ec496..5009341 100644 --- a/integration/src/macromod/cli.py +++ b/integration/src/macromod/cli.py @@ -141,11 +141,19 @@ def summary(as_json): res = core.svar_summary() if set(res) == {"error"}: raise click.ClickException(res["error"]) + rep = res.get("replication", {}) + fr_check = res.get("forecast_revision", {}) + if "error" in rep and "error" in fr_check: + raise click.ClickException( + "no parseable SVAR results — replication: " + f"{rep['error']}; forecast revision: {fr_check['error']}" + ) if as_json: _emit_json(res) return - rep = res.get("replication", {}) click.echo("Replication (results/summary.md)") + if "error" in rep: + click.echo(f" error: {rep['error']}", err=True) for ln in rep.get("metadata", []): click.echo(f" {ln}") fevd = rep.get("fevd_1yr_headline", []) @@ -154,6 +162,8 @@ def summary(as_json): click.echo(_table(fevd, list(fevd[0].keys()))) fr = res.get("forecast_revision", {}) click.echo("\nForecast-revision exercise (results/forecast_summary.md)") + if "error" in fr: + click.echo(f" error: {fr['error']}", err=True) for ln in fr.get("metadata", []): click.echo(f" {ln}") signs = fr.get("latest_shock_signs", []) diff --git a/integration/tests/test_contract.py b/integration/tests/test_contract.py index a0c2de3..acaf4bb 100644 --- a/integration/tests/test_contract.py +++ b/integration/tests/test_contract.py @@ -12,6 +12,7 @@ e.g. the Modal image. """ +import importlib import os import pytest @@ -19,12 +20,24 @@ from macromod import core +def _import_or_require(modname: str): + """Import an upstream module; in the dedicated contract job + (MACROMOD_REQUIRE_PE=1) a missing/broken install FAILS instead of + skipping — a contract run that silently skips is vacuous.""" + require = os.environ.get("MACROMOD_REQUIRE_PE") == "1" + try: + return importlib.import_module(modname) + except Exception as e: + msg = f"{modname} not importable: {type(e).__name__}: {e}" + pytest.fail(msg) if require else pytest.skip(msg) + + # --------------------------------------------------------------------------- # boe_var (fast) # --------------------------------------------------------------------------- def test_svar_headline_columns_exist_upstream(): - data = pytest.importorskip("boe_var.data") + data = _import_or_require("boe_var.data") for col in (core._COL_CPI, core._COL_GDP): assert col in data.COLUMNS, ( f"boe_var.data.COLUMNS no longer contains {col!r}; " @@ -36,7 +49,7 @@ def test_svar_identified_schema_aligned(): """WORLD_SHOCKS + UK_SHOCKS is the canonical schema the adapter uses; SHOCK_NAMES must stay positionally aligned with it (an upstream reorder must fail here, not silently shift UK shock probabilities).""" - analysis = pytest.importorskip("boe_var.analysis") + analysis = _import_or_require("boe_var.analysis") identified = sorted(analysis.WORLD_SHOCKS + analysis.UK_SHOCKS) assert len(identified) == 6 and len(set(identified)) == 6 assert all(0 <= j < len(analysis.SHOCK_NAMES) for j in identified) @@ -49,6 +62,14 @@ def test_svar_identified_schema_aligned(): if j not in identified ] assert all(n.startswith("Unident") for n in unidentified), unidentified + # Semantic alignment, not just structure: a swap of e.g. UK demand/supply + # labels would silently relabel probabilities — pin the exact names. + assert tuple( + analysis.SHOCK_NAMES[j] for j in analysis.WORLD_SHOCKS + ) == ("World demand", "World energy", "World supply") + assert tuple( + analysis.SHOCK_NAMES[j] for j in analysis.UK_SHOCKS + ) == ("UK demand", "UK supply", "UK mon. pol.") # --------------------------------------------------------------------------- diff --git a/integration/tests/test_core.py b/integration/tests/test_core.py index fe075b0..b138061 100644 --- a/integration/tests/test_core.py +++ b/integration/tests/test_core.py @@ -50,7 +50,7 @@ def test_svar_forecast_and_cache(): # --------------------------------------------------------------------------- def test_pe_list_common_parameters(): - params = core.pe_list_common_parameters() + params = core.pe_list_common_parameters(resolve=False) assert len(params) >= 8 for p in params: assert {"country", "path", "description", "unit"} <= set(p) @@ -165,3 +165,17 @@ def test_cli_summary_without_boe_var_errors_actionably(monkeypatch): res = CliRunner().invoke(main, ["summary"]) assert res.exit_code != 0 assert "MACROMOD_BOE_VAR_REPO" in res.output + + +def test_cli_summary_env_checkout_missing_files_errors(monkeypatch, tmp_path): + """A configured fallback whose files are missing must error, not print + empty headings and exit 0 (round-2 review, new finding 1).""" + from click.testing import CliRunner + + from macromod.cli import main + + _block_boe_var(monkeypatch) + monkeypatch.setenv("MACROMOD_BOE_VAR_REPO", str(tmp_path)) + res = CliRunner().invoke(main, ["summary"]) + assert res.exit_code != 0 + assert "no parseable SVAR results" in res.output diff --git a/integration/tests/test_remote_mcp.py b/integration/tests/test_remote_mcp.py index 8651743..c76748c 100644 --- a/integration/tests/test_remote_mcp.py +++ b/integration/tests/test_remote_mcp.py @@ -26,7 +26,9 @@ EXPECTED_TOOLS = { "score_reform", "obr_shock", "list_reform_variables", "forecast_uk", - "latest_shocks", "model_summary", + "latest_shocks", "model_summary", "calculate_household", + "household_reform_impact", "list_reform_parameters", + "population_reform_impact", } diff --git a/integration/tests/test_wiring.py b/integration/tests/test_wiring.py index 3bc0ac6..daac8ef 100644 --- a/integration/tests/test_wiring.py +++ b/integration/tests/test_wiring.py @@ -254,3 +254,39 @@ def test_core_obr_extreme_shock_is_wellformed(): res = core.obr_shock(var="CGG", shock=1_000_000, periods=2) assert res["periods"] == 2 and len(res["results"]) >= 2 json.dumps(res) + + +def test_cli_obr_shock_closure_tristate(runner, monkeypatch): + """Omitted --investment-closure must reach core as None (per-variable + default), not False — the TCPRO zero-effects footgun (review #15.1).""" + seen = [] + + def fake_obr_shock(**kwargs): + seen.append(kwargs["investment_closure"]) + return {"name": "x", "var": "TCPRO", "shock": -0.05, "periods": 1, + "investment_closure": True, "results": [], + "cumulative_delta_gdp_bn_over_shock_periods": 0.0, + "peak_pct_gdp": 0.0, "peak_period": "2025Q1"} + + monkeypatch.setattr(core, "obr_shock", fake_obr_shock) + for args, expected in [ + (["obr-shock", "--var", "TCPRO", "--shock", "-0.05", "--json"], None), + (["obr-shock", "--var", "TCPRO", "--shock", "-0.05", + "--investment-closure", "--json"], True), + (["obr-shock", "--var", "TCPRO", "--shock", "-0.05", + "--no-investment-closure", "--json"], False), + ]: + res = runner.invoke(main, args) + assert res.exit_code == 0, res.output + assert seen[-1] is expected + + +def test_cli_wrong_shaped_reform_is_clean_error(runner): + """Valid JSON of the wrong shape ('[]', '{}') must be a Click error, + never a traceback (review #15.4).""" + for cmd in (["score", "--model", "og"], ["og-score"]): + for bad in ("[]", "{}"): + res = runner.invoke(main, cmd + ["--reform", bad]) + assert res.exit_code != 0 + assert "Traceback" not in res.output + assert "non-empty" in res.output diff --git a/pe/index.html b/pe/index.html index d468262..cff7a03 100644 --- a/pe/index.html +++ b/pe/index.html @@ -212,7 +212,7 @@

What it answers — and what it doesn't yet.

  • It is a static microsimulation at heart — reforms are modelled statically, with optional post-hoc behavioural responses (e.g. labour supply elasticities). No general equilibrium: that second act is exactly what the macro members of the suite add.
    • -
  • PolicyEngine Macro integration status: the household tools — computing one specific family's taxes, benefits, and net income under current law or a reform — are hosted and live on the MCP server (and the macromod CLI): calculate_household, household_reform_impact, and list_reform_parameters. Representative-population reform scoring (aggregates, deciles, poverty, and inequality over whole microdata, via ensure_datasets + Simulation) is planned but not yet connected to the hosted service: the enhanced-FRS microdata lives in a private Hugging Face repository and the downloads are large, so it does not yet fit the zero-auth, scale-to-zero deployment.
    • +
  • PolicyEngine Macro integration status: the household tools — computing one specific family's taxes, benefits, and net income under current law or a reform — are hosted and live on the MCP server (and the macromod CLI): calculate_household, household_reform_impact, and list_reform_parameters. Representative-population reform scoring (population_reform_impact: budgetary impact, deciles, winners/losers over whole microdata) is hosted too — the private enhanced-FRS credential is provisioned server-side — and runs locally with a HUGGING_FACE_TOKEN.
  • Two countries: UK and US.