From fa81d568fabbf2e2a55154bd1f694f44f2b004d4 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:22:18 +0400 Subject: [PATCH 1/4] Remove policyengine_core.charts.api and the chart functions it exported get_api_chart_data read the Public API's GET /{country}/metadata, then requested GET /{country}/economy/{reform}/over/{baseline} once a second until the result was ready, so a chart call could start a full society-wide calculation on the production Public API. intra_decile_chart and decile_chart plotted that result. policyengine_core.charts re-exported all three. The Public API depends on the country packages, which depend on Core, so these functions made Core depend on the service at run time. Remove the module and its re-export; the formatting helpers and bar charts in policyengine_core.charts stay. The new tests check that the module cannot be found and that policyengine_core.charts no longer has the three names. Co-Authored-By: Claude Opus 5.5 --- .../remove-public-api-functions.removed.md | 1 + policyengine_core/charts/__init__.py | 1 - policyengine_core/charts/api.py | 226 ------------------ tests/core/test_no_public_api_calls.py | 27 +++ 4 files changed, 28 insertions(+), 227 deletions(-) create mode 100644 changelog.d/remove-public-api-functions.removed.md delete mode 100644 policyengine_core/charts/api.py create mode 100644 tests/core/test_no_public_api_calls.py diff --git a/changelog.d/remove-public-api-functions.removed.md b/changelog.d/remove-public-api-functions.removed.md new file mode 100644 index 000000000..79f0ac678 --- /dev/null +++ b/changelog.d/remove-public-api-functions.removed.md @@ -0,0 +1 @@ +Removed `policyengine_core.charts.api` and the chart functions it exported through `policyengine_core.charts` (`get_api_chart_data`, `intra_decile_chart` and `decile_chart`), which requested society-wide results from the PolicyEngine Public API. diff --git a/policyengine_core/charts/__init__.py b/policyengine_core/charts/__init__.py index f12564970..a488cffa9 100644 --- a/policyengine_core/charts/__init__.py +++ b/policyengine_core/charts/__init__.py @@ -12,4 +12,3 @@ ) from .bar import * -from .api import * diff --git a/policyengine_core/charts/api.py b/policyengine_core/charts/api.py deleted file mode 100644 index 4bcd6736c..000000000 --- a/policyengine_core/charts/api.py +++ /dev/null @@ -1,226 +0,0 @@ -import requests -import time -import plotly.graph_objects as go -from .formatting import ( - DARK_GRAY, - MEDIUM_DARK_GRAY, - WHITE, - BLUE_LIGHT, - BLUE_PRIMARY, - format_fig, -) - - -def get_api_chart_data( - country_id: str, - reform_policy_id: int, - chart_key: str, - region: str, - time_period: str, - baseline_policy_id: int = None, - version: str = None, -) -> dict: - if baseline_policy_id is None or version is None: - response = requests.get( - f"https://api.policyengine.org/{country_id}/metadata", - # Add timeouts so stalled API responses can't hang us forever - # (bug M11). - timeout=30, - ) - result = response.json().get("result", {}) - baseline_policy_id = result.get("current_law_id") - version = result.get("version") - url = f"https://api.policyengine.org/{country_id}/economy/{reform_policy_id}/over/{baseline_policy_id}?region={region}&time_period={time_period}&version={version}" - response = requests.get(url, timeout=60) - if not response.ok: - raise ValueError(response.text) - json = response.json() - while json.get("status") == "computing": - time.sleep(1) - response = requests.get(url, timeout=60) - if not response.ok: - raise ValueError(response.text) - json = response.json() - return json.get("result", {}).get(chart_key) - - -# Specific chart definitions - - -def intra_decile_chart( - country_id: str, - reform_policy_id: int, - region: str, - time_period: str, - baseline_policy_id: int = None, -) -> go.Figure: - impact = get_api_chart_data( - country_id=country_id, - reform_policy_id=reform_policy_id, - chart_key="intra_decile", - baseline_policy_id=baseline_policy_id, - region=region, - time_period=time_period, - ) - impact = {"intra_decile": impact} - - decile_numbers = list(range(1, 11)) - outcome_labels = [ - "Gain more than 5%", - "Gain less than 5%", - "No change", - "Lose less than 5%", - "Lose more than 5%", - ] - outcome_colours = [ - DARK_GRAY, - MEDIUM_DARK_GRAY, - WHITE, - BLUE_LIGHT, - BLUE_PRIMARY, - ][::-1] - - data = [] - - for outcome_label, outcome_colour in zip(outcome_labels, outcome_colours): - data.append( - { - "type": "bar", - "y": ["All"], - "x": [impact["intra_decile"]["all"][outcome_label]], - "name": outcome_label, - "legendgroup": outcome_label, - "offsetgroup": outcome_label, - "marker": { - "color": outcome_colour, - }, - "orientation": "h", - "text": [f"{impact['intra_decile']['all'][outcome_label] * 100:.0f}%"], - "textposition": "inside", - "textangle": 0, - "xaxis": "x", - "yaxis": "y", - "showlegend": False, - "hoverinfo": "none", - } - ) - - for outcome_label, outcome_colour in zip(outcome_labels, outcome_colours): - data.append( - { - "type": "bar", - "y": decile_numbers, - "x": impact["intra_decile"]["deciles"][outcome_label], - "name": outcome_label, - "marker": { - "color": outcome_colour, - }, - "orientation": "h", - "text": [ - f"{value * 100:.0f}%" - for value in impact["intra_decile"]["deciles"][outcome_label] - ], - "textposition": "inside", - "textangle": 0, - "xaxis": "x2", - "yaxis": "y2", - "hoverinfo": "none", - } - ) - - layout = { - "barmode": "stack", - "grid": { - "rows": 2, - "columns": 1, - }, - "yaxis": { - "title": "", - "tickvals": ["All"], - "domain": [0.91, 1], - }, - "xaxis": { - "title": "", - "tickformat": ".0%", - "anchor": "y", - "matches": "x2", - "showgrid": False, - "showticklabels": False, - }, - "xaxis2": { - "title": "Population share", - "tickformat": ".0%", - "anchor": "y2", - }, - "yaxis2": { - "title": "Income decile", - "tickvals": decile_numbers, - "anchor": "x2", - "domain": [0, 0.85], - }, - "uniformtext": { - "mode": "hide", - "minsize": 10, - }, - } - - return format_fig(go.Figure(data=data, layout=layout)).update_traces( - marker_line_width=0, - width=[0.9] * 10, - ) - - -def decile_chart( - country_id: str, - reform_policy_id: int, - region: str, - time_period: str, - baseline_policy_id: int = None, -) -> go.Figure: - impact = get_api_chart_data( - country_id=country_id, - reform_policy_id=reform_policy_id, - chart_key="decile", - baseline_policy_id=baseline_policy_id, - region=region, - time_period=time_period, - ) - impact = {"decile": impact} - - decile_numbers = list(range(1, 11)) - # Sort deciles by key order 1 to 10 - decile_values = [] - for i in decile_numbers: - decile_values.append(impact["decile"]["relative"][str(i)]) - - data = [ - { - "x": decile_numbers, - "y": decile_values, - "type": "bar", - "marker": { - "color": [ - DARK_GRAY if value < 0 else BLUE_PRIMARY for value in decile_values - ], - }, - "text": [f"{value:+.1%}" for value in decile_values], - "textangle": 0, - } - ] - - layout = { - "xaxis": { - "title": "Income decile", - "tickvals": decile_numbers, - }, - "yaxis": { - "title": "Relative change", - "tickformat": "+,.0%", - }, - "uniformtext": { - "mode": "hide", - "minsize": 8, - }, - } - - return format_fig(go.Figure(data=data, layout=layout)) diff --git a/tests/core/test_no_public_api_calls.py b/tests/core/test_no_public_api_calls.py new file mode 100644 index 000000000..e98b9923d --- /dev/null +++ b/tests/core/test_no_public_api_calls.py @@ -0,0 +1,27 @@ +"""Core must not call the PolicyEngine Public API. + +The Public API depends on the country packages, which depend on Core. Core +code that sends requests to api.policyengine.org reverses that dependency at +run time. +""" + +import importlib.util + +import policyengine_core.charts as charts + +REMOVED_CHART_FUNCTION_NAMES = [ + "get_api_chart_data", + "decile_chart", + "intra_decile_chart", +] + + +def test_public_api_chart_module_is_removed(): + assert importlib.util.find_spec("policyengine_core.charts.api") is None + + +def test_charts_package_does_not_export_public_api_chart_functions(): + exported_names = [ + name for name in REMOVED_CHART_FUNCTION_NAMES if hasattr(charts, name) + ] + assert exported_names == [] From fd495c7147c5c6266ae67b6f29d45ca082bb3178 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:31:02 +0400 Subject: [PATCH 2/4] Remove Reform.from_api and Reform.api_id, and keep the Public API host out of Core Reform.from_api downloaded a saved policy from the Public API's GET /{country}/policy/{id}, and Reform.api_id posted a reform's parameter values to POST /{country}/policy and returned the new policy's ID. They were the last code in Core that called the Public API. Remove them, together with what only they used: _api_period_range and its tests, classproperty, and the requests import in the reform module. requests stays a dependency because policyengine_core/data/dataset.py uses it. The docstrings of Reform.country_id and Reform.parameter_values, and the reforms usage page, no longer describe them as inputs to Public API calls. New tests fail if any file in the package source contains api.policyengine.org, or if Reform has a from_api or api_id attribute (looked up with inspect.getattr_static, so the check never runs them). The removed changelog fragment now covers both commits and releases the change as a minor version. Co-Authored-By: Claude Opus 5.5 --- .../remove-public-api-functions.removed.md | 2 +- docs/usage/reforms.ipynb | 8 +- policyengine_core/reforms/reform.py | 106 +----------------- tests/core/test_no_public_api_calls.py | 32 +++++- tests/core/test_reform_period_keys.py | 17 --- 5 files changed, 38 insertions(+), 127 deletions(-) diff --git a/changelog.d/remove-public-api-functions.removed.md b/changelog.d/remove-public-api-functions.removed.md index 79f0ac678..5755fa790 100644 --- a/changelog.d/remove-public-api-functions.removed.md +++ b/changelog.d/remove-public-api-functions.removed.md @@ -1 +1 @@ -Removed `policyengine_core.charts.api` and the chart functions it exported through `policyengine_core.charts` (`get_api_chart_data`, `intra_decile_chart` and `decile_chart`), which requested society-wide results from the PolicyEngine Public API. +Removed `Reform.from_api`, `Reform.api_id` and `policyengine_core.charts.api` (with the chart functions `get_api_chart_data`, `intra_decile_chart` and `decile_chart` that `policyengine_core.charts` exported from it), which sent HTTP requests to the PolicyEngine Public API; build reforms with `Reform.from_dict` instead. diff --git a/docs/usage/reforms.ipynb b/docs/usage/reforms.ipynb index b9bd4c2ea..c969559a0 100644 --- a/docs/usage/reforms.ipynb +++ b/docs/usage/reforms.ipynb @@ -25,7 +25,7 @@ "```python\n", "Reform.from_dict(\n", " parameter_values: dict, # Parameter path -> period -> value mappings\n", - " country_id: str = None, # Optional: country code for API integration\n", + " country_id: str = None, # Optional: the country the reform applies to\n", " name: str = None, # Optional: human-readable name for the reform\n", ") -> Reform\n", "```" @@ -197,9 +197,9 @@ "cell_type": "markdown", "metadata": {}, "source": [ - "### Named reforms for API integration\n", + "### Named reforms\n", "\n", - "When working with the PolicyEngine API, you can provide a `country_id` and `name` for better integration." + "You can provide a `country_id` and a `name`, which are stored on the reform class to identify it." ] }, { @@ -208,7 +208,7 @@ "metadata": {}, "outputs": [], "source": [ - "# Named reform with country ID (useful for API integration)\n", + "# Named reform with a country ID\n", "named_reform = Reform.from_dict(\n", " {\n", " \"benefits.basic_income\": {\n", diff --git a/policyengine_core/reforms/reform.py b/policyengine_core/reforms/reform.py index 9358ee5c1..afcee0358 100644 --- a/policyengine_core/reforms/reform.py +++ b/policyengine_core/reforms/reform.py @@ -14,17 +14,6 @@ Period, ) -import requests - - -class classproperty(object): - def __init__(self, f): - self.f = f - - def __get__(self, obj, owner): - return self.f(owner) - - # --------------------------------------------------------------------------- # Reform period-key handling. # @@ -88,23 +77,6 @@ def _update_start_instant(update_kwargs: dict): return instant_("0001-01-01") -def _api_period_range(start: str, stop: str) -> str: - """Clamp an API policy's ``[start, stop]`` to the supported window and - return an explicit ``"start.stop"`` key, so :meth:`Reform.from_dict` - treats every API policy as bounded. (A single-civil-year intersection - would otherwise stringify to a bare ``"YYYY"`` which, under the - from-onward rule, would extend the change past the policy's end.)""" - clamped = period_(_SCALAR_REFORM_PERIOD).intersection( - instant_(start), instant_(stop) - ) - if clamped is None: - raise ValueError( - f"Policy period '{start}.{stop}' does not overlap the supported " - f"reform window ({_SCALAR_REFORM_PERIOD})." - ) - return f"{clamped.start}.{clamped.stop}" - - class Reform(TaxBenefitSystem): """A modified TaxBenefitSystem @@ -138,10 +110,10 @@ class Reform(TaxBenefitSystem): """The name of the reform. This is used to identify the reform in the UI.""" country_id: str = None - """The country id of the reform. This is used to inform any calls to the PolicyEngine API.""" + """The country id of the reform, if one was given.""" parameter_values: dict = None - """The parameter values of the reform. This is used to inform any calls to the PolicyEngine API.""" + """The parameter values a reform created by :meth:`Reform.from_dict` applies.""" simulation: "Simulation" = None @@ -281,80 +253,6 @@ def apply(self): return reform - @staticmethod - def from_api( - api_id: str, - country_id: str = None, - ) -> Reform: - """Create a reform from a dictionary of parameters. - - Args: - parameters: A dictionary of parameter -> { period -> value } pairs. - - Returns: - A reform. - """ - - data = requests.get( - f"https://api.policyengine.org/{country_id}/policy/{api_id}", - # Timeout so a stalled API can't hang the caller forever - # (bug M11). - timeout=30, - ).json() - - parameter_values = data.get("result", {}).get("policy_json", {}) - - for path in parameter_values: - keys_to_remove = [] - for start_stop_str in list(parameter_values[path].keys()): - start, stop = start_stop_str.split(".") - time_period = _api_period_range(start, stop) - parameter_values[path][time_period] = parameter_values[path][ - start_stop_str - ] - keys_to_remove.append(start_stop_str) - for key in keys_to_remove: - del parameter_values[path][key] - - return Reform.from_dict( - parameter_values, - country_id, - data.get("result", {}).get("label", None), - ) - - @classproperty - def api_id(self): - if self.country_id is None: - raise ValueError( - "`country_id` is not set. This is required to use the API." - ) - if self.parameter_values is None: - raise ValueError( - "`parameter_values` is not set. This is required to use the API." - ) - - sanitised_parameter_values = {} - - for path, period_values in self.parameter_values.items(): - sanitised_period_values = {} - for period, value in period_values.items(): - period = period_(period) - sanitised_period_values[f"{period.start}.{period.stop}"] = value - sanitised_parameter_values[path] = sanitised_period_values - - response = requests.post( - f"https://api.policyengine.org/{self.country_id}/policy", - json={ - "data": sanitised_parameter_values, - "name": self.name, - }, - # Timeout so a stalled API can't hang the caller forever - # (bug M11). - timeout=30, - ) - - return response.json().get("result", {}).get("policy_id") - def set_parameter( path: Union[Parameter, str], diff --git a/tests/core/test_no_public_api_calls.py b/tests/core/test_no_public_api_calls.py index e98b9923d..d37067d46 100644 --- a/tests/core/test_no_public_api_calls.py +++ b/tests/core/test_no_public_api_calls.py @@ -2,12 +2,18 @@ The Public API depends on the country packages, which depend on Core. Core code that sends requests to api.policyengine.org reverses that dependency at -run time. +run time, so the package source must not name the host at all. """ import importlib.util +import inspect +from pathlib import Path import policyengine_core.charts as charts +from policyengine_core.reforms import Reform + +PACKAGE_SOURCE_DIRECTORY = Path(__file__).resolve().parents[2] / "policyengine_core" +PUBLIC_API_HOST = "api.policyengine.org" REMOVED_CHART_FUNCTION_NAMES = [ "get_api_chart_data", @@ -15,6 +21,30 @@ "intra_decile_chart", ] +REMOVED_REFORM_ATTRIBUTE_NAMES = ["from_api", "api_id"] + + +def test_package_source_does_not_name_the_public_api_host(): + files_naming_host = sorted( + str(path.relative_to(PACKAGE_SOURCE_DIRECTORY)) + for path in PACKAGE_SOURCE_DIRECTORY.rglob("*") + if path.is_file() + and "__pycache__" not in path.parts + and PUBLIC_API_HOST.encode() in path.read_bytes() + ) + assert files_naming_host == [] + + +def test_reform_has_no_public_api_functions(): + # getattr_static looks the names up without running them: reading + # Reform.api_id ran a property that posted the reform to the Public API. + remaining_names = [ + name + for name in REMOVED_REFORM_ATTRIBUTE_NAMES + if inspect.getattr_static(Reform, name, None) is not None + ] + assert remaining_names == [] + def test_public_api_chart_module_is_removed(): assert importlib.util.find_spec("policyengine_core.charts.api") is None diff --git a/tests/core/test_reform_period_keys.py b/tests/core/test_reform_period_keys.py index edff5ea6b..816cad0a8 100644 --- a/tests/core/test_reform_period_keys.py +++ b/tests/core/test_reform_period_keys.py @@ -4,7 +4,6 @@ from policyengine_core.periods import Instant, instant, period from policyengine_core.reforms.reform import ( - _api_period_range, _bounded_period_update_kwargs, _eternity_update_kwargs, _from_instant_update_kwargs, @@ -104,19 +103,3 @@ def test_period_key_update_kwargs_malformed_raises(): ) def test_update_start_instant(update_kwargs, expected): assert _update_start_instant(update_kwargs) == expected - - -# ---- from_api range normalization ----------------------------------------- - - -def test_api_period_range_single_year_stays_bounded(): - assert _api_period_range("2026-01-01", "2026-12-31") == "2026-01-01.2026-12-31" - - -def test_api_period_range_multi_year_stays_bounded(): - assert _api_period_range("2026-01-01", "2030-12-31") == "2026-01-01.2030-12-31" - - -def test_api_period_range_out_of_window_raises(): - with pytest.raises(ValueError): - _api_period_range("2200-01-01", "2300-01-01") From f11fa5653bb3a6fc545f09250cbb8222936d7c5b Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:39:29 +0400 Subject: [PATCH 3/4] Make the Public API tests fail instead of passing with nothing checked The host scan passed when the package directory it scans did not exist, because rglob then returns nothing; it now first asserts that the reform module is there, so moving the test or the package fails it. The Reform check used None as getattr_static's default, so an attribute set to None passed as absent; it now uses a sentinel object. Co-Authored-By: Claude Opus 5.5 --- tests/core/test_no_public_api_calls.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/core/test_no_public_api_calls.py b/tests/core/test_no_public_api_calls.py index d37067d46..d2c67a857 100644 --- a/tests/core/test_no_public_api_calls.py +++ b/tests/core/test_no_public_api_calls.py @@ -23,8 +23,12 @@ REMOVED_REFORM_ATTRIBUTE_NAMES = ["from_api", "api_id"] +ATTRIBUTE_NOT_FOUND = object() + def test_package_source_does_not_name_the_public_api_host(): + # Fail instead of passing with nothing scanned if the package moves. + assert (PACKAGE_SOURCE_DIRECTORY / "reforms" / "reform.py").is_file() files_naming_host = sorted( str(path.relative_to(PACKAGE_SOURCE_DIRECTORY)) for path in PACKAGE_SOURCE_DIRECTORY.rglob("*") @@ -41,7 +45,8 @@ def test_reform_has_no_public_api_functions(): remaining_names = [ name for name in REMOVED_REFORM_ATTRIBUTE_NAMES - if inspect.getattr_static(Reform, name, None) is not None + if inspect.getattr_static(Reform, name, ATTRIBUTE_NOT_FOUND) + is not ATTRIBUTE_NOT_FOUND ] assert remaining_names == [] From 1bfb518f46fe02c432e70073a4e445724d1137af Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:33:35 +0400 Subject: [PATCH 4/4] Remove the Public API code-health tests Drop tests/core/test_no_public_api_calls.py. The removal of Reform.from_api, Reform.api_id and policyengine_core.charts.api stays as it is. Co-Authored-By: Claude Opus 5.5 --- tests/core/test_no_public_api_calls.py | 62 -------------------------- 1 file changed, 62 deletions(-) delete mode 100644 tests/core/test_no_public_api_calls.py diff --git a/tests/core/test_no_public_api_calls.py b/tests/core/test_no_public_api_calls.py deleted file mode 100644 index d2c67a857..000000000 --- a/tests/core/test_no_public_api_calls.py +++ /dev/null @@ -1,62 +0,0 @@ -"""Core must not call the PolicyEngine Public API. - -The Public API depends on the country packages, which depend on Core. Core -code that sends requests to api.policyengine.org reverses that dependency at -run time, so the package source must not name the host at all. -""" - -import importlib.util -import inspect -from pathlib import Path - -import policyengine_core.charts as charts -from policyengine_core.reforms import Reform - -PACKAGE_SOURCE_DIRECTORY = Path(__file__).resolve().parents[2] / "policyengine_core" -PUBLIC_API_HOST = "api.policyengine.org" - -REMOVED_CHART_FUNCTION_NAMES = [ - "get_api_chart_data", - "decile_chart", - "intra_decile_chart", -] - -REMOVED_REFORM_ATTRIBUTE_NAMES = ["from_api", "api_id"] - -ATTRIBUTE_NOT_FOUND = object() - - -def test_package_source_does_not_name_the_public_api_host(): - # Fail instead of passing with nothing scanned if the package moves. - assert (PACKAGE_SOURCE_DIRECTORY / "reforms" / "reform.py").is_file() - files_naming_host = sorted( - str(path.relative_to(PACKAGE_SOURCE_DIRECTORY)) - for path in PACKAGE_SOURCE_DIRECTORY.rglob("*") - if path.is_file() - and "__pycache__" not in path.parts - and PUBLIC_API_HOST.encode() in path.read_bytes() - ) - assert files_naming_host == [] - - -def test_reform_has_no_public_api_functions(): - # getattr_static looks the names up without running them: reading - # Reform.api_id ran a property that posted the reform to the Public API. - remaining_names = [ - name - for name in REMOVED_REFORM_ATTRIBUTE_NAMES - if inspect.getattr_static(Reform, name, ATTRIBUTE_NOT_FOUND) - is not ATTRIBUTE_NOT_FOUND - ] - assert remaining_names == [] - - -def test_public_api_chart_module_is_removed(): - assert importlib.util.find_spec("policyengine_core.charts.api") is None - - -def test_charts_package_does_not_export_public_api_chart_functions(): - exported_names = [ - name for name in REMOVED_CHART_FUNCTION_NAMES if hasattr(charts, name) - ] - assert exported_names == []