-
-
Notifications
You must be signed in to change notification settings - Fork 6
When runs fail, save and report run failure to Slack #729
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might pass this on to @cmgosnell or @krivard for another review, but wondering why the FERC 6 archiver change outputs look like this:
"UPDATE": [
{
"ferc6-xbrl-2014.zip": -0.0
},
{
"ferc6-xbrl-2015.zip": -0.0
Is this because of the rounding? I'm assuming it's just a verrryyyy small number.
# Convert the size diff to MB for speed of assessment, and round | ||
{change["name"]: round(change["size_diff"] * (2**-20), 4)} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're converting from bytes to MB, wouldn't it be 1**-6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly binary math should run the game here, not SI units - https://www.calculatinghub.com/converters/data/bytes-to-mb-converter/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Austen is right here though; using binary math gives you mebibytes (MiB) not megabytes (MB). MB have been SI units since 1998!
Right now FERC archives always change a tiny bit. @zschira is currently working on fixing this in https://github.com/catalyst-cooperative/pudl-archiver/tree/refs/heads/ferc-stable-archives, but I chose to use it in testing to get a sense of what it would look like to visualize basically zero byte changes with MBs instead of bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the units thing for sure; other than that I'm just being nosy and this is fine
if ( | ||
error_file.exists() and error_file.stat().st_size > 0 | ||
): # Handle case where no files are found or file is empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what causes the script to sometimes receive a filename that doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be fine without this, let me see if it breaks!
|
||
error_blocks = list( | ||
itertools.chain.from_iterable( | ||
_format_errors(e) for e in errors if _format_errors(e) is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than calling _format_errors
twice, consider
_format_errors(e) for e in errors if _format_errors(e) is not None | |
filter(None, (_format_errors(e) for e in errors)) |
Does this also close #474 ? With all the github issue stuff this ended up touching? |
Overview
Closes #723, #242 and #473.
What problem does this address?
What did you change in this PR?
Note that nothing in the datapackage itself or the actual validation process is changed, this is all affecting the GHA and reporting layers.
Testing
How did you make sure this worked? How can a reviewer verify this?
See the following runs and their corresponding messages:
censuspep
but noteia860m
)To-do list