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

Fix benchmarking on travis (thanks to @tkf) #323

Merged
merged 2 commits into from
Sep 1, 2019

Conversation

ericphanson
Copy link
Collaborator

No description provided.

@ericphanson
Copy link
Collaborator Author

ericphanson commented Sep 1, 2019

Ok, I am maybe starting to understand what was going on. It looks like the problem in #322 was the if: NOT (branch = master) as @tkf suggested (although surely it would have run into the other problems soon after), because Travis says it is running on the master branch so that conditional stops it from running benchmarks. In this PR, it does not use the conditional (I guess it takes the travis config from the PR, which I didn't quite expect). That means however that this bit

        - git fetch origin '+refs/heads/master:refs/remotes/origin/master'
        - git branch master origin/master

errors because the master branch already exists.

What I don't understand is: why is it running on the master branch? I thought it was running on the branch of the PR?

@tkf
Copy link

tkf commented Sep 1, 2019

why is it running on the master branch?

I think GitHub or Travis merges PR to the master branch before CI (I guess that's why the CI doesn't run when you have conflicts). So, maybe the solution could be to use git branch baseline origin/master and tweak the benchmark script to use baseline as the baseline branch name?

@ericphanson
Copy link
Collaborator Author

Ok, that makes sense and is a clever solution; I just pushed a commit to try that. I don't quite understand why it works on the Transducers.jl repo though. Does the conditional if: NOT (branch = master) mean you use it instead on branches you push?

@ericphanson ericphanson changed the title Fix travis (thanks to @tkf) Fix benchmarking on travis (thanks to @tkf) Sep 1, 2019
@tkf
Copy link

tkf commented Sep 1, 2019

Yeah, if: NOT (branch = master) thing was OK for my case because I push branches directly to https://github.com/tkf/Transducers.jl. So, when I create a PR, there is always a non-master branch (and I run CI only on those branches, not the one created automatically for each PR).

@ericphanson
Copy link
Collaborator Author

Ah ok, that makes sense. Looks like the benchmarks are running! Thanks for all your help :)

@ericphanson
Copy link
Collaborator Author

Ok great, Travis works perfectly now. As an interesting aside, this PR (travis log link) gives us reliability info for Travis as a benchmarking platform, since there were no code changes (so everything should be invariant).

Indeed, most of the benchmarks are invariant; the ones which aren't are:

––––––––––––––––––––––––––––––––––––––– ––––––––––– ––––––––––––
             ["ECOS", "GT_constraints"] 0.72 (5%) ✅    1.00 (1%)
             ["ECOS", "LT_constraints"] 0.75 (5%) ✅    1.00 (1%)
       ["ECOS", "equality_constraints"] 0.76 (5%) ✅    1.00 (1%)
              ["SCS", "GT_constraints"] 0.77 (5%) ✅    1.00 (1%)
               ["SCS", "LT_constraint"] 0.00 (5%) ✅  0.00 (1%) ✅
              ["SCS", "LT_constraints"] 0.79 (5%) ✅    1.00 (1%)
             ["SCS", "SDP_constraints"] 0.80 (5%) ✅    1.00 (1%)
        ["SCS", "equality_constraints"] 0.77 (5%) ✅    1.00 (1%)
      ["formulation", "GT_constraints"] 0.71 (5%) ✅    1.00 (1%)
      ["formulation", "LT_constraints"] 0.72 (5%) ✅    1.00 (1%)
["formulation", "equality_constraints"] 0.75 (5%) ✅    1.00 (1%)

All the benchmarks ending with constraints (plural) involve a lot of allocations and GC, and represent how you are not supposed to make constraints with Convex (e.g. 1000 scalar constraints on a vector of length 1000, instead of a single vector constraint). All of the non-invariant constraints are of this form, except the ["SCS", "LT_constraint"] one, where it appears something weird happened (no memory use, no time...). And indeed, the differences in time seem to come from GC time in the log (8% vs 23%). So maybe GC is sometimes way slower depending on system load?

So I think this might be a datapoint to suggest Travis is pretty decent for benchmarking low-allocation code but we should be wary of its reliability on high-allocation code.

@ericphanson ericphanson merged commit 8956166 into jump-dev:master Sep 1, 2019
@ericphanson ericphanson deleted the fix-travis branch September 1, 2019 01:22
@ericphanson ericphanson mentioned this pull request Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants