Fix for epa-airpollutantemission-level1 - #2148
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request increases the resource limits (CPU, memory, and disk) in the manifest configuration and refactors pandas DataFrame operations in process.py to avoid using .loc for simple column assignments and to replace deprecated inplace=True usage. Feedback is provided to remove a redundant .replace('', np.nan) call on the observation column, as empty strings are already converted to NaN earlier in the processing pipeline.
abhishekjaisw
left a comment
There was a problem hiding this comment.
Review scope
- Target: PR #2148 (
4fc4609467faf6425589b232ce6e674c6677ec2d) - Reviewed:
scripts/us_epa/national_emissions_inventory/manifest.jsonscripts/us_epa/national_emissions_inventory/process.pyscripts/us_epa/national_emissions_inventory/validation_config.json
- Skipped: None
Summary of Findings
- [P1] CI Test Suite Failure (
data-pull-request-py) & Missing Regression Tests for 2017/2020 Nonpoint & Tribal Fixes (process.py:144,process_test.py)- Running
./run_tests.sh -p scripts/us_epa/national_emissions_inventory(executed by GitHub CIdata-pull-request-py) fails withImportError: attempted relative import with no known parent packageatprocess_test.py:20(from .process import *) becausescripts/us_epa/national_emissions_inventory/lacks an__init__.py. - Furthermore, PR #2148 modifies
_regularize_columnsto fix the silent drop of 767,314 observations across 2017/2020nonpoint,point_, and 2014tribesfiles, but adds zero unit tests or test fixtures for these branches.
- Running
- [P2] Missing Date Freshness Validation Rule in
validation_config.json(Outdated Branch / Already Fixed DuckDB Differ Issue) (validation_config.json:9)validation_config.jsonomits date freshness validation becauseSQL_VALIDATORpreviously failed on emptydiffer_df(Need a DataFrame with at least one column). However, this framework bug was already fixed onmasterin commits3315fc4b(Aug 14, 2026) and4d856505(Aug 31, 2026) intools/import_validation/validator.py:62-63andrunner.py:194. Rebasing PR #2148 ontomasterenables re-adding theSQL_VALIDATORdate freshness check (MAX(MaxDate) >= 2020andMIN(MaxDate) >= 2014).
- [P2] Exception Tracebacks Discarded in
logging.fatalCatch Blocks (process.py:239)- Passing only
{e}tologging.fatal(...)discards the Python stack trace (exc_info), obscuring line numbers and call stacks during Cloud Batch failures.
- Passing only
- [P3] Operational Configs in
source_files& Google CLA Check (manifest.json:25)- Consider adding
"validation_config.json"to"source_files"inmanifest.jsonso validation configs are archived in GCS. Additionally, please resolve the failingcla/googlecheck on GitHub before merge.
- Consider adding
Positive findings
scripts/us_epa/national_emissions_inventory/process.py:139-155- Precise filename/path disambiguation and direct column assignment ✓- Finding: Good - Replaced broad
"point" in file_pathchecks with"point_" in os.path.basename(file_path) or "facility_process" in file_pathand explicit"nonpoint"branches, avoiding substring collisions withnonpointfiles, and replaced.loc[:, col] = ""with direct column assignment (df["emissions type code"] = "") to prevent Pandas 2.xfloat64dtype errors.
- Finding: Good - Replaced broad
scripts/us_epa/national_emissions_inventory/process.py:314- Eager evaluation ofThreadPoolExecutor.map✓- Finding: Good - Wrapped
executor.map(...)inlist(...)so worker thread exceptions andSystemExitfromlogging.fatalpropagate immediately to the main thread rather than silently dropping failed files.
- Finding: Good - Wrapped
scripts/us_epa/national_emissions_inventory/manifest.json:21-29- Scaled Cloud Batch compute resources and registerednode_mcf✓- Finding: Good - Increased memory to 512 GiB and CPU to 32 to eliminate OOM kills (
exit code 50002) during multi-year DataFrame concatenation, and explicitly wirednode_mcfandvalidation_config_file.
- Finding: Good - Increased memory to 512 GiB and CPU to 32 to eliminate OOM kills (
Coverage
| File | Status | Result |
|---|---|---|
scripts/us_epa/national_emissions_inventory/manifest.json |
Reviewed | One P3 finding |
scripts/us_epa/national_emissions_inventory/process.py |
Reviewed | One P1 finding, one P2 finding |
scripts/us_epa/national_emissions_inventory/validation_config.json |
Reviewed | One P2 finding |
Verification and limitations
- Checks run:
./run_tests.sh -p scripts/us_epa/national_emissions_inventory(Reproduced CI failure:ImportError: attempted relative import with no known parent packageinprocess_test.py)PYTHONPATH=. .env/bin/python3 -m unittest scripts/us_epa/national_emissions_inventory/process_test.py(Passed: 1 test in 0.575s)- Inspected GCS prod (
2025_12_31T16_03_29_040514_08_00) and dev (2026_09_06T07_23_45_163580_07_00)summary_report.csvandvalidation_output.csv.
- Checks not run: None
- Limitations: None
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ove tracebacks, and restore date freshness validation
4fc4609 to
15b2958
Compare
shourya116
left a comment
There was a problem hiding this comment.
Changed Implemented as per feedback
Code Review Summary for PR #2148 (
|
…t manifest - In process.py, remove unreachable raise and sys.exit(1) calls after logging.fatal(), avoid calling logging.fatal() inside worker threads (_national_emissions, _process_file) so exceptions propagate cleanly via list(executor.map(...)), and remove commented-out imports. - In validation_config.json, update check_deleted_records_percent description to explicitly state the 0.1% threshold and deletion rationale. - In manifest.json, remove extra trailing blank lines at EOF.
There was a problem hiding this comment.
All the latest review comments have been resolved and verified. Both the CRA review document and Postmortem report have also been updated and synced live.
Summary of Actions Taken
-
[P2]
process.pyException Handling & Cleanup:- Worker Thread Exception Propagation: Avoided calling
logging.fatal()insideThreadPoolExecutorworker threads (_national_emissionsand_process_file). Errors are logged usinglogging.exception()and re-raised so exceptions propagate cleanly tolist(executor.map(...))on the main thread. - Unreachable Code Removal: Removed unreachable
raiseandsys.exit(1)statements after terminatinglogging.fatal()calls across 6 exception blocks (intermediate file reading loop, empty DataFrame check, input file discovery, and main execution handler). - Import Cleanup: Removed commented-out imports (
# import shutil,# import tempfile).
- Worker Thread Exception Propagation: Avoided calling
-
[P2]
validation_config.jsonDescription Update:- Updated
check_deleted_records_percentdescription to explicitly state the 0.1% threshold and deletion rationale:{ "rule_id": "check_deleted_records_percent", "description": "Verifies that the percentage of deleted records does not exceed the 0.1% threshold, ensuring unintended observation drops (such as missing nonpoint sources which drop >20% of records) are caught while accommodating minor upstream EPA revisions.", "validator": "DELETED_RECORDS_PERCENT", "params": { "threshold": 0.1 } } - Corrected the typo in Postmortem Section 5.3 from
(10%)to(0.1%).
- Updated
-
[P3]
manifest.jsonFormatting:- Removed the 3 extra trailing blank lines at EOF.
-
[P2] CRA Paste & Postmortem Sync:
- Head Commit Sync: Updated both documents to reference head commit
242f22800dcf60581dc309111b5388ddb77f974e. - Unit Tests: Updated unit test counts in both documents to reflect 5 passing tests (
0.635s, OK). - Date Freshness Validation: Confirmed that
check_date_freshness(SQL_VALIDATOR) is active and passing following the clean rebase with master's DuckDB empty-differ fix. - Updated Paste Links:
- CRA Paste: https://paste.googleplex.com/6620650563829760
- Postmortem Report: https://paste.googleplex.com/5957668808818688
- Head Commit Sync: Updated both documents to reference head commit
-
Cloud Batch Verification Run (SUCCEEDED):
- Ran end-to-end verification job
epa-airpollutantemission-level1-shouryasingh-20260908-184507onus-west4usingn2-highmem-64(32 CPUs / 512 GiB RAM / 300 GB disk). - Status:
SUCCEEDED(execution duration: 7,932s / ~2.2 hrs). - All 3,041,170 rows were successfully processed (all 767,314 missing nonpoint observations recovered).
- All 5 validations passed:
check_deleted_records_percent: PASSED (0 deleted records, 0.0% vs 0.1% threshold)check_empty_import: PASSED (3,041,634 nodes, 3,041,170 rows)check_missing_refs_count: PASSED (0 missing references)check_lint_error_count: PASSED (0 lint errors)check_date_freshness: PASSED (MAX(MaxDate) >= 2020 AND MIN(MaxDate) >= 2008)
- Cloud Batch Job: https://console.cloud.google.com/batch/jobsDetail/regions/us-west4/jobs/epa-airpollutantemission-level1-shouryasingh-20260908-184507/details?project=datcom-infosys-dev
- GCS Output:
gs://datcom-import-test/scripts/us_epa/national_emissions_inventory/EPA_AirPollutantEmission_Level1/2026_09_08T11_47_48_764425_07_00 - GCS Console: https://console.cloud.google.com/storage/browser/datcom-import-test/scripts/us_epa/national_emissions_inventory/EPA_AirPollutantEmission_Level1/2026_09_08T11_47_48_764425_07_00?project=datcom-infosys-dev
- Ran end-to-end verification job
-
Sanitized PR Description
Fixes an issue where 767,314 observations across the 2017 and 2020 observation periods (specifically nonpoint/area sources) were silently dropped during the Cloud Batch import validation pipeline for EPA_AirPollutantEmission_Level1.
Root Cause
emissions type codecolumns, which Pandas loaded asfloat64. The check'point' in file_pathevaluated toTruefor..._nonpoint/...files, triggeringdf.loc[:, 'emissions type code'] = ''. In Pandas 2.x, assigning a string to afloat64series via.locraisesTypeError: Invalid value '' for dtype 'float64'._national_emissions()caught theTypeErrorand returned an empty DataFrame, silently dropping the entire nonpoint dataset (767k rows) during consolidation.Key Changes
process.py:'point_' in os.path.basename(file_path)and direct column assignment (df['emissions type code'] = '')._regularize_columns._national_emissions,_process_file) log vialogging.exceptionand re-raise so exceptions bubble cleanly tolist(executor.map(...)).raise,sys.exit(1)) after terminatinglogging.fatalcalls, and removed commented-out imports.process_test.py&__init__.py:./run_tests.sh -p scripts/us_epa/national_emissions_inventory).RegularizeColumnsTestcovering 2017/2020 nonpoint, point, and 2014 tribal schema variants (5 unit tests passing).manifest.json:n2-highmem-64) to resolve historical OOM failures.node_mcf(resolving 8 missing reference warnings).validation_config.jsontosource_filesand removed trailing blank lines.validation_config.json:check_deleted_records_percentwith a 0.1% threshold and explicit deletion rationale.check_date_freshness(SQL_VALIDATOR:MAX(MaxDate) >= 2020 AND MIN(MaxDate) >= 2008).Verification & Artifacts
SUCCEEDED, all 3,041,170 observations processed, all validations passed).gs://datcom-import-test/scripts/us_epa/national_emissions_inventory/EPA_AirPollutantEmission_Level1/2026_09_08T11_47_48_764425_07_00