Repository navigation
Conversation
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 <noreply@anthropic.com>
…t 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #628
What and why
Three parts of policyengine-core sent HTTP requests to the PolicyEngine Public API (api.policyengine.org):
Reform.from_apidownloaded a saved policy by its numeric ID (GET /{country}/policy/{id}) and built a reform from it.Reform.api_idsent the reform's parameter values toPOST /{country}/policyand returned the new policy's ID.policyengine_core.charts.api:get_api_chart_datareadGET /{country}/metadata, then requestedGET /{country}/economy/{reform}/over/{baseline}once a second until the result was ready, andintra_decile_chartanddecile_chartplotted it.policyengine_core.chartsre-exported all three.The Public API depends on the country packages, which depend on policyengine-core, so these functions made the lowest-level package depend on the top-level service at run time. The Public API's newer policy routes also have no lookup by these numeric IDs.
Changes
policyengine_core/charts/api.pyand its re-export frompolicyengine_core.charts. The formatting helpers and bar charts stay. Because the removed module had no__all__,from policyengine_core.charts import *also no longer provides therequestsandtimemodules it imported; nothing in the package relies on that.Reform.from_apiandReform.api_id, with the helpers only they used:_api_period_range(and its three tests),classproperty, and therequestsimport inpolicyengine_core/reforms/reform.py. Therequestsdependency stays, becausepolicyengine_core/data/dataset.pyuses it. The docstrings ofReform.country_idandReform.parameter_values, and the reforms usage page, no longer describe them as inputs to Public API calls.tests/core/test_no_public_api_calls.py:api.policyengine.org, after first checking that the package directory it scans exists, so a moved package cannot make it pass with nothing scanned;Reformhas afrom_apiorapi_idattribute (looked up withinspect.getattr_staticand a sentinel default, so the check never runs them and an attribute set toNonestill counts);policyengine_core.charts.apican be imported, or ifpolicyengine_core.chartshas any of the three chart functions.removedchangelog fragment, so the release is a minor version (3.33.12 to 3.34.0 at the time of writing).What this waits for
Do not merge until all of these have merged. Merging this PR publishes a new policyengine-core release to PyPI, and each of them stops using a removed function:
core_reform_apitest cases that callReform.from_api. policyengine-uk accepts anypolicyengine-core>=3.32.27, so its tests would fail as soon as this release is published.Reform.from_api.Reform.from_api.Reform.api_id.Reform.from_apiandReform.api_id.A code search of PolicyEngine's repositories found no other users. Article notebooks in PolicyEngine/policyengine-app-v2 still contain old cells that used these functions; the site shows their saved outputs and does not re-run them, so they are left as they are.
Environment variables
None. The change reads no environment variables and touches no workflows.
Tests
Each commit was red-green:
7ca5e511): the two chart tests failed with the module still present (find_specfoundpolicyengine_core.charts.api, andpolicyengine_core.chartshad all three names), and pass after the removal.ed369823): the host test failed namingreforms/reform.py, and theReformtest failed listingfrom_apiandapi_id; both pass after the removal.d5d2bccb, test module only): a check of the tests themselves showed the host scan passing when pointed at a directory that does not exist, and theReformcheck passing withReform.api_id = None; both now fail in those cases, and the four tests pass on the real source.uv run pytest tests(the full suite): 2663 passed, 4 skipped, 3 xfailed at commit 1; 2662 passed, 4 skipped, 3 xfailed at commit 2 (three_api_period_rangetests removed, two tests added).policyengine-core test policyengine_core/country_template/tests -c policyengine_core.country_template: 39 passed at commits 1 and 2.ruff format --check .andruff check .: pass at each commit.make documentation(the usage-page change is text in a notebook that the docs build does not execute) and the country smoke tests, which CI runs.🤖 Generated with Claude Code