Use context managers for opening zipfiles

[bendnorman]

Overview

Hopefully, using context managers to close connections to zip files will resolve our sporadic BadZipFile error #3364.

Testing

How did you make sure this worked? How can a reviewer verify this?

To-do list

Beta Give feedback

1. Make sure full ETL runs & make pytest-integration-full passes locally
2. For major data coverage & analysis changes, run data validation tests
3. If updating analyses or data processing functions: make sure to update or write data validation tests
4. Update the release notes: reference the PR and related issues.
5. Review the PR yourself and call out any questions or issues you have

[jdangerx]

Sweet, glad to see it's so straightforward :) There are a few other places where we use get_zipfile_resource outside of a context manager (dbf.py, eia176.py, epacems.py, etl_test.py, glue_assets.py) - any particular reason not to change those as well?

[bendnorman]

Ugh I goofed this rebase. @jdangerx any idea how to resolve it? I only meant to push the latest commit.

[bendnorman]

I went back and added context managers to where we use get_zipfile_resource except for pudl.extract.dbf.FercDbfReader.

pudl/src/pudl/extract/dbf.py

Lines 340 to 350 in 3630dca

	@lru_cache # noqa: B019
	def get_archive(self: Self, year: int, **filters) -> FercDbfArchive:
	"""Returns single dbf archive matching given filters."""
	nfilters = self._normalize(filters)
	return FercDbfArchive(
	self.datastore.get_zipfile_resource(self.dataset, year=year, **nfilters),
	dbc_path=self._dbc_path[year],
	partition=filters,
	table_file_map=self._table_file_map,
	field_parser=self.field_parser,
	)

It doesn't seem right to pass an open ZipFile object into an objection initialization because it's not super clear when it should be closed. I think it'd be better to pass a zipfile path or the arguments of get_zipfile_resource into this class so it's methods can open and close the file as needed. Given we haven't seen this bit of code produce the BadZipError I think it's fine to keep it as is for now. I can create an issue to redesign the classes that accept open zipfiles.

I'm also more optimistic adding context managers will resolve the BadZipFile("File is not a zip file") error. The zipfile docs say:

You must call close() before exiting your program or essential records will not be written.

This makes me think the "essential records" weren't written and the PHMSA and FERC 714 extractors tried to reopen these corrupted zipfiles. However, I don't know what "essential records" would need to be written when you are reading the file 🤷

[jdangerx]

Leaving the FercDbfReader as is seems fine! Maybe a comment in-line to leave a breadcrumb trail would be helpful for future Ben :)

Your comment about essential records led me down this rabbit hole: https://github.com/python/cpython/blob/3.12/Lib/zipfile/__init__.py#L1211

[jdangerx]

🤐