add exception handling to ckan upload script #3227
Conversation
charlie-costanzo
force-pushed
the
handle-open-data-exceptions
branch
from
c5f5e70 to
3654e25
Compare
warehouse/scripts/publish.py
Outdated
| file=fp, | ||
| resource_id=resource.id, | ||
| ) | ||
| except requests.exceptions.RequestException as e: |
There was a problem hiding this comment.
I believe that this should be requests.exceptions.HTTPError, which is the specific class of error produced when an upload times out.
There was a problem hiding this comment.
yes this makes sense! I almost did that but defaulted to a more general exception for some unknown reason
warehouse/scripts/publish.py
Outdated
| resource_id=resource.id, | ||
| ) | ||
| except requests.exceptions.RequestException as e: | ||
| print(e) |
There was a problem hiding this comment.
We're already using typer.echo statements instead of the built-in print tool elsewhere in this script, so I'd make this into something like:
typer.echo(f"Failed to upload to {fpath} due to error: {e}")
We use styled logs in this script (which don't actually display with styling in the Airflow UI, so jury's out on whether it's worth continuing to do so). But if you want to align this with most of the other logging in the same script, you'd use the slightly more complex typer.secho command like this one:
typer.secho(f"Failed to upload to {fpath} due to error: {e}", fg=typer.colors.RED)
There was a problem hiding this comment.
ah yes good point!
warehouse/scripts/publish.py
Outdated
| ) | ||
| except requests.exceptions.RequestException as e: | ||
| print(e) | ||
| continue |
There was a problem hiding this comment.
I don't think you need this continue statement. The end of the loop iteration, a few lines further down, manually deletes the df dataframe after the if-else logic but before the loop iteration completes, which was probably done for resource constraint reasons. We likely want to still be able to reach that line after an error is logged, after which point the loop will still move on to its next iteration as normal.
There was a problem hiding this comment.
makes a lot of sense, I went back on forth on this for a long time too hah
This review was very helpful Soren, thank you! I just pushed the changes now. Let me know if anything else looks off |
charlie-costanzo
requested review from
evansiroky and
tiffanychu90
as code owners
Description
As described in #3182, when trying to upload data to the open data portal (CKAN), if one file fails (specifically, stop times tends to fail because of its size), then the subsequent files (in alphabetical order) are not even attempted.
This PR adds exception handling to
warehouse/scripts/publish.pyso that if one file upload fails the others are attemped.Resolves #3182
Type of change
How has this been tested?
Will have to test in prod as this is an error generated by Caltrans servers
Post-merge follow-ups