Adding support for Table.Reader protocol - #369
Conversation
|
Thanks @akoutmos! As further motivation for adding this, note that this will also allow |
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
|
Hey @PragTob! What do you think about this integration? We already support Also, we are ready to release |
|
Would be awesome if we can merge this PR |
|
Hello hello folks and sorry, let me just repost (but with different bunny pic) what I posted in the other thread: livebook-dev/kino#132 (comment)
|
PragTob
left a comment
There was a problem hiding this comment.
Looking good, left some minor comments. I'll probably take it on myself to fix those and change some minor stuff around since it took me so long to get here 😅
Thanks for your work and patience 💚
| end | ||
|
|
||
| if Code.ensure_loaded?(Table.Reader) do | ||
| defimpl Table.Reader, for: Benchee.Suite do |
There was a problem hiding this comment.
I'd be inclined to move this to its own file just for some separation as it is somehow meaty :)
There was a problem hiding this comment.
haha I myself implemented the deep_merge resolver here so... maybe not so that it starts breaking the pattern 😅
| alias Benchee.Scenario | ||
|
|
||
| def init(suite_results) do | ||
| columns = get_columns_from_suite(suite_results) |
There was a problem hiding this comment.
In the code base we usually refer to this as suite or well just the suite, so I think that's fine 🤔
|
|
||
| memory_fields = Enum.map(fields_per_type, fn field -> "memory_#{field}" end) | ||
| reductions_fields = Enum.map(fields_per_type, fn field -> "reductions_#{field}" end) | ||
| run_time_fields = Enum.map(fields_per_type, fn field -> "run_time_#{field}" end) |
There was a problem hiding this comment.
Theoretically we'd only want to collect these if theses measurements have been run, right? We could use Scenario.data_processed? to check although I see that is missing the reductions implementation.
Or are these auto hidden if empty? I really don't know.
|
I'm trying to follow up on this in #377 |
|
I'm gonna close this one (for now) as I've followed up and extended on it with what I feel like are good/meaningful extensions over here: #377 reviews welcome! |
First of all, hello and thanks for all the hard work you have put into Benchee!
I took on implementing Benchee support in Livebook (see the GitHub issue here: livebook-dev/kino#132) and one of the ideas that José and Jonatan had was to add Table support in Benchee. This PR adds
Tableas an optional dependency and also provides the implementation of theTable.Readerprotocol if the dependency is pulled down by the user.Let me know what you think and we'll take it from there!
Thanks 😃