-
-
Notifications
You must be signed in to change notification settings - Fork 188
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 #943 - switch to one compiler per OS #944
Conversation
There is a very active discussion here: http://discourse.mc-stan.org/t/one-compiler-per-os/4899/42 After we decide what to do, we can implement. |
The discussion had reached a point where I think we already decided that:
I think the only crux left in the discussion I think has been centering on whether or not Columbia devs (ie, Bob, Mitzi, and I) should be obligated to work on compiler bugs for compilers we don't test. |
Our threading stuff gets right now only tested on Travis with clang and gcc for good reasons. It would be good to keep these around to ensure that the ad stack is thread safe. |
in this PR I move those tests to Jenkins, though just for one compiler and OS. You think we should do both Linux+gcc and Mac+clang threading tests? |
I'll restart it this afternoon when I go into the office.
…On Thu, Jul 19, 2018 at 8:12 AM seantalts ***@***.***> wrote:
The discussion had reached a point where I think we already decided that:
1. We're okay testing one compiler per OS, as long as the Linux one is
some form of modern-ish GCC (and clang for Mac and gcc 4.9.3 for windows)
2. We're okay testing the distribution tests on just clang and *nix
(current status)
I think the only crux left in the discussion I think has been centering on
whether or not Columbia devs (ie, Bob, Mitzi, and I) should be obligated to
work on compiler bugs for compilers we don't test.
Plus some suggestions about how to test more quickly. I'm mostly waiting
for Ben to restart the Linux box so I can try to figure out how we can run
our tests on GCC without crashing that box before merging this.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#944 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqrSor7kCcxt5Jy04k1G4gpHbC0nIks5uIHefgaJpZM4VRdmV>
.
|
We need to test the ad tests with gcc and clang, yes...but really only those which were in the Travis file configured. Your suggestion makes sense to me. |
@bgoodri Thank you! Sorry for crashing it again. |
So I think before I merge this I need to make the threading tests execute on both clang and gcc and probably reorganize things a little. a future TODO may be to do this interstitial branch before develop or some thing like that such that we move some of these tests to post PR. |
67ba28c
to
4d654c8
Compare
I think it was pretty helpful; I like the test plan we have now better than the one we had a few days ago. |
@syclik I tried to tally up the changes in the top-level post in this PR. I was thinking of working that into this wiki page (and updating it to basically say what we test and what C++ level we think we can use, rather than what we "support" since that is ambiguous). Thoughts on that? |
@syclik Did you want to review this and make sure everything is up to your standards? The tests completed and currently show the outline of what is tested on a PR in the gui for this PR's test results. I updated the wiki and the PR to show what we test now (and the PR to show what we did test). |
Sure. I didn't realize it was waiting on a review. I'm on it. |
I think it's ready only today, haha. I wasn't sure if you wanted to review the PR given that you okay'd the plan in the other thread. |
ha! I'm definitely ok with the plan! But no real need for me to review the PR other than I'm looking at it now. |
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.
Neat. A few changes required:
- test-headers is only tested on one compiler. We should test it on the ones we say we're testing it on. (It's not just the unit tests that'll pick up all the compiler compatibilities.)
- I think the GPU tests were meant to run on either linux or Mac. Right now, I think it's only running on Mac by accident.
- Where are the Windows tests??
Jenkinsfile
Outdated
unstash 'MathSetup' | ||
sh setupCC() | ||
sh "echo CXXFLAGS+=-DSTAN_THREADS >> make/local" | ||
runTests("test/unit") |
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.
Do we want to specify the number of threads? The current implementation uses the STAN_NUM_THREADS
environment variable.
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.
The same comment applies to all the other uses of threads.
@@ -125,6 +125,7 @@ pipeline { | |||
post { | |||
always { | |||
warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true | |||
warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true |
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.
The make test-math-dependencies
outputs things in the exact format as CppLint. Should we just use that as the parser? I don't know if the parserName
is just a name or if it refers to a particular type of parser.
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.
This is leftover from before I was here. I don't know how these custom parsers work and messing with it is a separate issue.
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.
If this is the case, is this change necessary? Is there even a separate parser called math-dependencies
?
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.
There is, yeah.
@@ -139,23 +140,24 @@ pipeline { | |||
} |
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.
It won't let me comment directly on L133 (just above this), so I'm putting it here.
We should be running the header tests on every place where you said we'd be testing (the three places you've highlighted). This will catch compiler incompatibilities and some header include incompatibilities. We should be passing this on all the compiler / platforms you've listed.
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 currently only test it on one platform. I'm worried we're taking this thread about reducing the compilers we support and making us actually test more toolchain/OS setups because we didn't realize what was being tested in the first place.
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'm just doing a review and checking what you've said against what you've done. Here, it's just come up short.
So... please add it so it matches your top-level PR comment? Or... update your top-level PR comment with this special case? I think it makes more sense to add it if that's what we're claiming we're testing.
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.
The top-level note is referring to the unit tests in most places that lack a qualifier. I can add that explicitly to the "now we test" section. I'm not sure how the header include order tests would break on just one compiler but we can address that when it happens or in an orthogonal issue.
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.
Thanks for updating that in the pull request. Since this pull request doesn't fully fix the issue, can you do these things:
- update issue Test one compiler per OS #943 appropriately so it's scoped to what you've fixed.
- create a new issue for the remaining items that weren't addressed here so we know to go and fix them?
I'm happy getting this in as an partial fix to the overall issue, but we need to note that it's not complete.
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.
Are you just saying to liberally sprinkle the phrase "unit tests" all over #943? Are the remaining issues you're talking about the sysadmin stuff on the Xenial box? We haven't typically created math repo issues for that kind of thing before.
@@ -183,9 +185,21 @@ pipeline { | |||
} | |||
} | |||
} | |||
stage('Mac Unit with Threading') { |
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.
Suggestion: can we label these more explicitly now so it matches what we're saying? A more concrete suggestion:
Unit: mac / clang 6 / libc++ Threading
.
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.
How do I know this is clang 6 and it uses libc++? Is there some way to specify that explicitly in the setup of make local?
} | ||
} | ||
stage('Modded tests') { | ||
stage('Additional merge tests') { | ||
when { anyOf { branch 'develop'; branch 'master' } } | ||
parallel { | ||
stage('Unit with GPU') { | ||
agent { label "gelman-group-mac" } |
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 you meant to remove the agent
label so that it can run either on: "mac / clang 6 libc++ OR linux / gcc 5 stdlibc++ GPU"
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.
The Linux box, among other things, does not have a working GPU install now. That's something I think @SteveBronder is going to work on at some point with Ben.
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.
Thanks for updating the top-level PR comment with this information.
@@ -200,13 +214,13 @@ pipeline { | |||
} | |||
post { always { retry(3) { deleteDir() } } } | |||
} | |||
stage('Unit with MPI') { | |||
agent any | |||
stage('Linux Unit with Threading') { |
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.
Shouldn't we also be running the header tests on the other platforms? (To be consistent with what you've said)
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.
For the record, we're pushing this off on to another issue / PR.
So... I didn't even notice, but it's missing Windows. Completely.
…On Tue, Jul 24, 2018 at 1:58 PM ChrisChiasson ***@***.***> wrote:
If/once this goes through, does it automatically take hold on the complex
feature branch, or do I need to rebase onto the latest develop?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#944 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZ_F1XXavaIJJ-waiI0-INhDtvfjs8Sks5uJ2AygaJpZM4VRdmV>
.
|
I should add - there are sort of two loose ends from the thread. One is getting clang 6.0 to work on Ben's Ubuntu box with stdlibc++ (right now it's still using clang 3.8 when it runs distribution tests; gcc otherwise). The other is getting gcc 7 or 8 to work on the same box, haha. For now it's using the default gcc 5. Troublesome computer. @ChrisChiasson It wouldn't hurt for you to merge in develop as a way to restart the tests. That merge should be problem free anyway for it to be able to run the tests and be merged in to develop. Rebasing also works but I find that logistically it can... often provide many more opportunities to resolve conflicts and no real benefit other than making the history prettier (from some perspectives). |
@syclik Windows hasn't been tested by the Math library for a long while - I mentioned this in the thread, but I think they've been broken. I forget when this happened (or if it happened before I started). |
I don't know what thread you've mentioned it on. On this PR, you mention that we should test against Windows, but there's no other mention. If we're not building on Windows, that sounds like a high priority bug. I didn't know that we're not building on Windows. There isn't an open issue. Can you file an issue with what you know? |
I mentioned it in the discourse thread, here. I don't know anything other than that the tests were probably broken on Windows. I'll write an issue that says that. |
We do build and test CmdStan integration tests on Windows, btw. So some of the math library is tested that way. |
Thanks. If there's an issue open, I can help track it down.
I can't even find where you mentioned in on that thread...
…On Tue, Jul 24, 2018 at 2:51 PM seantalts ***@***.***> wrote:
I mentioned it in the discourse thread, here
<http://discourse.mc-stan.org/t/one-compiler-per-os/4899/91>. I don't
know anything other than that the tests were probably broken on Windows.
I'll write an issue that says that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#944 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZ_F1qs8u5mz-OqcBGN8nkcHzOWMKgZks5uJ2yBgaJpZM4VRdmV>
.
|
The way discourse hijacks the find-in-page browser command is one of the worse UI mistakes I've seen. With clever JS they could probably have gotten the same information on user searches sent to them without destroying usability. |
You can get back to the browser command by hitting ctrl-f twice in a row.
The only problem is that discourse tries to be clever and doesn't load all
the comments in the thread, making that search useless on long threads.
…On Tue, Jul 24, 2018 at 3:02 PM seantalts ***@***.***> wrote:
I can't even find where you mentioned in on that thread...
The way discourse hijacks the find-in-page browser command is one of the
worse UI mistakes I've seen. With clever JS they could probably have gotten
the same information on user searches sent to them without destroying
usability.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#944 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZ_F1LdMFJYWaIrlnyUzjg95P3syZmFks5uJ282gaJpZM4VRdmV>
.
|
Nice! I guess that's why they hijack it. |
I added a commit that should make it a little easier to tell what compiler is being used (the toolchain is the default - do you want me to explicitly set it so we know what is being used?). I think its best if compiler locations are set in the Jenkins server node config instead of in the Jenkinsfile, but this will print out what compiler is used like so: |
I also added clarification to both the wiki and the PR top comment saying we were talking about unit tests and adding asterisks to things that are aspirational. I think it's ready for review again. |
@syclik Would you mind taking another look? Thanks. |
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.
Thanks for making it easier to tell what compiler is being used. Can we set the toolchain explicitly? Defaults can change accidentally, so I think it's much better being explicit about that.
There are no code changes necessary. I'm going to mark as "Request changes" so we don't merge until we have the existing issue updated to the appropriate scope and a new issue that lists the remaining items that haven't been addressed. Once those are in, I'll approve this one and merge.
@@ -125,6 +125,7 @@ pipeline { | |||
post { | |||
always { | |||
warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true | |||
warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true |
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.
If this is the case, is this change necessary? Is there even a separate parser called math-dependencies
?
@@ -139,23 +140,24 @@ pipeline { | |||
} |
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.
Thanks for updating that in the pull request. Since this pull request doesn't fully fix the issue, can you do these things:
- update issue Test one compiler per OS #943 appropriately so it's scoped to what you've fixed.
- create a new issue for the remaining items that weren't addressed here so we know to go and fix them?
I'm happy getting this in as an partial fix to the overall issue, but we need to note that it's not complete.
} | ||
} | ||
stage('Modded tests') { | ||
stage('Additional merge tests') { | ||
when { anyOf { branch 'develop'; branch 'master' } } | ||
parallel { | ||
stage('Unit with GPU') { | ||
agent { label "gelman-group-mac" } |
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.
Thanks for updating the top-level PR comment with this information.
@@ -200,13 +214,13 @@ pipeline { | |||
} | |||
post { always { retry(3) { deleteDir() } } } | |||
} | |||
stage('Unit with MPI') { | |||
agent any | |||
stage('Linux Unit with Threading') { |
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.
For the record, we're pushing this off on to another issue / PR.
And thanks for updating the wiki. I'm going to edit the top-level comment to have a link to the wiki page. |
@ChrisChiasson, if you want to continue this discussion, please start a new thread on discourse. It'll get lost here and it's off topic. (Btw, that's the default behavior of make. We already have the dependencies done correctly, so if you do this locally without cleaning, it'll only build the tests that have changes to its dependencies.) |
Sure. I don't think they've changed before but we can set it explicitly with a little more infrastructure. I agree that that would be the ideal situation, I just didn't know if it was worth putting the work in since it's never been needed before. |
Actually can we just create an issue for that too? I see how setting the toolchain explicitly could be related to this PR but I need to work on other things now. |
Absolutely! Please feel free to punt, but make sure it gets recorded as something we need to do for the future. I might be paranoid, but I've seen Mac's clang version change with an update. I don't know if we're disciplined enough with the other boxes to make sure the defaults stay the same. But please, by all means, leave that for a future date. |
Maybe I misunderstood you - the compiler IS set explicitly in the node config for each Jenkins agent here and then printed out when the job actually runs. I think this makes sense as the install location even for a specific version of a compiler may change. I thought you were referring to the standard library, which on OS X and FreeBSD is always libc++ for clang and on linux or GCC is always stdlibc++. Those configs are pointing to specific versions of clang now (and they are installed by homebrew on OS X), so we shouldn't see those changing out from under us either. |
Sorry -- I misunderstood you. I think given that clarification, we're good. I think using the default standard library is standard practice. I don't think we'll be switching that anytime soon and I don't want to be overengineering for a way off use case. Thanks! |
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 just created #952 to list the remaining items. We can merge this.
See #943. This also gets rid of Travis and integrates the tests we want to keep into Jenkins.
Travis was running these configurations:
And Jenkins was running:
With this PR we will run:
On PRs:
Additional tests on merge to develop
So there will be a little bit of non-determinism for the sake of running tests more quickly on available hardware.
[1] the GPU libraries are currently broken on Linux, so currently they run on Mac and Linux is an aspirational target
[2] clang 6 is also aspirational on that Linux box - clang 3.8 is the only one that works without libc++ right now.
Documentation
This information is captured in a more permanent location in the wiki: https://github.com/stan-dev/stan/wiki/Supported-C---Compilers-and-Language-Features
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Columbia University.
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: