prestodb / benchto

Framework for running macro benchmarks in a clustered environment
Apache License 2.0
24 stars 18 forks source link

Make benchmarks hardware agnostic #37

Open sopel39 opened 7 years ago

sopel39 commented 7 years ago

Currently benchmarks contains attributes which are specific to the hardware and cluster:

This makes reusing benchmarks on different clusters problematic since those elements need to be fine tuned to a given execution environment.

Major requirements of benchmarks are:

Having those requirements we propose to refactor benchto-driver and benchmarks in the following way:

FYI: @findepi @idemura

idemura commented 7 years ago

Thank you for quick response. Let's move all discussion here, in one place.

I can't agree that inheritance itself is bad as and idea. It works good in environments like Google borg, Google config language to name few and this is very convenient. What is problem there actually is that every parameter can be overriden. Before I explain approach to fix this, let's see what we try to solve here:

Approach you suggest looks not generic enough. Every time something rises a new tiny-little-thingy that needs to be tuned we have to introduce more and more code in driver to support and so code of benchmark is coupled with Java code. I'd like to have this possibility in configs themselves.

I propose to fix inheritance. I totally agree that uncontrolled overriding can be a problem (and I observed such abuse at Google!). Let's only apply inheritance to designated section, say params or overrides. Other sections remain untouched by overrides (you can only replace them, as it was in a new file, or only add - we can figure out what behavior we want).

Example:

overrides:
  cluster: prn1
  catalog: movies

library:
  name: ${movies}
  url: master.${cluster}.facebook.com
  capacity: 100GB

If I need to tune this, I do in another file:

base: ../template/base.yaml

overrides:
  cluster: prn2

I see this to be:

Thoughts?

sopel39 commented 7 years ago

I think we are converging. I think we can mix approaches and decide on some convention (including inheritance).

Let's have example benchmark definition (presto/tpch.yaml):

datasource: presto
query-names: presto/tpch/${query}.sql
runs: 6
prewarm-runs: 2
before-execution: sleep-4s, presto/session_set_reorder_joins.sql, presto/session_set_join_distribution_type.sql
frequency: 7
database: hive

# default schema and data size mappings
tpch_tiny: 1gb
tpch_medium: 10gb

variables:
  1:
    query: q2, q8, q9
    schema: ${tpch_medium}
    reorder_joins: true
    join_distribution_type: automatic

Then we could have a "profile" yaml, for instance:

runs: 6
prewarm-runs: 2
tpch_tiny: 10gb

# NEW: it should be possible to override properties of individual benchmarks
presto.tpch.runs: 8
presto.tpch.tpch_tiny: 100gb

We could also have conventional (not enforced) overrides as you mentioned, e.g: benchmark:

...
overrides:
  tpch_tiny: 1gb

variables:
  1:
    query: q2, q8, q9
    schema: ${overrides.tpch_medium}

profile yaml:

overrides.tpch_tiny: 10gb
presto.tpch.overrides.tpch_tiny: 10gb

However, I think that inheritance should be explicit parameter to benchto driver instead of base path value, e.g:

--configuration overrides1.yml overrides2.yml

This has following advantages:

What do you think?

FYI: @kokosing

idemura commented 7 years ago

I'm not following everything, but seems to be quite complex because allows lots of ways to do same. In particular, I can't reason about profile yaml. Let's take example from above:

runs: 6
prewarm-runs: 2
tpch_tiny: 10gb

# NEW: it should be possible to override properties of individual benchmarks
presto.tpch.runs: 8
presto.tpch.tpch_tiny: 100gb

# Is this the same as:
presto:
  tpch:
    runs: 8
    tpch_tiny: 100gb

Q1: Is this file named like teradata.profile.yaml? Q2: Is first section contains defaults, and second for particular benchmark and those from particular benchmark inherit from default one? If so, presto.tpch.runs should follow file name presto/tpch.yaml? Q3: Do you restrict somehow what can be overriden?

As I understand, this is about moving all inheritance into one profile file, but inheritance is strictly from default profile?

Let me understand this proposal. I think though we can have two separate for now and figure out what we want from inheritance, what exactly is duplicated and then return back to this.

kokosing commented 7 years ago

I will try to answer your questions (@sopel39 please correct me if am wrong) and add my two cents.

A1. It could. A2. Yes. Yes. A3. No. The idea is leave this up to the user. We think that user may impose a convention that only some variables should be updatable, like in your case only these from overrides section.

Additionally I think that driver should be able to print out the all benchmarks configuration where all the inheritance (profile files) are applied (like docker-compose) with additional information what file produced this line. That way it will be really easy to track down any kind of issue with benchmark configuration.

I think the current problem (feature) with driver is that it loads all the benchmark files at once without any kind of order. We take advantage of this as we are running all the benchmarks against the cluster within some time limit. So this is why we would like to have one profile file to configure them all, or to have couple profile files to configure couple groups of benchmarks.

As I understand you would prefer to run benchmark one by one, is that correct? In that case you would like to say to benchto-driver to run this benchmark file, and then run this one.

sopel39 commented 7 years ago

Let me understand this proposal. I think though we can have two separate for now and figure out what we want from inheritance, what exactly is duplicated and then return back to this.

We could have a copy for now in order not to block benchmarks. Concurrently we could improve benchto and refactor benchmarks so that they follow our proposal, so that copy is no longer needed.

idemura commented 7 years ago

@sopel39 I have concern this this approach.

If you guys say confirm that

If so, presto.tpch.runs should follow file name presto/tpch.yaml?

but above you complain about static binding with paths:

We would avoid static path binding between overrides and benchmark files.

Looks like contradiction to me. Profile should not reference file.

Consider use case. You have tpch, tpcds, facebook benchmarks (for our typical worloads), for your workload. Now I want say:

So, I want run only some benchmarks (should be supported), ability to have tpch with 2 profiles, maybe different profiles for facebook benchmark.

Params to benchmarks, as we concluded above, should be explicit in template (benchmark file with description of files).

# tpch.yaml:
profile: # overrides before
  a = ...
  b = ...

<description>

and profiles, that you can reuse (in command line: --benchmark=tpch.yaml --profiles=facebook_daily.yaml):

# facebook_daily
a = ...
b = ...

Seems like very clear design that supports all that we agreed to be values of this feature.

sopel39 commented 7 years ago

but above you complain about static binding with paths:

It's not strictly a path, but rather benchmark name. The difference is that profile yaml can be located anywhere and override specific benchmark params by name, which base was explicitly pointing to a benchmark file with relative path.

So, I want run only some benchmarks (should be supported), ability to have tpch with 2 profiles, maybe different profiles for facebook benchmark.

Yes, we have a mechanism for that which is called activeBenchmarks. We are working to improve it, see @findepi PR: https://github.com/prestodb/benchto/pull/38

Other than that I think your use case/proposal is in line with our idea. Therefore I think we could follow this direction.

idemura commented 7 years ago

Great. Now there is not path in environment overrides section so profile (in env) can be applied to any benchmark description. It is not coupled with file name as I can see from PR. Good! Thanks.