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

[FR][Python] Bind Benchmark methods / builder pattern #1038

Closed
AntoinePrv opened this issue Sep 5, 2020 · 4 comments · Fixed by #1040
Closed

[FR][Python] Bind Benchmark methods / builder pattern #1038

AntoinePrv opened this issue Sep 5, 2020 · 4 comments · Fixed by #1040

Comments

@AntoinePrv
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Missing features in Python, such as the ability to use->MeasureProcessCPUTime(), ->UseRealTime(), ->Unit(benchmark::kMillisecond)...

Describe the solution you'd like
We currently have

@register(name="Some benchmark)
def sum_million(state):
  while state:
    sum(range(1_000_000))

I suggest extending the API in a similar fashion to the C++ one. So something like

@register(name="Some benchmark)
    .measure_process_time()
    .use_real_time()
    .unit(benchmark.kMilisecond)
def sum_million(state):
  while state:
    sum(range(1_000_000))

Describe alternatives you've considered
Alternatively, we could use Python named parameters inside the register function

@register(
    name="Some benchmark)
    measure_process_time=True,
    use_real_time=True,
    unit=benchmark.kMilisecond,
)
def sum_million(state):
  while state:
    sum(range(1_000_000))

One downside is that in C++, the extra arguments to BENCHMARK_REGISTER are passed to the benchmark function.
Doing so here may prevent us from doing this in the future.

Additional context

@dmah42
Copy link
Member

dmah42 commented Sep 7, 2020

Great, yes please.

I think the python named parameter version is more Pythonic than the builder pattern, so i think i'd go with that if it doesn't complicate the implementation too much. But either is better than none.

@AntoinePrv
Copy link
Contributor Author

So, as I'm working on these bindings, I realize named parameters are going to be awkward when the associated builder method takes multiple arguments:

@register(
    dense_range=(0, 8, 1)
)
def sum_million(state):
  while state:
    sum(range(1_000_000))

The distinction between no parameters methods, and boolean parameter is also not explicited:

@register(
    use_real_time=True,          # Calls ->UseRealTime()
   report_aggregated_only=True,  # Calls ->ReportAgregatesOnly(true)
)
@register(
    use_real_time=False,          # Not calling ->UseRealTime()
   report_aggregated_only=False,  # Calls ->ReportAgregatesOnly(false)
)

@dmah42
Copy link
Member

dmah42 commented Sep 9, 2020

ok, builder method it is then i guess.

@AntoinePrv
Copy link
Contributor Author

I spoke too fast, seems like using a builder pattern (anything with multiple parenthesis) in a decorator is a Syntax Error as of Python 3.8.

Not breaking the current code, we can still use it with the current syntax:

def sum_million(state):
    while state:
        sum(range(1_000_000))

benchmark.register(sum_million, name="Benchmarking sum on a range")
    .measure_process_time()
    .use_real_time()
    .unit(benchmark.kMilisecond)

Not perfect, but it's explicit, it's very close to the C++ API, so I think I will just go with this.
IIRC Python bindings are still in beta, so this can be changed later.

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

Successfully merging a pull request may close this issue.

2 participants