Skip to content

[Commerce_NTIA] Modified preprocess.py script - #2222

Open
Krishnam24maheshwari wants to merge 4 commits into
datacommonsorg:masterfrom
Krishnam24maheshwari:commerce
Open

Krishnam24maheshwari wants to merge 4 commits into
datacommonsorg:masterfrom
Krishnam24maheshwari:commerce

Conversation

@Krishnam24maheshwari

@Krishnam24maheshwari Krishnam24maheshwari commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

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

  1. Preprocessing (preprocess.py)
  • Robust Download: Added HTTP browser headers (User-Agent, Accept) to download_file() to prevent request blocking, along with explicit error checking and logging on download failure.
  • Universe & Variable Age Resolution: Explicitly maps and preserves universeAgeResol and variableAgeResol (CivilPerson for isPerson, Adult for isAdult) across both general and age-specific subsets to ensure StatisticalVariables retain appropriate demographic resolution (Civilian, Age).
  • Directory Safety & Formatting: Ensured safe directory creation (os.makedirs(..., exist_ok=True)), clean error handling (sys.exit(1)), and standard code formatting.
  1. Validation Rules (validation_config.json)
  • Added data freshness check (check_max_date_freshness: SELECT MAX(MaxDate) AS max_date FROM stats WHERE max_date >= 2023).
  • Enforced deleted records threshold check (check_deleted_records_percent: 0.1).

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Krishnam24maheshwari Krishnam24maheshwari changed the title Modified preprocess.py script [Commerce_NTIA] Modified preprocess.py script Sep 14, 2026

@abhishekjaisw abhishekjaisw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review scope

  • Target: PR #2222 (e780f0d0 vs 0226108d)
  • 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-Agent and Accept headers to download_file(), resolving upstream HTTP 403 Forbidden rejections from ntia.gov.
  • 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), and os.path.getsize(INPUT_FILE) > 0 before invoking preprocess_data().
  • statvar_imports/ntia_internet_use_survey/commerce_ntia/manifest.json:16 - Counter generation and retention configured ✓
    • Finding: Good - Adds --output_counters to stat_var_processor.py and retains "counters/*.csv" under source_files.

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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants