Skip to content

added golden USCensusPEP_PopulationEstimatebyRace#2048

Open
niveditasing wants to merge 18 commits into
datacommonsorg:masterfrom
niveditasing:goldes_pop_estimate
Open

added golden USCensusPEP_PopulationEstimatebyRace#2048
niveditasing wants to merge 18 commits into
datacommonsorg:masterfrom
niveditasing:goldes_pop_estimate

Conversation

@niveditasing

@niveditasing niveditasing commented May 29, 2026

Copy link
Copy Markdown
Contributor

This PR includes:

  1. Robust Download Retries & Timeout Handling ( urlllib3 & Session integration )
    To prevent intermittent run-time connection failures or GCS/Census endpoint timeouts:
    • Added Urllib3 adaptors: Integrated standard Retry and HTTPAdapter at the module level.
    • Persistent Session: Replaced individual requests.get calls with a shared requests.Session() object configured to automatically retry
      failed requests (up to 5 retries, handling status codes 429, 500, 502, 503, 504 with exponential backoff).
    • Passed Session to downstreams: Forwarded the configured session object to downstream loaders like _resolve_pe_11 so they benefit from
      the same robust timeout and retry safety.

  1. Safe Coordinate-Based DataFrame Access (iloc Refactoring)
    To eliminate risk of unexpected behavior, performance side-effects, and standard Pandas warnings (such as SettingWithCopyWarning):
  • Replaced chained indexing: Converted chained lookups like df.iloc[j]["Area"] = ... to standard, safe coordinate-based syntax:
    df.iloc[j, 0] = ....
  • Added explicit casting: Wrapped coordinates inside explicit str(...) casting to guarantee concatenation safety regardless of the
    column's underlying data type.

  1. Correction of Length Evaluation Bug
    Fixed a critical logical/syntactical bug in checking filename lengths:
  • Before: len(file_to_dowload['file_name'] > 5) (This incorrectly evaluated the length of a boolean comparison expression, causing
    logical failures).
  • After: len(file_to_dowload['file_name']) > 5 (Correctly measures the length of the string value).

  1. Deterministic Input File Scanning (sorted())
  • Wrapped os.listdir() in sorted(...) when populating input files list.
  • This ensures that input files are read in a deterministic order on any operating system, preventing unpredictable schema mapping,
    mismatched generation, or tests behaving differently on different filesystems.

  1. Pandas Deprecation Upgrades
  • Modernized deprecated parameters under pd.read_table(...).
  • Replaced the deprecated delim_whitespace=True with explicit standard separator regex: sep=r'\s+', ensuring perfect compatibility with
    recent and future Pandas versions.

  1. Added goldens with 0.1% increase in threshold

PR checklist : https://docs.google.com/spreadsheets/d/1BzweR9Sj58j0H2_BweGTmfE4Z1lrjPZL8u1FS0kzCeg/edit?pli=1&gid=0#gid=0

@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 adds several golden data CSV files and introduces a validation configuration file (validation_config.json) along with its reference in the manifest to enable automated validation checks for the US Census PEP population estimate by race import. Feedback on the changes highlights a critical filename mismatch in the validation configuration that would cause a file-not-found error, as well as recommendations to ensure all validation rules have unique rule_id values for clearer reporting.

Comment thread scripts/us_census/pep/population_estimate_by_race/validation_config.json Outdated
Comment thread scripts/us_census/pep/population_estimate_by_race/validation_config.json Outdated
Comment thread scripts/us_census/pep/population_estimate_by_race/validation_config.json Outdated
Comment thread scripts/us_census/pep/population_estimate_by_race/validation_config.json Outdated
@niveditasing niveditasing changed the title added golden added golden UsCensuspep_population_estimate_by_race Jul 7, 2026
@niveditasing niveditasing changed the title added golden UsCensuspep_population_estimate_by_race added golden USCensusPEP_PopulationEstimatebyRace Jul 7, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

The refactoring of iloc indexing and the fix for the length evaluation bug are important improvements for data integrity and performance. Using sorted(os.listdir()) is a good practice for ensuring deterministic behavior across different filesystems. The transition to requests.Session with urllib3 retries will significantly improve the robustness of the data ingestion process. One suggestion: verify that the validation_config.json thresholds are sufficiently strict to catch regressions in the golden files, as the 0.1% threshold should be evaluated against the expected variance of the Census data.

@niveditasing

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist review

@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 and golden summary reports for US Census PEP population estimates by race. It updates preprocess.py to use a robust requests.Session with retry logic, fixes a bug in a string length check, sorts input files, and updates pandas operations to avoid deprecation warnings. Feedback is provided to extend the retry adapter configuration to cover both HTTP and HTTPS protocols.

Comment on lines +1306 to +1310
retries = Retry(total=5,
backoff_factor=1,
status_forcelist=[429, 500, 502, 503, 504],
raise_on_status=False)
session.mount('https://', HTTPAdapter(max_retries=retries))

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

Currently, the retry adapter is only mounted to https:// URLs. If any input URLs or future URLs use http://, they will not benefit from the configured retry mechanism. It is safer and more robust to mount the adapter to both http:// and https:// protocols.

Suggested change
retries = Retry(total=5,
backoff_factor=1,
status_forcelist=[429, 500, 502, 503, 504],
raise_on_status=False)
session.mount('https://', HTTPAdapter(max_retries=retries))
retries = Retry(total=5,
backoff_factor=1,
status_forcelist=[429, 500, 502, 503, 504],
raise_on_status=False)
adapter = HTTPAdapter(max_retries=retries)
session.mount('http://', adapter)
session.mount('https://', adapter)

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.

2 participants