Add typed local H5 request construction#749
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
4e33475 to
93d027a
Compare
dfef9d3 to
fa0346b
Compare
93d027a to
e076b94
Compare
d92611c to
ddee97f
Compare
juaristi22
left a comment
There was a problem hiding this comment.
Approving overall because the incremental migration direction looks sound and the dual-path worker boundary is a sensible step.
Two things I want us to consider before merge:
-
AreaBuildRequest.output_relative_pathis only validated as non-empty, andworker_script.pyjoins it directly withoutput_dir. That means an absolute path or../segments can escape the run directory. Now that--requests-jsonaccepts typed request payloads directly, I think we should either harden this path handling or at least make the constraint explicit. -
The new contract distinguishes
area_idfromdisplay_name, but the validation path still usesdisplay_nameas the emittedarea_id. Worker completion keys are built fromarea_id, while validation rows and downstream summaries can end up keyed bydisplay_name, which will drift as soon as those fields differ.
Neither is blocking for me on this PR, but I do think they are worth a deliberate check before we merge.
|
Please rebase from upstream/main — there are merge conflicts. |
a338f7e to
f848260
Compare
Fixes #748
Summary
This is PR 2 in the incremental local H5 publication migration.
It introduces typed request construction for local H5 workers and makes
USAreaCatalogthe canonical owner of request-construction rules, while preserving backward compatibility at the worker boundary.This is also the first PR that introduces the request contract types actually used by runtime code:
AreaFilterAreaBuildRequestHow It Fits Into The Broader Migration
PR 1 now introduces only the pure partitioning seam.
This PR adds the next boundaries:
local_h5/requests.pyfor typed request values used by the worker pathlocal_h5/area_catalog.pyfor US request constructionAreaBuildRequestsupport at the worker boundaryThe coordinator is intentionally not migrated yet. That remains planned for PR 9.
What This PR Does
policyengine_us_data/calibration/local_h5/requests.pypolicyengine_us_data/calibration/local_h5/area_catalog.pyAreaFilterandAreaBuildRequestmodal_app/worker_script.pyto accept typed--requests-jsoninput--work-itemssupportUSAreaCatalog.default()modal_app/local_area.pythat coordinator-side inline request construction is temporary and scheduled for PR 9 migrationWhat This PR Does Not Do
modal_app/local_area.pyto emit typed requestswork_itemsbuild_h5(...)internalsExisting Interfaces Preserved
These remain supported after this PR:
modal_app/worker_script.py --work-items ...build_h5(...)Internal Boundaries Replaced Or Made Canonical
USAreaCatalogis now the canonical place for new request rulesAreaBuildRequestJSON is now supported at the worker boundaryMigration State After This PR
After merge:
USAreaCatalogTesting
Ran locally:
python -m py_compile policyengine_us_data/calibration/local_h5/__init__.py policyengine_us_data/calibration/local_h5/requests.py policyengine_us_data/calibration/local_h5/partitioning.py policyengine_us_data/calibration/local_h5/area_catalog.py modal_app/local_area.py modal_app/worker_script.py tests/unit/calibration/fixtures/test_local_h5_requests.py tests/unit/calibration/test_local_h5_requests.py tests/unit/calibration/fixtures/test_local_h5_partitioning.py tests/unit/calibration/test_local_h5_partitioning.py tests/unit/calibration/fixtures/test_local_h5_area_catalog.py tests/unit/calibration/test_local_h5_area_catalog.py tests/unit/fixtures/test_modal_local_area.py tests/unit/test_modal_local_area.py tests/unit/fixtures/test_modal_worker_script.py tests/unit/test_modal_worker_script.pyruff check policyengine_us_data/calibration/local_h5/__init__.py policyengine_us_data/calibration/local_h5/requests.py policyengine_us_data/calibration/local_h5/partitioning.py policyengine_us_data/calibration/local_h5/area_catalog.py modal_app/local_area.py modal_app/worker_script.py tests/unit/calibration/fixtures/test_local_h5_requests.py tests/unit/calibration/test_local_h5_requests.py tests/unit/calibration/fixtures/test_local_h5_partitioning.py tests/unit/calibration/test_local_h5_partitioning.py tests/unit/calibration/fixtures/test_local_h5_area_catalog.py tests/unit/calibration/test_local_h5_area_catalog.py tests/unit/fixtures/test_modal_local_area.py tests/unit/test_modal_local_area.py tests/unit/fixtures/test_modal_worker_script.py tests/unit/test_modal_worker_script.pypython -m pytest --noconftest tests/unit/calibration/test_local_h5_requests.py tests/unit/calibration/test_local_h5_partitioning.py tests/unit/calibration/test_local_h5_area_catalog.py tests/unit/test_modal_local_area.py tests/unit/test_modal_worker_script.py -q