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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 25, 2018
Merged

Fix #943 - switch to one compiler per OS #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
483169d
No more travis; use Jenkins.
seantalts Jul 16, 2018
6bd9304
Reorder to allow Mac to be used
seantalts Jul 16, 2018
178e788
Use GCC on Linux.
seantalts Jul 19, 2018
4d654c8
Use new test plan:
seantalts Jul 21, 2018
6ad2dfb
Use GCC on all Linux tests; change name
seantalts Jul 21, 2018
d5627ab
More easily see what compilers are being used.
seantalts Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 0 additions & 175 deletions .travis.yml
View file
Open in desktop

This file was deleted.

70 changes: 36 additions & 34 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,6 @@
@Library('StanUtils')
import org.stan.Utils

def setupCC(Boolean failOnError = true) {
errorStr = failOnError ? "-Werror " : ""
"echo CC=${env.CXX} ${errorStr}> make/local"
}

def setup(Boolean failOnError = true) {
sh """
git clean -xffd
${setupCC(failOnError)}
"""
}

def runTests(String testPath) {
sh "./runTests.py -j${env.PARALLEL} ${testPath} --make-only"
try { sh "./runTests.py -j${env.PARALLEL} ${testPath}" }
Expand Down Expand Up @@ -112,9 +100,9 @@ pipeline {
script {
deleteDir()
retry(3) { checkout scm }
setup(false)
sh "git clean -xffd"
stash 'MathSetup'
sh setupCC()
sh "echo CC=${env.CXX} -Werror > make/local"
parallel(
CppLint: { sh "make cpplint" },
Dependencies: { sh 'make test-math-dependencies' } ,
Expand All @@ -125,6 +113,7 @@ pipeline {
post {
always {
warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true
warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is, yeah.

deleteDir()
}
}
Expand All @@ -134,35 +123,36 @@ pipeline {
steps {
deleteDir()
unstash 'MathSetup'
sh setupCC()
sh "echo CC=${env.CXX} -Werror > make/local"
sh "make -j${env.PARALLEL} test-headers"
}
Copy link
Member

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.

Copy link
Member Author

@seantalts seantalts Jul 24, 2018 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member Author

@seantalts seantalts Jul 24, 2018 •
edited
Loading

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.

Copy link
Member

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:

  1. update issue Test one compiler per OS #943 appropriately so it's scoped to what you've fixed.
  2. 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.

Copy link
Member Author

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.

post {
always {
warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true
warnings canRunOnFailed: true, consoleParsers: [[parserName: 'GNU C Compiler 4 (gcc)'], [parserName: 'Clang (LLVM based)']]
deleteDir()
}
}
}
stage('Vanilla tests') {
stage('Linux Unit with MPI') {
agent { label 'linux' }
steps {
deleteDir()
unstash 'MathSetup'
sh "echo CC=${MPICXX} >> make/local"
sh "echo STAN_MPI=true >> make/local"
runTests("test/unit")
}
post { always { retry(3) { deleteDir() } } }
}
stage('Always-run tests') {
parallel {
stage('Unit') {
agent any
steps {
deleteDir()
unstash 'MathSetup'
sh setupCC()
runTests("test/unit")
}
post { always { retry(3) { deleteDir() } } }
}
stage('Distribution tests') {
agent { label "distribution-tests" }
steps {
deleteDir()
unstash 'MathSetup'
sh """
${setupCC(false)}
echo CC=${env.CXX} > make/local
echo 'O=0' >> make/local
echo N_TESTS=${env.N_TESTS} >> make/local
"""
Expand All @@ -183,30 +173,42 @@ pipeline {
}
}
}
stage('Mac Unit with Threading') {
Copy link
Member

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.

Copy link
Member

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?

agent { label 'osx' }
steps {
deleteDir()
unstash 'MathSetup'
sh "echo CC=${env.CXX} -Werror > make/local"
sh "echo CXXFLAGS+=-DSTAN_THREADS >> make/local"
runTests("test/unit")
}
post { always { retry(3) { deleteDir() } } }
}
}
}
stage('Modded tests') {
stage('Additional merge tests') {
when { anyOf { branch 'develop'; branch 'master' } }
parallel {
stage('Unit with GPU') {
agent { label "gelman-group-mac" }
Copy link
Member

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"

Copy link
Member Author

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.

Copy link
Member

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.

steps {
deleteDir()
unstash 'MathSetup'
sh setupCC()
sh "echo CC=${env.CXX} -Werror > make/local"
sh "echo STAN_OPENCL=true>> make/local"
sh "echo OPENCL_PLATFORM_ID=0>> make/local"
sh "echo OPENCL_DEVICE_ID=0>> make/local"
runTests("test/unit")
}
post { always { retry(3) { deleteDir() } } }
}
stage('Unit with MPI') {
agent any
stage('Linux Unit with Threading') {
Copy link
Member

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)

Copy link
Member

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.

agent { label 'linux' }
steps {
deleteDir()
unstash 'MathSetup'
sh "echo CC=${MPICXX} >> make/local"
sh "echo STAN_MPI=true >> make/local"
sh "echo CC=${GCC} >> make/local"
sh "echo CXXFLAGS+=-DSTAN_THREADS >> make/local"
runTests("test/unit")
}
post { always { retry(3) { deleteDir() } } }
Expand Down