Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Augment tempfile.SpooledTemporaryFile() for expected behavior #2963

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

tony-sappe
Copy link
Contributor

@tony-sappe tony-sappe commented Feb 12, 2021

Description:
This PR creates a new class to give tempfile.SpooledTemporaryFile an uplift. The current implementation in the standard library isn't satisfying the expected API. Augmenting the class methods and importing io.IOBase should get the class close to where it needs to be. Until python/cpython#3249 is merge, released, and used by this project we will need to keep this augmented class around.

Technical details:
While this issue has existed since Python 3.0, it was only recently felt in this project with the upgrade from pandas v0.45.x to pandas v1.0.2+ (pandas v1.0.0 and v1.0.1 introduced another bug which they fixed in v1.0.2, relying on API missing in the SpooledTemporaryFile implementation). See https://bugs.python.org/issue26175 and python/cpython#3249

Locally loadcfda, load_rosetta, load_city_county_state_code, and downloads were tested to ensure the changes did not break other areas. FABS and FPDS ETL scripts also use the class for obtaining delete records and should be tested. Several other ETL scripts (load_tas, load_dabs_submission_window_schedule) contain a path for loading a file which isn't used by the nightly pipeline which connects directly to broker for refreshing data.

Requirements for PR merge:

  1. Unit & integration tests updated
  2. API documentation updated (N/A)
  3. Necessary PR reviewers:
    • Backend
    • Operations
  4. Matview impact assessment completed (N/A)
  5. Frontend impact assessment completed (N/A)
  6. Data validation completed
  7. Appropriate Operations ticket(s) created (N/A)
  8. Jira Ticket (N/A)

Area for explaining above N/A when needed:

PR is only a change to class used for retrieving temporary files

@tony-sappe tony-sappe added the warmfix [pr] This goes into STAGING label Feb 12, 2021
Copy link
Contributor

@sethstoudenmier sethstoudenmier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stepped through comments on PR related to this change in cpython and everything seems to be in line with what is planned for that. LGTM.

@tony-sappe tony-sappe merged commit 9878705 into staging Feb 12, 2021
@tony-sappe tony-sappe deleted the fix/augment-tempfile-spooled-temporary-file branch February 12, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
warmfix [pr] This goes into STAGING
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants