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

Adds a CI/CD workflow to detect performance regression. #264

Merged
merged 12 commits into from
May 10, 2023

Conversation

cheqianh
Copy link
Contributor

@cheqianh cheqianh commented Apr 26, 2023

Description

This PR enables GitHub Actions to detect performance regression using ion-python-benchmark-cli.

This PR adds

  1. a --results-file option to specify benchmark results destination,
  2. a compare command to compare perf results between the previous commit and the current one, and
  3. a CI/CD workflow to detect performance regression

Important Notes

  1. The threshold is set to 1, which should be optimized later. Refer to ion-java-benchmark-cli#53 for details.
  2. Currently the workflow compare the new commit with itself since the main branch doesn't supports features such as compare and output-results. Need to change the target branch back to the main branch here after merging this PR.

Details

The --results-file Option

The --results-file writes the benchmark results to the specified destination in Ion. Otherwise the tool will print the result table in stdout. An example result below.
Running

python amazon/ionbenchmark/ion_benchmark_cli.py read -o my_output tests/benchmark_sample_data/integers.ion 

and you can find the result in file my_output

[{file_name:"integers.ion",'file_size (MB)':"9.54e-07",command:"read",options:["load_dump","ion_binary","file"],'total_time (s)':"2.17e-04",'memory_usage_peak (MB)':"4.43e-02"}]

The compare command

This compare command compares two commits and identifies if regression has occurred. The results will be outputted to the specified destination. An example of the comparison result looks like the following:

Running below command, note that both previous_result and current_result are very similar to the my_output above section but just has an additional --format ion_text.

python amazon/ionbenchmark/ion_benchmark_cli.py compare --benchmark-result-previous previous_result --benchmark-result-new new_result my_compare_result

and will see the result in the my_compare_result (Edited to be pretty print for better read experience)

$ion_1_0 [
    {input:"integers.ion",
    command:"read"
    options:["load_dump","ion_binary","file"],
    relative_difference_score:{'total_time (s)':-0.20599999999999998868e+0,'file_size (MB)':0e0}
    }, 
    {input:"integers.ion",
    command:"read"
    options:["load_dump","ion_text","file"],
    relative_difference_score:{'total_time (s)':-0.31395949896868e+0,'file_size (MB)':0e0}
    },
]

both -0.2 and -0.3 are smaller than the threshold +0.6, so no regression is detected!

The CI/CD workflow

A workflow that generates sample data, benchmarks write/read performance of both previous and new commits, compares the results, and identifies if there are any regressions.

Example Output

A good example workflow including both ✅ and ❌ summaries.

Screenshot 2023-04-26 at 1 48 19 PM

and the detailed pipeline log when regression is detected:

Screenshot 2023-04-26 at 1 59 45 PM

After downloading the benchmark results:

$ion_1_0 [{input:"/home/runner/work/ion-python/ion-python/testData/testSexps.10n",command:"write",options:["load_dump","ion_text","file"],relative_difference_score:{'file_size (MB)':0.0e0,'total_time (s)':0.09815950920245405e0}},
{input:"/home/runner/work/ion-python/ion-python/testData/testSexps.10n",command:"write",options:["load_dump","ion_text","buffer"],relative_difference_score:{'file_size (MB)':0.0e0,'total_time (s)':-0.1616766467065868e0}},
{input:"/home/runner/work/ion-python/ion-python/testData/testSexps.10n",command:"write",options:["load_dump","ion_binary","file"],relative_difference_score:{'file_size (MB)':0.0e0,'total_time (s)':0.07633587786259535e0}},
{input:"/home/runner/work/ion-python/ion-python/testData/testSexps.10n",command:"write",options:["load_dump","ion_binary","buffer"],relative_difference_score:{'file_size (MB)':0.0e0,'total_time (s)':0.6116504854368932e0}}
]

we can see that the relative execution time difference for command:"write",options:["load_dump","ion_binary","buffer"] exceeds the threshold 0.6 (back then it's 0.6) so the workflow is failed. Note that we use the current commit to compare against itself so there's nothing really affect the performance. I increased the threshold to 1.

Recommended Review Order

Recommend to start with the GitHub Actions workflow to see the big picture of the workflow, then look into the benchmark-cli implementation to learn more about the technical details.

Test

See CI/CD below, and will create a new PR to change the targeted comparison commit back to the main branch.

* Adds a `--results-file` option.
* Adds a `compare` command.
@cheqianh cheqianh requested review from tgregg and linlin-s April 26, 2023 22:15
@cheqianh cheqianh marked this pull request as ready for review April 26, 2023 22:15
for field in relative_difference_score:
value_diff = relative_difference_score[field]
# TODO simply set the threshold to 1. Need optimization.
if value_diff > REGRESSION_THRESHOLD:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 0 a safer value to choose in the meantime? To exceed 1, wouldn't the new version have to be twice as slow?

@cheqianh
Copy link
Contributor Author

cheqianh commented May 3, 2023

The above commit addressed feedback except the threshold one.

Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

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

Approving, acknowledging that there is still work to do to refine the threshold value.

@cheqianh
Copy link
Contributor Author

cheqianh commented May 10, 2023

Approving, acknowledging that there is still work to do to refine the threshold value.

Okay, I'll open a GH issue for the threshold value, and discuss it with you later.

@cheqianh
Copy link
Contributor Author

Opened an issue here. I'll merge this PR first.

@cheqianh cheqianh merged commit 8cc4e16 into amazon-ion:master May 10, 2023
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.

3 participants