-
Notifications
You must be signed in to change notification settings - Fork 0
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
Combine recent data files w/ incomplete benchmarks #207
Conversation
My understanding so far is that a github actions runs regularly to update the plots. Looking at the actions history in Github I see several runs over time. Is there an artifact or label of files that shows which of the Github action runs was used to generate the latest plots show in the readme? I'm wondering if this would help identify which GH action run might have led to the issue. |
Not a label of which GH action run, but we can match the timestamps on the files with the timestamps of the actions. The "run-benchmarks" action is run upon merging a PR, and then there is a commit titled "Update benchmark results [benchmark chore]" and a set of actions run on that (skipping benchmarks to avoid an infinite loop). |
Some additional info:
Edit: as explained in #206 (comment),
|
This PR is a containment to fix our README and clean up our results folder. I propose to solve the underlying issue of saving one result file per run for each metric in a separate PR. |
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 believe the root cause of this issue is that run-benchmarks attempt # 1 on commit c135493 began close to midnight UTC, so some data were saved on Feb 3 at 23h and and some on Feb 4 at 00h, resulting in separate files and separate points on the plot over time.
hahah that's a pretty funny corner case. Would it be better to merge the two csv files then as opposed to deleting them?
Good idea- I combined the files and updated the title of the PR before merging. |
Is there a (small) risk this could happen again? Wondering if its worth a follow-up ticket in the futre to refacto and avoid re-occuring. |
Yes definitely. I'd think that each time the script is kicked off, the date should be set, and then not "checked" again. |
Yes, I opened #213 to fix the multiple timestamp issue. |
Fixes #206.
After removing the two problematic data files from yesterday and earlier today (the second a re-run of yesterday's benchmarks), I recovered the desired plot, which includes the latest results from today.
The plot was obtained from running the plotting script locally, which pulls existing results (including the latest run today on
main
), and we can see that the data is virtually unchanged, as expected.Only the results from earlier today (re-run of yesterday's incomplete benchmark run) are currently visible in the plot over time on
main
because the plotting scriptplot_avg_benchmarks_over_time.py
looks for unique dates in the dataframe, andpandas.core.groupby.SeriesGroupBy.unique
returns the first unique value for each group.I haven't determined what caused the earlier runs to give strange or incomplete results, but our latest data looks good, so it has "fixed itself" for the moment. I'll still investigate to see if recurrence is likely / easily preventable.