-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[hexagon][testing] refactor benchmark-table code #11400
Conversation
CC: @Lunderberg |
Output is basically unchanged from the previous table implementation:
|
Here's a cleaned-up version of that output:
|
Note: This PR is also lays some groundwork for providing this as a test fixture, if we eventually want to do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the extraction of the benchmark generation into a separate utility. I have a couple questions on the design
- Do we want the CSV names to be different from the names used in the code?
- Should the user be required to provide values for all columns, regardless of success/skip/failure?
If we can answer no to both of those questions, we have the potential for a cleaner interface.
- New columns are implicitly defined by
kwargs
passed to therecord_*
methods. If a column value isn't passed, the CSV has an empty cell in that column. - Column names are optional in
__init__
, but can still be passed. (e.g. To ensure that an "Error" column is always present, even if all benchmarks pass. Or to define the order of columns)
The downside of this approach would be decreased protection against typos in user-defined columns. Given that this is intended to create CSVs as part of ongoing optimization, and would quickly be seen by human eyes, I think that is worth the increased flexibility.
This comment was marked as resolved.
This comment was marked as resolved.
dee4767
to
9e024fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! There some assorted nitpicks, but no blockers.
f523fac
to
8c7f5b2
Compare
Generalize the benchmark-table code to support arbitrary independent values. This supports future changes to the benchmark code.
@cconvey do we want tests/python/contrib/test_hexagon/benchmark_hexagon.py to be tested in the CI? If that's the case, you need to rename the file to |
Thanks for the info! I was wondering what mechanism decided which scripts got run. IMHO I don't think it make sense (yet) for CI to include benchmarking runs, so I suggest we leave this as-is. |
@cconvey sounds good! Since this file is under a test directory, can you rename the file and add |
@cconvey we can also address that in a separate PR. |
Generalize the benchmark-table code to support arbitrary
independent values. This supports future changes to the benchmark
code.
cc @mehrdadh