-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
Improve promote_op #17929
Improve promote_op #17929
Conversation
@nanosoldier |
@tkelman I made a mistake and force-pushed a commit. Can we try again? |
Okay, not sure exactly what this'll do. We'll also probably want to compare this against 0.5.0-rc0. @nanosoldier |
Something went wrong when running your job: |
It just adds the job to Nanosoldier's queue, so that it will run as soon as a node is free. It shouldn't mess anything up, other than possibly delivering job statuses in a weird order if the jobs happen to run concurrently with the old job (which will continue to run until completion or failure). The error comment is Nanosoldier trying to report the build failure, but hitting a dumb typo in the error handling code that I've just fixed (some weekend I need to sit down and do a test coverage sprint for Nanosoldier...). I haven't yet deployed that fix, so error comments might be wonky for tonight, but I'll deploy it tomorrow. |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels edit by tkelman: this was relative to master |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
So relative to master this is much better, relative to release-0.5 where #17389 was reverted there are just a couple of regressions and they're not too huge. |
The remaining regressions seem to come from somewhere else. See https://github.com/JuliaCI/BaseBenchmarkReports/blob/07527ce784fc022b854f37a29230dad4816a6895/daily_2016_8_10/report.md |
Jeff, is this okay to merge? |
Yes. Thanks @pabloferz . |
This is the promised fix for #17794 (at least it works locally for me).
CC @JeffBezanson, @tkelman