benchee icon indicating copy to clipboard operation
benchee copied to clipboard

Adding support for Table.Reader protocol

Open akoutmos opened this issue 1 year ago • 1 comments

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: https://github.com/livebook-dev/kino/issues/132) and one of the ideas that José and Jonatan had was to add Table support in Benchee. This PR adds Table as an optional dependency and also provides the implementation of the Table.Reader protocol if the dependency is pulled down by the user.

Let me know what you think and we'll take it from there! Thanks :smiley:

akoutmos avatar Aug 07 '22 21:08 akoutmos

Thanks @akoutmos!

As further motivation for adding this, note that this will also allow Explorer.DataFrame.new(suite) :)

jonatanklosko avatar Aug 08 '22 07:08 jonatanklosko

Hey @PragTob! What do you think about this integration? We already support Table.Reader in myxql, postgrex, explorer, to name a few.

Also, we are ready to release kino_benchee, but it relies on this feature. Let us know if there's anything we can do to move forward :)

jonatanklosko avatar Oct 06 '22 22:10 jonatanklosko

Would be awesome if we can merge this PR ☺️

benkeil avatar Dec 06 '22 19:12 benkeil

Hello hello folks and sorry, let me just repost (but with different bunny pic) what I posted in the other thread: https://github.com/livebook-dev/kino/issues/132#issuecomment-1362733865

:wave:

From a long slumber the @PragTob awakens. Sorry, lots of stress, bunny health, own health etc... and while bunny health is also... let's say medium right now... I'm looking at it now :) :green_heart:

I want to eventually accept the PR and ship it. I'm still medium torn on including support for essentially a third party thing that is none trivial within benchee itself (vs. a separate package to provide it) - but it seems like that's what most people are doing right now and the Table.Reader protocol seems general enough/not tied only to livebook to convince me that it's reasonable to do so. :)

IMG_20221016_093021

PragTob avatar Dec 22 '22 11:12 PragTob

I'm trying to follow up on this in #377

PragTob avatar Dec 22 '22 12:12 PragTob

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: https://github.com/bencheeorg/benchee/pull/377

reviews welcome!

PragTob avatar Dec 22 '22 14:12 PragTob