search

bencheeorg / benchee

Easy and extensible benchmarking in Elixir providing you with lots of statistics!
MIT License
1.41k stars 66 forks source link

Adding support for Table.Reader protocol #369

Closed akoutmos closed 1 year ago

akoutmos commented 2 years ago

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:

jonatanklosko commented 2 years ago

Thanks @akoutmos!

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

jonatanklosko commented 1 year ago

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 :)

benkeil commented 1 year ago

Would be awesome if we can merge this PR ☺️

PragTob commented 1 year ago

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 commented 1 year ago

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

PragTob commented 1 year ago

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!