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

Support reporting geometric mean by benchmark tags #132

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

mdboom
Copy link
Collaborator

@mdboom mdboom commented May 24, 2022

Addresses python/pyperformance#208

This reports geometric mean organized by the tag(s) assigned to each benchmark.
This will allow us to include benchmarks in the pyperformance suite that we
don't necessarily want to include in "one big overall number" to represent progress.

mdboom added a commit to mdboom/pyperformance that referenced this pull request May 24, 2022
This requires the following changes to pyperf first: psf/pyperf#132
pyperf/_metadata.py Outdated Show resolved Hide resolved
pyperf/_metadata.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

It seems like you want to add a new key in metadata. In that case, you should specify it in the doc: https://pyperf.readthedocs.io/en/latest/api.html#metadata

I dislike changing the default formatting to add "(all)". Can you try to omit "(all)"? For example, if no benchmark has tags, it's weird to display the magic "all" tag.

            +----------------------+---------------------+-----------------------+
            | Benchmark            | mult_list_py36_tags | mult_list_py37_tags   |
            +======================+=====================+=======================+
            | [1]*1000             | 2.13 us             | 2.09 us: 1.02x faster |
            +----------------------+---------------------+-----------------------+
            | [1,2]*1000           | 3.70 us             | 5.28 us: 1.42x slower |
            +----------------------+---------------------+-----------------------+
            | [1,2,3]*1000         | 4.61 us             | 6.05 us: 1.31x slower |
            +----------------------+---------------------+-----------------------+
            | Geometric mean (all) | (ref)               | 1.22x slower          |
            +----------------------+---------------------+-----------------------+
            | Geometric mean (bar) | (ref)               | 1.37x slower          |
            +----------------------+---------------------+-----------------------+
            | Geometric mean (foo) | (ref)               | 1.18x slower          |
            +----------------------+---------------------+-----------------------+

In this table, it's also not easy for me to understand which benchmarks are used to compute the geometric means for each tag, since benchmark tags are not listed. Would it make sense to list tags?

@vstinner
Copy link
Member

Do you have real examples of tags on benchmarks? I mean what are real tag values?

@vstinner
Copy link
Member

In this table, it's also not easy for me to understand which benchmarks are used to compute the geometric means for each tag, since benchmark tags are not listed. Would it make sense to list tags?

Another option is to render one table per tag: it would only list benchmarks matching this tag, and so the "geometric mean" final row would summarize the table. And there would always be a last table with all benchmarks.

@mdboom
Copy link
Collaborator Author

mdboom commented May 25, 2022

Do you have real examples of tags on benchmarks? I mean what are real tag values?

We're mostly doing this work in expectation of cleaning the tags up to be more useful. The motivation is so we don't overoptimize for microbenchmarks, of which there are currently many in the suite. There's further discussion of how we might use tags going forward.

Another option is to render one table per tag: it would only list benchmarks matching this tag, and so the "geometric mean" final row would summarize the table. And there would always be a last table with all benchmarks.

I like this idea. It also would resolve your other comment about 'all' being surprising in the untagged case.

Addresses python/pyperformance#208

This reports geometric mean organized by the tag(s) assigned to each benchmark.
This will allow us to include benchmarks in the pyperformance suite that we
don't necessarily want to include in "one big overall number" to represent progress.
@mdboom
Copy link
Collaborator Author

mdboom commented Jun 13, 2022

@vstinner: Do these changes work for you?

+----------------+---------------------+-----------------------+
| Geometric mean | (ref) | 1.22x slower |
+----------------+---------------------+-----------------------+
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, that looks great, thank you!

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@vstinner
Copy link
Member

Sadly, the tests fail:

- FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/pyperf/pyperf/pyperf/tests/mult_list_py36_tags.json'

@mdboom
Copy link
Collaborator Author

mdboom commented Jun 16, 2022

Sadly, the tests fail:

- FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/pyperf/pyperf/pyperf/tests/mult_list_py36_tags.json'

Sorry -- forgot to commit the new test files -- let's see how this works.

EDIT: I guess this needs @vstinner or someone to re-approve the CI run.

@mdboom mdboom requested a review from vstinner June 16, 2022 19:29
@vstinner vstinner merged commit 2430bf8 into psf:main Jun 16, 2022
mdboom added a commit to mdboom/pyperformance that referenced this pull request Aug 10, 2022
This requires the following changes to pyperf first: psf/pyperf#132
ericsnowcurrently pushed a commit to python/pyperformance that referenced this pull request Aug 15, 2022
* Support reporting geometric mean by tags

This requires the following changes to pyperf first: psf/pyperf#132

* Ensure `tags` is always a list

* Use property

* Update pyperf
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

Successfully merging this pull request may close these issues.

2 participants