-
Notifications
You must be signed in to change notification settings - Fork 520
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
Implementing BDN Perf pipeline #758
base: main
Are you sure you want to change the base?
Conversation
…king. Also, a fix in ClusterBench for and renamed Lua Runner benchmarking
… build statement in YML
…vs previous BDN perf numbers
… push collisions with GitHub
…etimes is in 40%+ range of difference.
…ntinuous benchmarking and also changed to where script is only checking allocated values and let github-action-benchmark compare mean values
… tmp which does have access
…vary into the 40% from run to run.
Associated documentation (will send to team when PR is merged): |
"framework": "net8.0", | ||
"filter": "BDN.benchmark.Cluster.ClusterOperations.*", | ||
|
||
"expectedGet_DSV_AllocatedValue_linux": 0, |
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 think we should only have to add files and entries with a non-zero allocated target. the script should assume, if it does not find a benchmark entry, that the allocated should be zero. that way this entire file gets deleted. and, many lines in other files get deleted as well. will make maintenance a tad easier.
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 see where you are coming from but not sure I agree with this. Maintenance is easiest if it is consistent so we know one config file per BDN run. Also, assuming it should be zero if not there is an assumption and it might be better to specifically state to show we know it should be zero. Currently, the BDN name/ filter is in the config file as well, so that would be a change to only just use the test name.
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.
Since we are only pulling values we need for the specific test (based on other comment), we need to know what values we need for that specific test. Those specific values are in the config file, so I think it only makes sense to have all config files even if the values are 0.
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.
we could have one config file that has a section per test. for every test we add we would simply run the test, and for every result, we would comare it to either zero (by default) or an override (if specified in the JSON).
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.
Yeah, I guess that would be another way to implement it
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 will try to implement it with one config file for all BDN test runs
test/BDNPerfTests/ConfigFiles/CI_CONFIG_BDN_Benchmark_Cluster.ClusterOperations.json
Outdated
Show resolved
Hide resolved
test/BDNPerfTests/ConfigFiles/CI_CONFIG_BDN_Benchmark_Operations.ObjectOperations.json
Outdated
Show resolved
Hide resolved
test/BDNPerfTests/ConfigFiles/CI_CONFIG_BDN_Benchmark_Lua.LuaScripts.json
Outdated
Show resolved
Hide resolved
test/BDNPerfTests/ConfigFiles/CI_CONFIG_BDN_Benchmark_Lua.LuaScripts.json
Outdated
Show resolved
Hide resolved
test/BDNPerfTests/ConfigFiles/CI_CONFIG_BDN_Benchmark_Operations.BasicOperations.json
Outdated
Show resolved
Hide resolved
have shorter names for values not have configuration or framework (moved to ps1 file) not have windows / linux differnce in expected Moved acceptable allocated range to script modified the script: to only load values for the specific test instead of looking for all values changes to match the changes in config (shorter names, etc)
There are three areas of this PR:
a) Runs individual bdn test based on the associated config file (in /config) directory
b) Compares "allocated" value found from the run to the expected value in config file and if it more than 10% from expected, it will pop a fail
a) Goes through the matrix of Linux, WIndows and runs each BDN test on those two (total of 10 runs)
b) Each run saves results file and error log to GH Actions artifacts (for debugging if needed)
c) Each run takes the BDN results (from the ps1 file run) and compares MEAN to previous and if changes by 50% or more (represented as 150% in yml file) then it will comment in commit
d) Each run has task that adds to the "Summary" part of the GH Action run (table that shows previous run to current run)
e) Each run adds to charts (data is stored in continuousbenchmarking branch in the website/static/charts folder of that branch)
a) Name of BDN filter it is running
b) Expected "ground truth" for Allocated values
b) Acceptable range % for allocated ("acceptableAllocatedRange": 10) before flags a fail