Skip to content

Added goldens for USCensuspep imports#2091

Open
niveditasing wants to merge 7 commits into
datacommonsorg:masterfrom
niveditasing:golden_us_imports
Open

Added goldens for USCensuspep imports#2091
niveditasing wants to merge 7 commits into
datacommonsorg:masterfrom
niveditasing:golden_us_imports

Conversation

@niveditasing

Copy link
Copy Markdown
Contributor

No description provided.

@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 validation configurations, golden summary reports, and generated outputs for the US Census PEP monthly population estimate and sex datasets. Additionally, it updates download routines in process.py to include timeouts, retries with exponential backoff, and sorted file processing. Feedback on the changes highlights an incorrect golden file path in the us_pep_sex validation configuration and a potential resource leak in process.py where temporary files are not cleaned up if an exception occurs during file copy or move operations.

"rule_id": "check_goldens_summary_report",
"validator": "GOLDENS_CHECK",
"params": {
"golden_files": "../../../../golden_data/golden_summary_report.csv"

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.

high

The path ../../../../golden_data/golden_summary_report.csv points to a golden file that does not exist for this import, or it incorrectly references a shared path. Since us_pep_sex is a different dataset from monthly_population_estimate, it should have its own golden summary report (e.g., generated from scripts/us_census/pep/us_pep_sex/output/new_rec_gen_sa/summary_report.csv and placed in a golden_data directory under us_pep_sex). Note that the path in the configuration should still be specified as a relative path from the execution context (using ../ relative pathing) rather than relative to the configuration file itself.

References
  1. Golden file paths in validation configurations should be specified as relative paths from the execution context (using relative pathing like ../) rather than relative to the configuration file itself.

Comment on lines +1331 to +1346
with tempfile.NamedTemporaryFile(delete=False) as tmp_file:
for chunk in response.iter_content(chunk_size=8192):
if chunk:
tmp_file.write(chunk)
tmp_file_path = tmp_file.name

# Copy to local destination
shutil.copy(
tmp_file_path,
os.path.join(_INPUT_FILE_PATH, file_name_to_save))

# Move to gcs destination (optimized from shutil.copy + os.remove)
shutil.move(
tmp_file_path,
os.path.join(_GCS_FOLDER_PERSISTENT_PATH,
file_name_to_save))

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.

medium

If an exception occurs during shutil.copy or shutil.move, the temporary file created by tempfile.NamedTemporaryFile(delete=False) will not be cleaned up, leading to orphaned files in the temp directory. Wrap the file operations in a try...finally block to ensure the temporary file is always deleted.

                        tmp_file_path = None
                        try:
                            with tempfile.NamedTemporaryFile(delete=False) as tmp_file:
                                for chunk in response.iter_content(chunk_size=8192):
                                    if chunk:
                                        tmp_file.write(chunk)
                                tmp_file_path = tmp_file.name

                            # Copy to local destination
                            shutil.copy(
                                tmp_file_path,
                                os.path.join(_INPUT_FILE_PATH, file_name_to_save))

                            # Move to gcs destination (optimized from shutil.copy + os.remove)
                            shutil.move(
                                tmp_file_path,
                                os.path.join(_GCS_FOLDER_PERSISTENT_PATH,
                                             file_name_to_save))
                        finally:
                            if tmp_file_path and os.path.exists(tmp_file_path):
                                try:
                                    os.remove(tmp_file_path)
                                except Exception:
                                    pass

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.

1 participant