[Commerce_NTIA] Modified preprocess.py script - #2222
Krishnam24maheshwari wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces unit and regression tests, adds validation configuration, and refactors the preprocessing script for the Commerce NTIA import, including better error handling and output counters. The code reviewer recommends specifying exact MCF files in the manifest instead of using wildcards to prevent cross-contamination, removing a redundant directory creation call, and refactoring nested lambda functions into a cleaner dictionary mapping.
abhishekjaisw
left a comment
There was a problem hiding this comment.
Review scope
- Target: PR #2222 (
e780f0d0vs0226108d) - Reviewed:
manifest.json,validation_config.json,preprocess.py,commerce_ntia_test.py,README.md - Skipped: None
Summary
The core fix for b/552394335 (adding browser HEADERS and fail-fast download verification in preprocess.py) is verified and succeeded in Cloud Batch test job commerce-ntia-krishnamm-20260914-075721 (datcom-infosys-dev, us-east1, SUCCEEDED in 377.93s). However, inspection of the PR diff and GCS test artifacts (gs://datcom-import-test/.../2026_09_14T01_00_19_735237_07_00/) revealed 1 P1 and 3 P2 issues (plus 1 P3 cleanup) that should be addressed before merging.
Positive findings
statvar_imports/ntia_internet_use_survey/commerce_ntia/preprocess.py:39- Resilient HTTP request headers for federal portal downloads ✓- Finding: Good - Supplies standard browser
User-AgentandAcceptheaders todownload_file(), resolving upstream HTTP 403 Forbidden rejections fromntia.gov.
- Finding: Good - Supplies standard browser
statvar_imports/ntia_internet_use_survey/commerce_ntia/preprocess.py:107- Explicit download verification before transformation ✓- Finding: Good - Verifies
success,os.path.exists(INPUT_FILE), andos.path.getsize(INPUT_FILE) > 0before invokingpreprocess_data().
- Finding: Good - Verifies
statvar_imports/ntia_internet_use_survey/commerce_ntia/manifest.json:16- Counter generation and retention configured ✓- Finding: Good - Adds
--output_counterstostat_var_processor.pyand retains"counters/*.csv"undersource_files.
- Finding: Good - Adds
Coverage
| File | Status | Result |
|---|---|---|
statvar_imports/ntia_internet_use_survey/commerce_ntia/manifest.json |
Reviewed | One P1 finding, one P2 finding |
statvar_imports/ntia_internet_use_survey/commerce_ntia/validation_config.json |
Reviewed | One P2 finding |
statvar_imports/ntia_internet_use_survey/commerce_ntia/preprocess.py |
Reviewed | One P2 finding, one P3 finding |
statvar_imports/ntia_internet_use_survey/commerce_ntia/commerce_ntia_test.py |
Reviewed | No actionable findings |
statvar_imports/ntia_internet_use_survey/commerce_ntia/README.md |
Reviewed | No findings |
| "template_mcf": "output_files/ntia_output.tmcf", | ||
| "cleaned_csv": "output_files/ntia_output.csv" | ||
| "cleaned_csv": "output_files/ntia_output.csv", | ||
| "node_mcf": "output_files/*.mcf" |
There was a problem hiding this comment.
[P1] Shared "node_mcf": "output_files/*.mcf" causes cross-contamination and duplicate schema ingestion across import_inputs
Finding: Both import_inputs[0] (ntia_output) and import_inputs[1] (ntia_age_output) write to output_files/ and specify "node_mcf": "output_files/*.mcf". Because ntia_age_output produces 0 new StatVars, stat_var_processor.py does not write an .mcf file for input1, and input1 matches and ingests output_files/ntia_output_stat_vars.mcf generated by input0.
Impact: In the GCS test run (2026_09_14T01_00_19_735237_07_00), ntia_age_output_counters.csv shows ntia_age_output generated 0 new schema nodes (nodes-matched: 283, nodes-with-diff: 0), but input1/validation/differ_summary.json reports "added_schema_count": 2 because input1 ingested output_files/ntia_output_stat_vars.mcf from input0, duplicating Count_Person_Civilian_Unemployed and Count_Person_Civilian_Employed into input1.
Recommendation: Scope the wildcard pattern to each output prefix so glob.has_magic() remains true while preventing cross-contamination:
import_inputs[0]:"node_mcf": "output_files/ntia_output*.mcf"import_inputs[1]:"node_mcf": "output_files/ntia_age_output*.mcf"
| "template_mcf": "output_files/ntia_age_output.tmcf", | ||
| "cleaned_csv": "output_files/ntia_age_output.csv" | ||
| "cleaned_csv": "output_files/ntia_age_output.csv", | ||
| "node_mcf": "output_files/*.mcf" |
There was a problem hiding this comment.
[P2] Enabling node_mcf exposes conflicting StatVar schema definitions for Count_Person_Civilian_Employed and Count_Person_Civilian_Unemployed
Finding: In ntia_output_counters.csv, stat_var_processor.py reports "nodes-with-diff": 2 (pvs-added-age: 2, pvs-added-employment: 2, pvs-deleted-employmentStatus: 1, pvs-modified-employmentStatus: 1) and writes Count_Person_Civilian_Employed and Count_Person_Civilian_Unemployed to output_files/ntia_output_stat_vars.mcf. Querying the Data Commons V2 API confirms that Count_Person_Civilian_Employed and Count_Person_Civilian_Unemployed already exist in canonical schema as BLS labor force series with age: Years16Onwards and employment: BLS_Employed / BLS_Unemployed. Because ntia_pvmap.csv (lines 121–122) clears Age/age (Age,"""""",age,"""""") on isPerson/isAdult rows, stat_var_processor generates colliding DCIDs without age while keeping name: "Not Active-Duty Military","Person Aged 3 and above","employed".
Impact: Adding "node_mcf" ingests these 2 conflicting schema nodes (added_schema_count: 2 in differ_summary.json), attempting to overwrite canonical BLS StatVar definitions with incompatible properties and misleading name attributes.
Recommendation: Preserve the appropriate age constraint (e.g. Years15Onwards or Years3Onwards) in ntia_pvmap.csv so distinct NTIA StatVar DCIDs are generated without colliding with canonical BLS labor force StatVars.
| }, | ||
| { | ||
| "rule_id": "check_max_date_freshness", | ||
| "description": "Checks that the latest observation date is at least 2023.", |
There was a problem hiding this comment.
[P2] Global SELECT MAX(MaxDate) aggregate fails to validate per-StatVar or cohort date freshness
Finding: SELECT MAX(MaxDate) AS max_date FROM stats reduces all 2,355 StatVars in input0 (and 283 in input1) to a single scalar maximum value. It passes as long as at least one StatVar in the import has MaxDate >= '2023'.
Impact: In summary_report.csv, StatVars fall into four MaxDate cohorts (input0: 1,935 at 2023-11, 360 at 2015-07, 30 at 2019-11, 30 at 2013-07; input1: 227 at 2023-11, 48 at 2015-07, 4 at 2019-11, 4 at 2013-07). If 1,934 of the 1,935 active StatVars in input0 lose their 2023-11 data, SELECT MAX(MaxDate) still returns '2023-11' and passes.
Recommendation: Replace with input-aware cohort and floor SQL checks:
{
"rule_id": "check_active_survey_wave_count",
"description": "Checks that all active StatVars (1935 in input0, 227 in input1) have MaxDate >= 2023-11.",
"validator": "SQL_VALIDATOR",
"params": {
"query": "SELECT COUNT(*) AS total_svs, SUM(CASE WHEN MaxDate >= '2023-11' THEN 1 ELSE 0 END) AS active_svs FROM stats",
"condition": "active_svs >= CASE WHEN total_svs > 500 THEN 1935 ELSE 227 END"
}
},
{
"rule_id": "check_statvar_min_max_date",
"description": "Checks that no StatVar regresses below the oldest historical wave (2013-07).",
"validator": "SQL_VALIDATOR",
"params": {
"query": "SELECT StatVar, MaxDate FROM stats",
"condition": "MaxDate >= '2013-07'"
}
}|
|
||
| df2_cols_to_keep = [col for col in org_df.columns if not col.startswith('age')] | ||
| # 2. Process General survey data | ||
| df2_cols_to_keep = [ |
There was a problem hiding this comment.
[P2] Changing not col.startswith('age') to col not in set(AGE_COLUMNS) leaks 15 unmapped age*Prop/age*SE columns into ntia-data.csv
Finding: ntia-analyze-table.csv contains 20 columns starting with age, but AGE_COLUMNS only lists the 5 Count columns. The remaining 15 proportion and standard-error columns (age314Prop, age314PropSE, age314CountSE, etc.) are now retained in ntia-data.csv even though none of them are mapped in ntia_pvmap.csv.
Impact: Adds 15 unmapped columns per row in ntia-data.csv (ignored-svobs-pvs: 16968 in ntia_output_counters.csv).
Recommendation: Filter using the 5 age-group prefixes (not col.startswith(('age314', 'age1524', 'age2544', 'age4564', 'age65p'))), which excludes all 20 age-breakdown columns while preserving any hypothetical column like agencyAccess.
|
|
||
| # 1. Process Age-only data | ||
| df1 = org_df[COMMON_COLUMNS + AGE_COLUMNS].copy() | ||
| df1['universeAgeResol'] = df1['universe'].apply( |
There was a problem hiding this comment.
[P3] Duplicated ternary lambdas and unreachable sys.exit(1) calls after logging.fatal()
Finding: The ternary lambda mapping 'isPerson' -> 'CivilPerson' and 'isAdult' -> 'Adult' is repeated 4 times (lines 66, 70, 81, 85). Also, sys.exit(1) after logging.fatal() (lines 92, 110, 114) is unreachable in production because absl.logging.fatal() calls os.abort().
Recommendation: Extract _AGE_RESOL_MAP = {'isPerson': 'CivilPerson', 'isAdult': 'Adult'} with .map(_AGE_RESOL_MAP), and set mock_fatal.side_effect = SystemExit(1) in commerce_ntia_test.py.
This PR updates the Commerce NTIA import pipeline (statvar_imports/ntia_internet_use_survey/commerce_ntia/). It refactors the download and preprocessing scripts with robust error handling and headers, introduces hermetic unit tests, registers node_mcf and output counters in manifest.json, adds data validation checks, and fixes documentation casing.
Key Changes
input0 : https://storage.mtls.cloud.google.com/datcom-import-test/statvar_imports/ntia_internet_use_survey/commerce_ntia/Commerce_NTIA/2026_09_14T01_00_19_735237_07_00/input0/validation/validation_output.csv
differ0 : https://storage.mtls.cloud.google.com/datcom-import-test/statvar_imports/ntia_internet_use_survey/commerce_ntia/Commerce_NTIA/2026_09_14T01_00_19_735237_07_00/input0/validation/differ_summary.json
input1 : https://storage.mtls.cloud.google.com/datcom-import-test/statvar_imports/ntia_internet_use_survey/commerce_ntia/Commerce_NTIA/2026_09_14T01_00_19_735237_07_00/input1/validation/validation_output.csv
differ1 : https://storage.mtls.cloud.google.com/datcom-import-test/statvar_imports/ntia_internet_use_survey/commerce_ntia/Commerce_NTIA/2026_09_14T01_00_19_735237_07_00/input1/validation/differ_summary.json