Skip to content
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

Automatically render benchee results #132

Closed
jonatanklosko opened this issue Apr 15, 2022 · 27 comments
Closed

Automatically render benchee results #132

jonatanklosko opened this issue Apr 15, 2022 · 27 comments

Comments

@jonatanklosko
Copy link
Member

Benchee.run returns a %Benchee.Suite{} struct with a ton of numbers. We can implement the Kino.Render protocol to show some plots. As for what to show, there's benchee_html to take inspiration from. Maybe alternatively we could embed the result of benchee_html.

@josevalim
Copy link
Contributor

We may also be able to implement table for Benchee.Suite. Maybe they can even accept it upstream.

@akoutmos
Copy link
Contributor

akoutmos commented Aug 4, 2022

I am starting to play around with plotting benchee results with Livebook. I'll post some screen shots here once I have the visuals up and running. @josevalim can you expand on your comment? Perhaps a silly question...but what's table 🙈?

@jonatanklosko
Copy link
Member Author

jonatanklosko commented Aug 4, 2022

@akoutmos it's a package we published recently, see the docs. When a struct implements the Table.Reader protocol it can be easily read as rows or columns. The protocol is already implemented for lists, Postgrex.Result, MyXQL.Result, Explorer.DataFrame, etc. Then anything that implements that protocol can be passed to Kino.DataTable, VegaLite or used with the Chart smart cell :)

@akoutmos
Copy link
Contributor

akoutmos commented Aug 5, 2022

Ahhh. It's all clear to me now :D. I added the defimpl Table.Reader, for: Benchee.Suite to benchee (will open up a PR there in a couple days once I clean up the implementation). But I was able to get some results to plot and to show up via DataTable:

image

The boxplot doesn't look the best because of the outliers, but it does plot. With the Table.Reader protocol in place, I'll play around with the Kino.Render protocol next and see if I can easily render a couple plots similar to benchee_html. Thanks for the guidance @jonatanklosko!

@akoutmos
Copy link
Contributor

akoutmos commented Aug 5, 2022

I'm almost done with the defimpl Kino.Render, for: Benchee.Suite implementation as well. Below is an example of the rendered benchmark suite. In order to render the charts though, Kino will depend on VegaLite and KinoVegaLite. Is that OK?

image

@jonatanklosko
Copy link
Member Author

@akoutmos awesome!! I think we will end up with kino_benchee, because depending on VegaLite is too much, but we can discuss that later :) Also, maybe we could use the new tabbed view to save space? For example "Run time"/"Memory usage" tabs.

@jonatanklosko
Copy link
Member Author

What if we flip the bar chart? This should make the job names horizontal and easier to read.

@akoutmos
Copy link
Contributor

akoutmos commented Aug 5, 2022

That was my feeling as well pulling it in. Can you create the kino_benchee repo and I can make a PR directly to that?

I'll update the visuals with your UX suggestions :).

@jonatanklosko
Copy link
Member Author

@akoutmos kino_benchee 🐈

@akoutmos
Copy link
Contributor

akoutmos commented Aug 5, 2022

Thanks!

@akoutmos
Copy link
Contributor

akoutmos commented Aug 6, 2022

I think I am more or less done with the implementation of everything. I'll open up the PR to benchee later tonight for table support and link it here for visibility. After that gets merged in and a new release is cut, I can require that version in kino_benchee. Also, can you cut a new release of Kino since I need Kino.Layout for laying out the results? I'll also require that version in kino_benchee once the release is cut. Thanks a bunch!

End result:

image

@josevalim
Copy link
Contributor

We cannot ship and a new kino version Because it requires a new livebook-version, therefore we have To wait until we release 0.7 :(

I expect that to happen within a month!

@jonatanklosko
Copy link
Member Author

In the meantime, pointing to kino github in kino_benchee is fine!

@akoutmos
Copy link
Contributor

akoutmos commented Aug 8, 2022

No worries at all @josevalim! I opened up PRs for everything that I have for this feature and will address things as they come up :).

