-
Notifications
You must be signed in to change notification settings - Fork 53
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
ci: Add caching / storing of bench report on default branch #290
ci: Add caching / storing of bench report on default branch #290
Conversation
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.
See comments
uses: actions/upload-artifact@v3 | ||
with: | ||
name: bench-artifact | ||
path: bench-artifact.txt | ||
retention-days: 90 |
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.
Does this over-write the previous artifact? Since the "download" artifact uses the same name.
Seems like it would make sense to create two artifacts.
bench-artifact-develop.txt
bench-artifact-COMMIT_HASH.txt
Then we can keep their history and tie it to individual commits hashes
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.
So I was aiming to see if we can keep one file (the latest commit on develop branch's benchmark report) and always compare just with the report, which does mean we overwrite the latest on develop artifact (this could have issues if multiple runs are writing at the same time, but I just wanted to see it working barebones in this PR the rest was scoping for the next PR that does the actual comparisons).
Are you suggesting to use the same approach above but with bench-artifact-develop.txt
and in addition store all previous commit reports that were pushed to develop as well with bench-artifact-COMMIT_HASH.txt
(store previously stored artifact before overwriting) even if we don't use bench-artifact-COMMIT_HASH.txt
in CI for anything? This can be nice quickly see bench mark report of any previous commit quickly.
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.
Two not-strong arguments in favor of keeping history:
- non-zero/positive probability that benchmark information of current commits/PRs is useful in the future, e.g. to understand performance impact of some past changes to understand our "performance options" at a certain point
- storage is cheap
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.
So I was aiming to see if we can keep one file (the latest commit on develop branch's benchmark report) and always compare just with the report, which does mean we overwrite the latest on develop artifact (this could have issues if multiple runs are writing at the same time, but I just wanted to see it working barebones in this PR the rest was scoping for the next PR that does the actual comparisons).
Are you suggesting to use the same approach above but with
bench-artifact-develop.txt
and in addition store all previous commit reports that were pushed to develop as well withbench-artifact-COMMIT_HASH.txt
(store previously stored artifact before overwriting) even if we don't usebench-artifact-COMMIT_HASH.txt
in CI for anything? This can be nice quickly see bench mark report of any previous commit quickly.
Yupp, suggesting the additional per commit on develop benchmark artifact. Even if we're not using it yet it'll be useful soon, and as Orpheus said, storage is cheap
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.
Yup was intending to add that in the next PR because then there will be an artifact uploaded to cache already (when this PR gets merged).
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.
Might as well just add it to this PR, just a few extra lines.
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.
So this PR should just handle the uploading of the artifacts with their hashes on a develop push event. That is what the latest change on this PR is doing.
Next PR will have the fetching of a previous run's artifact logic and comparisons of the benchmark reports.
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
…twork#290) This PR starts uploading benchmark reports of every commit pushed (through PR) on the develop branch. Next PR will: - use the artifact downloader (which enables us to find the artifacts of previous runs). - compare the current benchmark report with the latest artifact report stored on develop.
This PR starts uploading benchmark reports of every commit pushed (through PR) on the develop branch.
Next PR will: