Fix Pandas compatibility, DCID collisions, and validation config for NCES_SchoolDistrict and NCES_PublicSchool - #2175
smarthg-gi wants to merge 28 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several updates to the US NCES demographics processing, including adding 'Total Staff' mapping, fixing a regex pattern for school grades, adding word boundaries to gender replacement keys to prevent incorrect replacements, fixing a missing comma in the school district configuration, and adding a validation configuration. The reviewer feedback recommends replacing the lambda assignment for _PV_FORMAT with a standard function to comply with PEP 8 and improve performance. Additionally, the reviewer advises completely removing various commented-out code blocks across the modified files to keep the codebase clean.
[P1] Test Regression & Fixture Desynchronization
[P2] Import Validation Compliance
[P3] Code Hygiene and Formatting
|
|
[P2] Clarify Semantic Unit and Threshold Value in DELETED_RECORDS_PERCENT (validation_config.json): Reasoning: In validator.py:L222-L231, percent is computed as (deleted_records_count / previous_obs_count) * 100 and compared directly against threshold. Thus, "threshold": 0.1 represents 0.1% (a 0.001 fraction). While the author’s rule description states "Strictly enforce historical deletion average threshold of 0.1%", the PR description merely states "Configured validation_config.json with historical deletion threshold" without justifying why an ultra-strict 0.1% buffer was selected (rather than the standard 10% mentioned in additional-guidelines.md). Requesting explicit justification or threshold adjustment is directly supported. [P2] Execution & Error Handling process.py:L105
[P2] Test Fixtures us_nces_demographics_district_school.csv:L1
[P3] Manifest Artifact Retention manifest.json:L14
|
[P2] Validation Configuration [P2] Execution & Error Handling [P2] Test Fixtures [P3] Manifest Artifact Retention |
[P1] Test Regression & Fixture Desynchronization: Missing golden test fixture update for newly ingested [P2] Import Validation Compliance [P3] Code Hygiene and Formatting |
smarthg-gi
left a comment
There was a problem hiding this comment.
Review summary
Reviewed pull request changes for NCES demographics processing across scripts/us_nces/**.
Positive findings
scripts/us_nces/common/prop_conf.py:135- Reusable function for Series positional indexing: Replacing the lambda assignment with_PV_FORMAT(pv)and wrapping inputs intuple(pv)gracefully handles both tuple inputs andpd.Seriesfrom.apply(axis=1)across Pandas 2.x/3.x, eliminatingKeyError: 1.scripts/us_nces/common/replacement_functions.py:375- Word boundaries on gender replacements: Adding regex word boundaries (\bfemale\b,\bmale\b) addresses the root cause of substring collisions, eliminating the need for downstreamstr.replace("FeMale", "Female")workarounds.scripts/us_nces/common/prop_conf.py:175- PyArrow / RE2 regex quantifier syntax fix: Updating_SCHOOL_GRADE_PATTERNfromGrade \d{,2}toGrade \d{1,2}complies with RE2 quantifier constraints and restores grade-level StatVar generation.scripts/us_nces/demographics/school_district/config.py:32- Comma separator unblocks staff column ingestion: Adding the missing comma after".*Adult Education.*"resolves implicit string literal concatenation that was silently suppressing".*Staff.*"matching.scripts/us_nces/common/us_education.py:440- Dynamic DC API root resolution: Removing'dc_api_root': Noneallowsdc_api_wrapperto inheritos.environ['DC_API_ROOT']when testing or pointing to custom endpoints.
Coverage
| File | Status | Result |
|---|---|---|
| scripts/us_nces/common/prop_conf.py | Reviewed | No findings (2 positive findings) |
| scripts/us_nces/common/replacement_functions.py | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/common/us_education.py | Reviewed | One P3 finding (1 positive finding) |
| scripts/us_nces/demographics/public_school/manifest.json | Reviewed | No findings |
| scripts/us_nces/demographics/public_school/validation_config.json | Reviewed | No findings |
| scripts/us_nces/demographics/public_school/test_data/sample_input/ | Reviewed | One P2 finding |
| scripts/us_nces/demographics/public_school/test_data/sample_output/us_nces_demographics_public_place.csv | Reviewed | No findings |
| scripts/us_nces/demographics/public_school/test_data/sample_output/us_nces_demographics_public_school.csv | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/config.py | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/demographics/school_district/manifest.json | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/validation_config.json | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/test_data/sample_input/ | Reviewed | One P2 finding |
| scripts/us_nces/demographics/school_district/test_data/sample_output/us_nces_demographics_district_place.csv | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/test_data/sample_output/us_nces_demographics_district_school.csv | Reviewed | No findings |
Verification and limitations
- Checks run:
- Fetched base (
5f61b980) and head (a815f567) commits and generated authoritative diff. - Inspected PR metadata, commit history, and prior comment resolutions via GitHub API.
- Loaded and applied supplemental review guidelines from
gs://datcom-prod-imports/agents/skills/dc-import-code-review/additional-guidelines.md. - Inspected production GCS directory structure for
public_schoolandschool_districtsemi-automated input files. - Verified place deduplication logic across school types.
- Fetched base (
- Checks not run:
- Automated
./run_tests.shcould not run in the local environment due to proxy 403 on virtualenv dependency download.
- Automated
- Limitations:
- Cloud Batch execution was verified via user-provided test job run (
nces-publicschool-smarthg-20260901-162007).
- Cloud Batch execution was verified via user-provided test job run (
smarthg-gi
left a comment
There was a problem hiding this comment.
Review summary
Reviewed pull request changes for NCES demographics processing across scripts/us_nces/** at head commit d2eae4ad.
Findings
No actionable findings.
All prior review findings and comment threads have been addressed and resolved:
- Multi-File Test Fixtures: Commit
d2eae4adrestored multi-file sample input fixtures across different years for bothpublic_schoolandschool_district. Automated unit tests (public_school/process_test.pyandschool_district/process_test.py) pass cleanly and verify multi-file place ingestion and deduplication. - Private School Scope: Clarified that
NCES_PrivateSchoolis intentionally excluded from this PR's scope and will be updated in a dedicated PR with its own testing and validation runs. - PR Description: Updated to accurately describe the
pd.concat()and.groupby("school_state_code").first()architecture.
Positive findings
scripts/us_nces/common/prop_conf.py:135- Series positional indexing compatibility: Replacing the lambda assignment with_PV_FORMAT(pv)and wrapping inputs intuple(pv)gracefully handles both tuple inputs andpd.Seriesfrom.apply(axis=1)across Pandas 2.x/3.x, eliminatingKeyError: 1.scripts/us_nces/common/replacement_functions.py:375- Word boundaries on gender replacements: Adding regex word boundaries (\bfemale\b,\bmale\b) addresses the root cause of substring collisions, eliminating the need for downstreamstr.replace("FeMale", "Female")workarounds.scripts/us_nces/common/prop_conf.py:175- PyArrow / RE2 regex quantifier syntax fix: Updating_SCHOOL_GRADE_PATTERNfromGrade \d{,2}toGrade \d{1,2}complies with RE2 quantifier constraints and restores grade-level StatVar generation.scripts/us_nces/demographics/school_district/config.py:32- Comma separator unblocks staff column ingestion: Adding the missing comma after".*Adult Education.*"resolves implicit string literal concatenation that was silently suppressing".*Staff.*"matching.scripts/us_nces/common/us_education.py:440- Dynamic DC API root resolution: Removing'dc_api_root': Noneallowsdc_api_wrapperto inheritos.environ['DC_API_ROOT']properly when targeting staging or local endpoints.scripts/us_nces/demographics/public_school/test_data/sample_input/&school_district/test_data/sample_input/- Multi-file fixture sizing and coverage: Compact multi-file test fixtures (7-9 KB and ~20 rows each, well below the 100 KB limit) provide continuous test coverage for multi-file concatenation and deduplication without checking in large data dumps.
Coverage
| File | Status | Result |
|---|---|---|
| scripts/us_nces/common/prop_conf.py | Reviewed | No findings (2 positive findings) |
| scripts/us_nces/common/replacement_functions.py | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/common/us_education.py | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/demographics/public_school/manifest.json | Reviewed | No findings |
| scripts/us_nces/demographics/public_school/validation_config.json | Reviewed | No findings |
| scripts/us_nces/demographics/public_school/test_data/sample_input/ | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/demographics/public_school/test_data/sample_output/us_nces_demographics_public_place.csv | Reviewed | No findings |
| scripts/us_nces/demographics/public_school/test_data/sample_output/us_nces_demographics_public_school.csv | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/config.py | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/demographics/school_district/manifest.json | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/validation_config.json | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/test_data/sample_input/ | Reviewed | No findings (1 positive finding) |
| scripts/us_nces/demographics/school_district/test_data/sample_output/us_nces_demographics_district_place.csv | Reviewed | No findings |
| scripts/us_nces/demographics/school_district/test_data/sample_output/us_nces_demographics_district_school.csv | Reviewed | No findings |
Verification and limitations
- Checks run:
- Fetched base (
5f61b980) and latest head (d2eae4ad) commits and generated authoritative diff. - Inspected PR metadata, commit history, and prior comment resolutions via GitHub API.
- Loaded and applied supplemental review guidelines from
gs://datcom-prod-imports/agents/skills/dc-import-code-review/additional-guidelines.md. - Executed unit tests in a clean detached worktree at head commit
d2eae4ad:python3 scripts/us_nces/demographics/school_district/process_test.py->OK(2 tests passed)python3 scripts/us_nces/demographics/public_school/process_test.py->OK(2 tests passed)
- Fetched base (
- Checks not run:
- Automated
./run_tests.shcould not run in the local environment due to proxy 403 on virtualenv dependency download (cuda-toolkit-13.0.3.0-py2.py3-none-any.whl). Direct unit tests were executed with Python 3 instead.
- Automated
- Limitations:
- Production Cloud Batch execution was verified via user-provided successful batch test job (
nces-publicschool-smarthg-20260901-162007).
- Production Cloud Batch execution was verified via user-provided successful batch test job (
…hool_district manifests
…lidation_config.json
…on 3.12 syntax warning
Import: NCES_SchoolDistrict (Places)
Test job run: nces-schooldistrict-smarthg-20261004-114152
validation_output.csv
differ_summary.json
Import: NCES_SchoolDistrictStats (Stats)
Test job run: nces-schooldistrictstats-smarthg-20261004-114127
validation_output.csv
differ_summary.json
Import: NCES_PublicSchool (Places)
Test job run: nces-publicschool-smarthg-20261004-114221
validation_output.csv
differ_summary.json
Import: NCES_PublicSchoolStats (Stats)
Test job run: nces-publicschoolstats-smarthg-20261004-114239
validation_output.csv
Summary of Changes
NCES_SchoolDistrictandNCES_PublicSchoolimports into dedicated Place and Stats imports (NCES_SchoolDistrict/NCES_SchoolDistrictStats,NCES_PublicSchool/NCES_PublicSchoolStats) so that place imports can be processed and ingested independently into the Knowledge Graph before stats imports run.raise FileNotFoundErrorwhen no CSV files are matched in input directories to prevent silent empty runs.validation_config.jsonfrom Place imports (which contain only static node MCFs and do not carry statistical time-series data requiring date consistency validation), keeping full validation enabled on Stats imports._PV_FORMATinputs intuple(pv)to resolve Series positional indexingKeyError: 1._SCHOOL_GRADE_PATTERNfrom\d{,2}to\d{1,2}to restore 135 grade-level StatVars in Pandas 3.0+.\bfemale\b) to_GENDERregex, mapped"Total Staff"to"Faculty"population type, and added the missing comma inschool_district/config.pyto prevent duplicate StatVar collisions and dropped demographic columns._format_fips_code(val, length)with 5-digit zero-padding and conditionalzip/prefixing to eliminate leading-zero truncation for Northeast schools/districts (e.g., Marlborough MA01752).np.nanbefore.groupby("school_state_code").first()to prevent empty-string attribute shadowing, and replaced pairwise outer joins with vectorized coalescing to eliminate memory bottlenecks.'dc_api_root': Noneacross place transformations so requests dynamically inheritos.environ['DC_API_ROOT'].dc_api_is_defined_dcidusingunittest.mock.patchacross both test suites to eliminate external network dependencies, 403 API errors, and latency.MAX_DATE_CONSISTENTand dynamicSQL_VALIDATORdate freshness verification (max_year >= CURRENT_DATE - 2) across stats validation configs to detect data staleness.public_school/process.pyandschool_district/process.pytodef main(argv):executed viaabsl.app.run(main).invoke_differ_tool: falseforNCES_PublicSchoolStatsto avoid OOM failures on the 130 GB MCF node dataset while preserving 100% data integrity.provenance_urltohttps://nces.ed.gov/in bothpublic_school/manifest.jsonandschool_district/manifest.json, linkedvalidation_config.jsonwith a 0.1% deleted records threshold, and added wildcardnode_mcfloading.Linked Issues & Reviews