Add retry logic to datastore.get_zipfile_resource.

[jdangerx]

Overview

Closes #3657.

What problem does this address?

One more step towards figuring out that weird BadZipFile error that shows up sporadically.

Testing

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

Bunch of unit tests!

Suppose I could run the pytest-coverage, too, but it might not run into those BadZipFile errors that seem to only pop up very sporadically.

To-do list

Preview Give feedback

1. Update the release notes: reference the PR and related issues.
2. Ensure docs build, unit & integration tests, and test coverage pass locally with make pytest-coverage (otherwise the merge queue may reject your PR)
3. Review the PR yourself and call out any questions or issues you have
4. For minor ETL changes or data additions, once make pytest-coverage passes, make sure you have a fresh full PUDL DB downloaded locally, materialize new/changed assets and all their downstream assets and run relevant data validation tests using pytest and --live-dbs.
5. For significant ETL, data coverage or analysis changes, once make pytest-coverage passes, ensure the full ETL runs locally and run data validation tests using make pytest-validate (a ~10 hour run). If you can't run this locally, run the build-deploy-pudl GitHub Action (or ask someone with permissions to). Then, check the logs on the #pudl-deployments Slack channel or gs://builds.catalyst.coop.

[jdangerx]

I did this because I started running into circular import problems.

However, most of the functions in pudl.helpers don't import PUDL. Only these four do:

• create_datasette_metadata_yaml
• check_tables_have_metadata
• expand_timeseries
• convert_cols_dtypes

Should we move those to a separate file instead of making a fresh file for all the functions where we definitely won't run into circular imports?

[e-belfer]

Where is the import error coming up? Is it possible that the call to import pudl plus the multiple from calls in pudl.workspace.datastore.py is the source of the import error and could be fixed through removing the first call, rather than splitting helpers into two not clearly defined separate files?

If not, my preference would be to do what you suggest (move these four into their own file for helpers that rely on other PUDL functions), rather than start a new file with a single function and expect us to migrate over time.

[jdangerx]

Huh. The error was:

> pytest test/unit/helpers_test.py                                 
ImportError while loading conftest '/Users/dazhong-catalyst/work/pudl/test/conftest.py'.                                        
test/conftest.py:15: in <module>                                                                                                
    import pudl                                                                                                                 
src/pudl/__init__.py:5: in <module>                                                                                             
    from . import (                                                                                                             
src/pudl/analysis/__init__.py:9: in <module>                                                                                    
    from . import (                                                                                                             
src/pudl/analysis/allocate_gen_fuel.py:145: in <module>                                                                         
    from pudl.metadata.fields import apply_pudl_dtypes          
src/pudl/metadata/__init__.py:3: in <module>                    
    from . import (             
src/pudl/metadata/classes.py:63: in <module>                    
    from pudl.workspace.datastore import Datastore, ZenodoDoi   
src/pudl/workspace/__init__.py:11: in <module>                  
    from . import datastore, resource_cache, setup              
src/pudl/workspace/datastore.py:26: in <module>                 
    from pudl.helpers import retry                              
src/pudl/helpers.py:34: in <module>                             
    from pudl.metadata.classes import DatasetteMetadata         
E   ImportError: cannot import name 'DatasetteMetadata' from partially initialized module 'pudl.metadata.classes' (most likely d
ue to a circular import) (/Users/dazhong-catalyst/work/pudl/src/pudl/metadata/classes.py)

And for some reason this diff fixed it - currently reading through the import system docs to try to figure out what the heck was going on.

diff --git a/src/pudl/helpers.py b/src/pudl/helpers.py
index 87d9fc41a..f97a27c10 100644
--- a/src/pudl/helpers.py
+++ b/src/pudl/helpers.py
@@ -30,7 +31,7 @@ from dagster import AssetKey, AssetsDefinition, AssetSelection, SourceAsset
 from pandas._libs.missing import NAType
 
 import pudl.logging_helpers
-from pudl.metadata.classes import DatasetteMetadata
+import pudl.metadata.classes
 from pudl.metadata.fields import apply_pudl_dtypes, get_pudl_dtypes
 from pudl.workspace.setup import PudlPaths
 
@@ -2029,7 +2030,7 @@ def create_datasette_metadata_yaml() -> str:
         derived from their datapackage.json as YAML.
     """
     pudl_output = PudlPaths().pudl_output
-    metadata = DatasetteMetadata.from_data_source_ids(pudl_output)
+    metadata = pudl.metadata.classes.DatasetteMetadata.from_data_source_ids(pudl_output)
     return metadata.to_yaml()

[jdangerx]

I guess my crude mental model is that:

At this specific import time, DatasetteMetadata hasn't been defined in pudl.metadata.classes yet, since pudl.metadata.classes imports pudl.workspace.datastore which imports pudl.helpers. But we can import the pudl.metadata.classes module directly since the import statement doesn't actually check that DatasetteMetadata is defined in the module.

The reason that we can import attributes from pudl.metadata.fields and pudl.workspace.setup is because they don't circularly depend on pudl.helpers.

I think long term we should probably aim to have "truly independent helpers" and "helpers that actually need the rest of PUDL" as separate files, to avoid but (a) I'm not sure what to name the two files and (b) this works - so I'm happy to punt on that until the next circular import happens >_<

[e-belfer]

Ah, ok. Glad you managed to figure it out, this seems like the much preferable solution.

[e-belfer]

Some questions and a request re: the reorganization of the pudl.helpers.py file. Otherwise, all looks good on my end.

[e-belfer]

Where is the import error coming up? Is it possible that the call to import pudl plus the multiple from calls in pudl.workspace.datastore.py is the source of the import error and could be fixed through removing the first call, rather than splitting helpers into two not clearly defined separate files?

If not, my preference would be to do what you suggest (move these four into their own file for helpers that rely on other PUDL functions), rather than start a new file with a single function and expect us to migrate over time.

[e-belfer]

Will MagicMock add a 7 second delay to our unit tests here? Doesn't seem to be the case when I run unit tests, but maybe I'm missing something. If so, we could reduce the number of RuntimeErrror calls by one to gain a bit of speed back here if this becomes a noticeable lag.

[jdangerx]

Good question! We're bypassing the delay by replacing time.sleep with just an empty MagicMock() - see that with mocker.patch(...) line above.

[e-belfer]

Much preferable solution and a helpful fix that should help make the nightly builds a hair more robust. All works as expected locally.