testing - #2122
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the US Census PEP sex/race processing scripts by replacing the deprecated delim_whitespace=True parameter with sep=r'\s+' in pandas read operations, adding validation configurations, and introducing safety checks for missing files and download lists. Feedback on these changes suggests raising a FileNotFoundError instead of logging a warning when intermediate files are missing in postprocess.py to prevent silent data loss. Additionally, it is recommended to use .loc for column assignment in county_1990_2000.py to avoid potential SettingWithCopyWarning issues.
| if not os.path.exists(file_path): | ||
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | ||
| continue |
There was a problem hiding this comment.
Skipping missing intermediate files with a warning can lead to silent data loss or incomplete final datasets. In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code.
| if not os.path.exists(file_path): | |
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | |
| continue | |
| if not os.path.exists(file_path): | |
| raise FileNotFoundError(f'Required intermediate file {i} not found.') |
References
- In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code, rather than returning False and allowing the script to exit with success.
| if not os.path.exists(file_path): | ||
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | ||
| continue |
There was a problem hiding this comment.
Skipping missing intermediate files with a warning can lead to silent data loss or incomplete final datasets. In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code.
| if not os.path.exists(file_path): | |
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | |
| continue | |
| if not os.path.exists(file_path): | |
| raise FileNotFoundError(f'Required intermediate file {i} not found.') |
References
- In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code, rather than returning False and allowing the script to exit with success.
| if not os.path.exists(file_path): | ||
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | ||
| continue |
There was a problem hiding this comment.
Skipping missing intermediate files with a warning can lead to silent data loss or incomplete final datasets. In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code.
| if not os.path.exists(file_path): | |
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | |
| continue | |
| if not os.path.exists(file_path): | |
| raise FileNotFoundError(f'Required intermediate file {i} not found.') |
References
- In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code, rather than returning False and allowing the script to exit with success.
| if not os.path.exists(file_path): | ||
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | ||
| continue |
There was a problem hiding this comment.
Skipping missing intermediate files with a warning can lead to silent data loss or incomplete final datasets. In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code.
| if not os.path.exists(file_path): | |
| logging.warning(f"File {i} not found in intermediate outputs, skipping.") | |
| continue | |
| if not os.path.exists(file_path): | |
| raise FileNotFoundError(f'Required intermediate file {i} not found.') |
References
- In pipeline scripts, critical failure points should raise exceptions to ensure the process exits with a non-zero status code, rather than returning False and allowing the script to exit with success.
| # providing geoId to the dataframe | ||
| # and making the geoId of 5 digit as county | ||
| df.loc[:, 'geo_ID'] = [f'{x:05}' for x in df.loc[:, 'geo_ID']] | ||
| df['geo_ID'] = df['geo_ID'].astype(int).astype(str).str.zfill(5) |
There was a problem hiding this comment.
Assigning to a column of a sliced DataFrame directly can trigger a SettingWithCopyWarning in pandas. It is safer to use .loc to perform the assignment.
| df['geo_ID'] = df['geo_ID'].astype(int).astype(str).str.zfill(5) | |
| df.loc[:, 'geo_ID'] = df['geo_ID'].astype(int).astype(str).str.zfill(5) |
This PR includes
Code fix:
Added golden checks
Testing results: https://storage.mtls.cloud.google.com/datcom-import-test/scripts/us_census/pep/us_pep_sexrace/USCensusPEP_By_Sex_Race/2026_07_31T01_39_54_500027_07_00/input0/validation/validation_output.csv