Prettify duration message at end of execution #5364
Conversation
There was a problem hiding this comment.
Nice, thanks for picking this up @jared-rimmer! The change proposed here looks good to me. I'll leave final code review to folks on the Core engineering team.
core/dbt/task/run.py
Outdated
| @@ -398,11 +398,15 @@ def print_results_line(self, results, execution_time): | |||
| execution = "" | |||
|
|
|||
| if execution_time is not None: | |||
| execution = " in {execution_time:0.2f}s".format(execution_time=execution_time) | |||
| execution = utils.humanise_execution_time(execution_time=execution_time) | |||
There was a problem hiding this comment.
Question for Core engineers: Does it make sense to have this prettification/humanisation logic live in the run task / event call-site? Or should it be within the class definition for HookFinished?
(Also, my EU-resident computer is very happy with this spelling of humanise, but we may wish to Americanize it for consistency)
There was a problem hiding this comment.
I've Americanized the spelling to Humanize as you suggested.
I'll wait on the response from the core engineers about the other point and will be happy to make the change if needed.
This is my first contribution so it would also be good to understand where some tests for the new functions should live.
There was a problem hiding this comment.
I think for now it's fine to leave the prettification it where it is. I suspect that HookFinished may be one of the events that we'll be touching with the upcoming event work, so rather than guess what info people will want in it, let's just defer it :)
I think a test in test/unit/test_utils.py would be fine. You could create a basic test that does 'run_dbt_and_capture' (grep tests/functional for examples). I'm not sure exactly where to put it, but someplace neutral like tests/functional/basic/test_jaffle_shop.py should be fine.
There was a problem hiding this comment.
Thanks for the guidance @gshank. I've gone ahead and done those things. Let me know what you think.
core/dbt/task/run.py
Outdated
| fire_event(HookFinished(stat_line=stat_line, execution=execution)) | ||
| fire_event( | ||
| HookFinished( | ||
| stat_line=stat_line, execution=execution, execution_time=execution_time |
There was a problem hiding this comment.
👍 Thanks for adding the real float in here as well!
jtcohen6
added
the
ready_for_review
|
Oh, it looks like you need to pip install pre-commit and run 'pre-commit run --all-files' to get the black formatting. |
|
And test/unit/test_events.py needs to be updated. |
There was a problem hiding this comment.
@jared-rimmer Thanks for the quick turn over and added the tests!!
|
@jared-rimmer looks like there might be some issue with the tests |
|
I believe that the functional test is failing because the order of the tests isn't guaranteed. You can either add a "seed" command to the new test, or combine it with the existing one (which already has run seed). Otherwise looks good! |
|
@jared-rimmer I went ahead and added the |
|
@jared-rimmer Merged! Thanks for taking the time to implement this!! |
|
Not at all @ChenyuLInx thanks for all your help! |
jared-rimmer
deleted the
feature/ct-649-prettify-duration-at-the-end-of-execution
branch
resolves #5253
Description
humanise_execution_timethat returns a string in the format:X hours Y minutes and Z secondsexecution, as a string, with hours/minutes/seconds (1 hours 6 minutes and 51 seconds)execution_time(float, representing seconds) as a dataclass attribute that gets passed into HookFinished, and included in its structured (JSON) output, even if it isn't templated into the human-freindly messageFinished running 1 table model, 1 view model in 0 hours 0 minutes and 0.36 seconds (0.36s).Checklist
changie newto create a changelog entry