Migrate compatible imports to use download_util_script.py - #2188
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates several import manifests to use the centralized download_util_script.py utility instead of custom download scripts, and changes a configuration setting in configs.py. The reviewer identified several critical path mismatches in the manifests where import_inputs and source_files were not updated to reflect the new output directories of the download and processing scripts. Additionally, the reviewer requested reverting the unrelated configuration change in configs.py to keep the pull request focused.
| "import_inputs": [ | ||
| { | ||
| "import_name": "BEA_USStatesQuarterlyGDP", | ||
| "curator_emails": [ | ||
| "support@datacommons.org" | ||
| ], | ||
| "provenance_url": "https://www.bea.gov/data/gdp/gdp-state", | ||
| "provenance_description": "Quarterly data on inflation-adjusted GDP at a state level in the United States from the US Bureau of Economic Analysis.", | ||
| "scripts": [ | ||
| "common_download.py", | ||
| "import_data.py", | ||
| "import_industry_data_and_gen_mcf.py" | ||
| ], | ||
| "source_files": [ | ||
| "input_data/*.csv" | ||
| ], | ||
| "import_inputs": [ | ||
| { | ||
| "cleaned_csv": "states_gdp.csv", | ||
| "template_mcf": "states_gdp.tmcf", | ||
| "node_mcf": "states_gdp_statvars.mcf" | ||
| }, | ||
| { | ||
| "cleaned_csv": "states_industry_gdp.csv", | ||
| "node_mcf": "states_gdp_industry_statvars.mcf", | ||
| "template_mcf": "states_industry_gdp.tmcf" | ||
| } | ||
| ], | ||
| "cron_schedule": "30 10 30 1,4,7,10,12 *" | ||
| "cleaned_csv": "states_gdp.csv", | ||
| "template_mcf": "states_gdp.tmcf", | ||
| "node_mcf": "states_gdp_statvars.mcf" | ||
| }, | ||
| { | ||
| "cleaned_csv": "states_industry_gdp.csv", | ||
| "node_mcf": "states_gdp_industry_statvars.mcf", | ||
| "template_mcf": "states_industry_gdp.tmcf" | ||
| } | ||
| ] | ||
| ], |
There was a problem hiding this comment.
There is a critical mismatch between the stat_var_processor.py output path (output_files/BEA_GDP_output) and the expected files in import_inputs (which still point to the old states_gdp.csv and states_industry_gdp.csv files). Since the new processor generates a single set of output files under output_files/BEA_GDP_output, import_inputs must be updated to point to these new files so the executor can find them.
"import_inputs": [
{
"cleaned_csv": "output_files/BEA_GDP_output.csv",
"template_mcf": "output_files/BEA_GDP_output.tmcf",
"node_mcf": "output_files/BEA_GDP_output_stat_vars.mcf"
}
],| "import_inputs": [ | ||
| { | ||
| "import_name": "US_CCD_Enrollment", | ||
| "curator_emails": [ | ||
| "support@datacommons.org" | ||
| ], | ||
| "provenance_url": "https://nces.ed.gov/programs/digest/d24/tables/dt24_203.65.asp", | ||
| "provenance_description": "Enrollment in public elementary and secondary schools, by level, grade, and race/ethnicity.", | ||
| "scripts": [ | ||
| "download.py", | ||
| "../../../tools/statvar_importer/stat_var_processor.py --input_data=input_files/ccd_enrollment_input.csv --pv_map=CCD_Enrollment_pvmap.csv --config_file=CCD_Enrollment_metadata.csv --output_path=output/CCD_Enrollment_output --existing_statvar_mcf=gs://unresolved_mcf/scripts/statvar/stat_vars.mcf" | ||
| ], | ||
| "source_files": [ | ||
| "input_files/ccd_enrollment_input.csv" | ||
| ], | ||
| "import_inputs": [ | ||
| { | ||
| "template_mcf": "output/CCD_Enrollment_output.tmcf", | ||
| "cleaned_csv": "output/CCD_Enrollment_output.csv" | ||
| } | ||
| ], | ||
| "cron_schedule": "0 06 * * 5" | ||
| "template_mcf": "output/CCD_Enrollment_output.tmcf", | ||
| "cleaned_csv": "output/CCD_Enrollment_output.csv" | ||
| } | ||
| ] | ||
| } No newline at end of file | ||
| ], |
There was a problem hiding this comment.
There is a critical mismatch between the stat_var_processor.py output path (output_files/enrollment_output) and the expected files in import_inputs (which still point to output/CCD_Enrollment_output.tmcf and output/CCD_Enrollment_output.csv). import_inputs must be updated to point to the new output files under output_files/enrollment_output so the executor can find them.
| "import_inputs": [ | |
| { | |
| "import_name": "US_CCD_Enrollment", | |
| "curator_emails": [ | |
| "support@datacommons.org" | |
| ], | |
| "provenance_url": "https://nces.ed.gov/programs/digest/d24/tables/dt24_203.65.asp", | |
| "provenance_description": "Enrollment in public elementary and secondary schools, by level, grade, and race/ethnicity.", | |
| "scripts": [ | |
| "download.py", | |
| "../../../tools/statvar_importer/stat_var_processor.py --input_data=input_files/ccd_enrollment_input.csv --pv_map=CCD_Enrollment_pvmap.csv --config_file=CCD_Enrollment_metadata.csv --output_path=output/CCD_Enrollment_output --existing_statvar_mcf=gs://unresolved_mcf/scripts/statvar/stat_vars.mcf" | |
| ], | |
| "source_files": [ | |
| "input_files/ccd_enrollment_input.csv" | |
| ], | |
| "import_inputs": [ | |
| { | |
| "template_mcf": "output/CCD_Enrollment_output.tmcf", | |
| "cleaned_csv": "output/CCD_Enrollment_output.csv" | |
| } | |
| ], | |
| "cron_schedule": "0 06 * * 5" | |
| "template_mcf": "output/CCD_Enrollment_output.tmcf", | |
| "cleaned_csv": "output/CCD_Enrollment_output.csv" | |
| } | |
| ] | |
| } | |
| \ No newline at end of file | |
| ], | |
| "import_inputs": [ | |
| { | |
| "template_mcf": "output_files/enrollment_output.tmcf", | |
| "cleaned_csv": "output_files/enrollment_output.csv" | |
| } | |
| ], |
| }) | ||
| # API Environment used by the import tool | ||
| use_autopush_dc_api: bool = True | ||
| use_autopush_dc_api: bool = False |
There was a problem hiding this comment.
This configuration change (use_autopush_dc_api: bool = False) is unrelated to the PR's objective of migrating compatible imports to use download_util_script.py. Unrelated changes should be kept out of this PR to keep it focused and avoid accidental environment switches.
| use_autopush_dc_api: bool = False | |
| use_autopush_dc_api: bool = True |
References
- Keep pull requests focused and confined to their intended scope. Do not mix unrelated pre-existing issues (such as missing timeout handling) or their corresponding test updates into the current PR; address them in separate PRs instead.
| "source_files": [ | ||
| "input_data/*.csv" | ||
| ], |
There was a problem hiding this comment.
The source_files path should be updated to match the actual downloaded files directory (input_files/) instead of the obsolete input_data/ directory, as the new download script downloads and extracts files to input_files.
| "source_files": [ | |
| "input_data/*.csv" | |
| ], | |
| "source_files": [ | |
| "input_files/*.csv" | |
| ], |
| "source_files": [ | ||
| "input_files/ccd_enrollment_input.csv" | ||
| ], |
There was a problem hiding this comment.
The source_files path should be updated to point to the raw downloaded file source_files/tabn203.65.xlsx instead of input_files/ccd_enrollment_input.csv to accurately reflect the download script's output folder and maintain consistency with other manifests.
| "source_files": [ | |
| "input_files/ccd_enrollment_input.csv" | |
| ], | |
| "source_files": [ | |
| "source_files/tabn203.65.xlsx" | |
| ], |
Migrates compatible data imports to use the shared util/download_util_script.py utility.