Sounds good @jonatanklosko. I did that and opened the draft PR in kino_benchee with everything needed to render the benchmark results.

Thanks!

@benkeil
Copy link

benkeil commented Dec 6, 2022

I used the following config

Mix.install([
  {:kino, git: "https://github.com/livebook-dev/kino.git", override: true},
  {:benchee, git: "https://github.com/bencheeorg/benchee.git", override: true},
  {:kino_benchee, git: "https://github.com/livebook-dev/kino_benchee.git", override: true}
])

suite = Benchee.run(
  %{
    "solve1" => fn -> solve1.(input, 4) end,
    "solve2" => fn -> solve2.(input, 4) end,
    "solve3" => fn -> solve3.(input, 4) end
  },
  time: 1,
  memory_time: 1,
)

Kino.Render.to_livebook(suite)

and get the error ** (Protocol.UndefinedError) protocol Table.Reader not implemented for %Benchee.Suite{...

@akoutmos
Copy link
Contributor

akoutmos commented Dec 6, 2022

Unfortunately what you posted won't work until this PR is merged in: bencheeorg/benchee#369

@jonatanklosko Should we instead provide the Table protocol implementation in this library (https://github.com/livebook-dev/kino_benchee) so we can better control the maintenance of it? If so I can move the protocol implementation and open up a PR to that lib instead. Thoughts?

@josevalim
Copy link
Contributor

@akoutmos it will all depend if they plan to eventually merge that PR or not. :)

@benkeil
Copy link

benkeil commented Dec 6, 2022

To make it easier for other users it might be helpful to mark the repo as "in development" or "wait for PR" in the README.

@akoutmos
Copy link
Contributor

akoutmos commented Dec 6, 2022

@akoutmos it will all depend if they plan to eventually merge that PR or not. :)

Sounds good to me 👍

@PragTob
Copy link

PragTob commented Dec 22, 2022

👋

@josevalim @akoutmos

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

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_20210925_095053_Bokeh

@josevalim
Copy link
Contributor

HI @PragTob! Thanks for the updates and hopefully everyone will recover soon :)

The goal of Table.Reader was exactly to decouple Livebook concerns from Benchee. Right now it is only used for Livebook but you can imagine we will add Table.display or Table.chart in the future and make it easy to see a table results and charts in the terminal.

That said, you don't have to merge Table.Reader support into Benchee at all. The beauty of protocols is that they can be implemented anywhere and we could implement it in Kino.Benchee too. We basically wanted to touch point with you so we don't implement a protocol you would implement anyway. But given Benchee's architecture where even basic plugins such as HTML support are separate projects, I would even say that you probably shouldn't merge it. We just need your position. :)

@PragTob
Copy link

PragTob commented Dec 22, 2022

@josevalim muito obrigado :)

Yeah that's what has been on my mind - benchee tries to be as slim of a core as possible (I event want to push this further) and do everything else in separate packages.

Given the more general use case for Table.Reader and at least the initial implementation being less than 100loc I think even for me it'd be a bit of a stretch to move it to a separate package. I'm currently polishing up some stuff on the PR (hoping I don't break stuff 😁 ) and getting it read to merge into Benchee itself!

@PragTob
Copy link

PragTob commented Dec 22, 2022

And of course supported elixir versions differ which hints at making a package or fiddle with CI 😅 But... I'll leave this thread alone with this now and focus it on the MR bencheeorg/benchee#377

@josevalim
Copy link
Contributor

at least the initial implementation being less than 100loc I think even for me it'd be a bit of a stretch to move it to a separate package.

To clarify: we will need the separate package anyway.

@PragTob
Copy link

PragTob commented Dec 22, 2022

@josevalim you mean like kino_benchee or even another one? The first one (kino_benchee) I understood, but I think it's reasonable for benchee itself to provide the Table.Reader support :)

@josevalim
Copy link
Contributor

Only Kino.Benchee, yeah.

@josevalim
Copy link
Contributor

Btw, another benefit of Table.Reader is that Explorer supports Table.Reader inputs. So you could load data into a dataframe and start further manipulating it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants