-
Notifications
You must be signed in to change notification settings - Fork 423
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
Show the evaluation time next to the cell indicator #366
Conversation
Show the evaluation time next to the cell indicator
Thank you @shavit! I have added some comments, but perhaps it is best to wait for @jonatanklosko's feedback before working on them. |
Oh, It would also be nice to show the timer ticking when evaluation starts. This would need to be client-side though. We can increment every 100ms? |
I think post-evaluation time is helpful in cases, but not necessarily worth showing by default on all the cells (as they are "immediate" most of the time), so perhaps we could show it as a tooltip when hovering over the status? For progress, we could replace "Evaluating" with the ticking time with in like 0.1s increments, but that should be handled on the client and can definitely be done separately. Thanks @shavit for working on this! |
Start the evaluation time inside the evaluator, display the time in seconds or milliseconds. The output is not a string but a map
@jonatanklosko let me know to which response you refer, the evaluator sends a string |
lib/livebook/evaluator.ex
Outdated
response: state.formatter.format_response(response), | ||
evaluation_time_ms: result_context.evaluation_time_ms | ||
} | ||
|
||
send(send_to, {:evaluation_response, ref, output}) |
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.
We should leave output
as is and just add a time entry to the tuple. To make this easier to extend in the future, the new element could be a map, something like this:
metadata = %{evaluation_time_ms: evaluation_time_ms}
send(send_to, {:evaluation_response, ref, output, metadata})
@josevalim please double-check if this less strict policy makes sense
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.
So it will be good to add metadata to all messages
+ * `{:evaluation_output, ref, output, metadata}` - output captured during evaluation
+ * `{:evaluation_response, ref, output, metadata}` - final result of the evaluation
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.
Cannot think of a use case for output metadata, but sounds like a good call to me yeah. Also please add a note to the second item like "recognised metadata entries are: evaluation_time_ms
".
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.
Won't we have to change Kino if we add metadata to the output? 🤔
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.
Nah, Kino sends {:livebook_put_output, output}
to the group leader, but the actual message is composed by IOProxy
:
• Add metadata argument to the evaluation_response message • Use a tooltip to display the evaluation time
I believe a number of test would fail after changing the response format, looking for |
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.
Great, one comment and we can ship it!
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.
Thanks @shavit!
Livebook_Demo_280.mov
This change adds the execution time next to the cell indicator.
It is also possible to add a progress bar or display the execution time if is greater than N. The cell info has the previous execution time.
#280