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

Jump to bottom

upgrade sundials to 4.1.0 #1098

Merged
merged 37 commits into from
Feb 21, 2019
Merged

upgrade sundials to 4.1.0 #1098

merged 37 commits into from
Feb 21, 2019

Conversation

wds15
Copy link
Contributor

@wds15 wds15 commented Jan 5, 2019 •
edited
Loading

Summary

This upgrades Sundials to the new major version 4.1.0.

The changes to the source base of stan-math are:

Tests

All existing tests (should) still run just fine. In particular the ODE & IDAS tests:

test/unit/math/rev/mat/functor/integrate_ode_adams_prim_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_adams_rev_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_bdf_rev_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_cvodes_grad_rev_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_bdf_prim_test.cpp
test/unit/math/rev/mat/functor/idas_integrator_test.cpp
test/unit/math/rev/mat/functor/idas_system_test.cpp

Side Effects

  • The Jenkinsfile had to change. We are using -Werror to run make test-headers. There's a warning in CVODES that prevents us from getting past this -- make test-headers was meant to only test our code. I applied a fix, which is to add a new make target called sundials, build that in the Jenkins test with our normal compiler settings, then calling make test-headers to use -Werror to test the header files.

Checklist

  • Math issue Upgrade Sundials to version 4.1.0 #1097

  • Copyright holder: Sebastian Weber, Generable

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)

  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@wds15
Copy link
Contributor Author

wds15 commented Jan 5, 2019

@bgoodri ... would be good if you can have a look at the printf stuff to make sure it will work with rstan wrt to CRAN policies. Thanks!

@wds15
Copy link
Contributor Author

wds15 commented Jan 5, 2019 •
edited
Loading

This is strange: I am getting unresolved symbol errors for sundials things in tests which do not involve at all any sundials stuff. If someone has an idea where this is coming from - that would be great.

EDIT: This likely has to do with updates to the CVODES interface to it's internal solver routines which changed with version 4.0. Let's see.

@wds15
avoid use of deprecated pre 4.0 interface to linear solvers of sundia…
0b96460 
…ls. The compatibility files from sundials cause link problems otherwise.
@wds15
fix missing include
5b90a69
@wds15
Copy link
Contributor Author

wds15 commented Jan 7, 2019

I think a number of PRs right now hang as the Windows test machine seems to be down. Can someone (@bgoodri , @seantalts ) maybe kick the respective machine? Thanks.

@seantalts
Copy link
Member

Yeah, unfortunately it's completely offline right now and located in NYC. @bgoodri said he should hopefully be able to physically kick it soon. Sorry about that.

@bob-carpenter
Copy link
Contributor

bob-carpenter commented Jan 7, 2019 via email

@wds15
Copy link
Contributor Author

wds15 commented Jan 7, 2019

@bob-carpenter : I think to have found the issue. With Sundials 4.0 the interface to the non-linear solvers was changed. The compatibility wrappers which make 4.0 source compatible with pre-4.0 codes was written in a way such that it does not only declare functions, but defines them instantly in the header files. This causes the problems here. Turning to the new 4.0 interface solves the issue, I think.

@seantalts No worries... I was hoping you have some remote access to the window machines.

@bgoodri Please kick those Windows boxes hard! It's Windows after all.

@syclik syclik changed the title upgrade sundials upgrade sundials to 4.0.1 Jan 8, 2019
@syclik
Copy link
Member

syclik commented Jan 8, 2019

@wds15, how can we confirm if this fixes #1008? If it doesn't, can we be clear that it doesn't?

@wds15
Copy link
Contributor Author

wds15 commented Jan 9, 2019

I am fairly sure it fixes it and was hoping that @bgoodri could confirm by looking and/or testing it. My windows machine I am sitting on is broken right now. The way to test it is to download StanHeaders 2.18.0 revert the changes @bgoodri did to the respective header file (include/stan_sundials_printf_override.hpp) to confirm that things break under Windows with the gcc 4.9. Then you can change that file to be like the one I have created in this PR. This should verify that things work.

Like I say I don't have a windows available which is not broken - maybe a VirtualBox at home will do, but if you have a Windows available then go ahead and test.

@syclik
Copy link
Member

syclik commented Jan 10, 2019

@wds15: ok. We'll wait for @bgoodri to respond.

@wds15
Copy link
Contributor Author

wds15 commented Jan 14, 2019

Ok, i got my windows toolchain working again. However, I am not able to trigger the issue @bgoodri has identified. Thus before this PR gets stalled, I suggest that we take down the claim from this PR to fix the issue @bgoodri found. I think #1008 will be fixed with this, but I cannot verify. I am modifying the PR text and I hope you agree, @syclik, that we can proceed and merge.

@bgoodri
Copy link
Contributor

bgoodri commented Jan 14, 2019 via email

@syclik
Copy link
Member

syclik commented Jan 14, 2019 via email

@bgoodri
Copy link
Contributor

bgoodri commented Jan 14, 2019 via email

@wds15
Copy link
Contributor Author

wds15 commented Jan 14, 2019

I think that @bgoodri won't need to change the file again as his instructions are taken care of wrt to the __VA_ARGS__ macro use... as a last resort rstan can revert back to the file currently in use with rstan (which we cannot use in stan-math as it pulls in R specifics). Sounds like we are good now or anything else, @syclik ?

@wds15
Copy link
Contributor Author

wds15 commented Jan 27, 2019

Bump. Would be great if we can merge this or define what requirements are needed to get this in.

I haven't changed anything other than aligning with the new sundials 4.0.1 interface to their solvers. No change in behavior should be implied by this. Note that we are at sundials 3.1 and in the meantime there was 3.2 and now there is 4.0.1.

@syclik , @bob-carpenter , @seantalts ... thoughts?

@syclik syclik self-requested a review January 28, 2019 15:27
@syclik
Copy link
Member

syclik commented Jan 28, 2019

I spoke with @wds15.

I need to follow up with pystan developers to find out if this will build once merged.

@syclik
Copy link
Member

syclik commented Jan 28, 2019

@ariddell @ahartikainen: how is Sundials packaged for PyStan now? If we bump the version, does this pose a problem for PyStan?

@riddell-stan
Copy link

riddell-stan commented Jan 28, 2019 via email

@syclik
Copy link
Member

syclik commented Jan 28, 2019 via email

syclik
syclik reviewed Feb 1, 2019
View reviewed changes
make/libraries Outdated Show resolved Hide resolved
@syclik
Copy link
Member

syclik commented Feb 1, 2019

@wds15, I'm looking at it. So far, so good. Should we just go to v4.0.2 since we're upgrading?

syclik
syclik requested changes Feb 2, 2019
View reviewed changes
Copy link
Member

@syclik syclik left a comment

Choose a reason for hiding this comment

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

@wds15, everything looks good but we need to document the shell script so it's useful in the future. Just putting a script undocumented there isn't cutting down the maintenance burden much.

Otherwise, awesome!

lib/prepare-sundials.sh Outdated
@@ -0,0 +1,21 @@
#!/bin/bash

Copy link
Member

Choose a reason for hiding this comment

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

@wds15, could you document this script at a high-level? What would be extremely helpful:

  1. Why / when a maintainer would use this.
  2. What the maintainer needs to set up before using this.
  3. What variables the maintainer needs to set in the script.
  4. What the expected output is and how the maintainer is supposed to know this worked correctly.

Finally, we need to put this in the wiki. Probably here: Updating Libraries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a lib/sundials-config/README.md file which covers the above (except 4). Point 4 can be handled by our unit tests. If those pass then all is fine.

Copy link
Member

Choose a reason for hiding this comment

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

What's sundials-config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sundials-config contains all stan specific configuration items for Sundials. I introduced it in my last commit. It serves to separate out things.

lib/prepare-sundials.sh Outdated
rm -rf INSTALL_GUIDE.pdf config/ doc/ examples/ test/ */cvode */ida */arkode */kinsol
find . -name CMakeLists.txt -exec rm {} \;

find src -name "*.c" -type f -exec sed -E -i _orig 's#[^sf]printf\(#STAN_SUNDIALS_PRINTF(#'g {} \;
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we would actually commit to git the source directly from Sundials at this point prior to changing things. Even if it's just one commit followed by another, I think it'll help us back out changes if we mess up the process. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I would not do that as it complicates things. Moreover, git is super clever with these version changes and only tracks actual changes. Having those vanilla sundials -> adapted sundials commits in between bloats this.

Instead I am suggesting to have a sundials_version and a sundials-config directory. The content of the sundials_version is the stock sundials tar with our changes applied. The sundials-config contains all stan specifics (README.md, script, special stan include & sundials_config).

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I would not do that as it complicates things. Moreover, git is super clever with these version changes and only tracks actual changes. Having those vanilla sundials -> adapted sundials commits in between bloats this.

I have the opposite reaction when I know this.

I'm looking for easier review and maintenance. The overall result will be the same.

If we have one commit that's the pruned Sundials and the next that's the changes to Sundials, I find using the GitHub interface it's easy to tell that the second commit actually does what we intend. And if we make a mistake, it's easy to back out that one commit. If the two actions aren't separated in git, then we can't tell and it's more work in verifying that this is correct.

In fact, this would have helped with this current PR. Try thinking about the person reviewing. If I told you that I've updated a library and changed things, how would you verify it? My first thought was to download and do a diff, but I can't compare that to the final. If there's a better way to validate the PR, let me know. If not, let's add the additional git commit, which, as you say, git is clever and won't actually impact the user (but will positively impact the reviewer).

@syclik
Copy link
Member

syclik commented Feb 18, 2019

@wds15, did we avoid this in the past due to this change to make/libraries?

from develop:

  $(addprefix $(SUNDIALS)/src/cvodes/, cvodes.c cvodes_io.c cvodea.c cvodea_io.c cvodes_direct.c cvodes_diag.c cvodes_spils.c cvodes_bandpre.c cvodes_bbdpre.c)

from this PR:

 $(wildcard $(SUNDIALS)/src/cvodes/*.c)

Should we just revert this change?

@wds15
Copy link
Contributor Author

wds15 commented Feb 18, 2019

I think I did this one purpose, because some files were missing otherwise.

@wds15
Copy link
Contributor Author

wds15 commented Feb 19, 2019

Assuming the testing suite gives a green light this is good to go, I hope.

@wds15 wds15 changed the title upgrade sundials to 4.0.2 upgrade sundials to 4.1.0 Feb 19, 2019
@wds15
Copy link
Contributor Author

wds15 commented Feb 20, 2019 •
edited
Loading

Hmm... we now have an upstream CmdStan test failing with

mpicxx.openmpi -std=c++1y  -Wno-sign-compare    -Wno-delete-non-virtual-dtor -O3 -I src -I stan/src -I stan/lib/stan_math/ -I stan/lib/stan_math/lib/eigen_3.3.3 -I stan/lib/stan_math/lib/boost_1.66.0 -I stan/lib/stan_math/lib/sundials_4.1.0/include -I stan/lib/stan_math/lib/gtest_1.8.1/include -I stan/lib/stan_math/lib/gtest_1.8.1     -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION   -DSTAN_MPI -DGTEST_USE_OWN_TR1_TUPLE       -Wl,-L,"/home/jenkins-slave/jenkins-slave-files/workspace/CmdStan_downstream_tests/stan/lib/stan_math/lib/boost_1.66.0/stage/lib" -Wl,-rpath,"/home/jenkins-slave/jenkins-slave-files/workspace/CmdStan_downstream_tests/stan/lib/stan_math/lib/boost_1.66.0/stage/lib"   -include src/test/test-models/print_uninitialized.hpp src/cmdstan/main.cpp        stan/lib/stan_math/lib/sundials_4.1.0/lib/libsundials_nvecserial.a stan/lib/stan_math/lib/sundials_4.1.0/lib/libsundials_cvodes.a stan/lib/stan_math/lib/sundials_4.1.0/lib/libsundials_idas.a stan/lib/stan_math/lib/boost_1.66.0/stage/lib/libboost_serialization.so stan/lib/stan_math/lib/boost_1.66.0/stage/lib/libboost_mpi.so stan/lib/stan_math/stan/math/prim/arr/functor/mpi_cluster_inst.o -o src/test/test-models/print_uninitialized
stan/lib/stan_math/lib/sundials_4.1.0/lib/libsundials_cvodes.a: file not recognized: File format not recognized
collect2.exe: error: ld returned 1 exit status
make: *** [make/program:37: src/test/test-models/printer.exe] Error 1

I am not sure why this is popping up now nor where this is coming from. I will need to look further into this.

The thing is that a lot of tests before this one just pass and then out of the blue this pops up. If I figure out how to restart this downstream test, then I will try to "fix" it like this for the moment being - out of lack of other ideas right now.

@wds15
Copy link
Contributor Author

wds15 commented Feb 20, 2019

Ok... when restarting only the downstream job the pipeline succeeds. Thus, I am kicking Jenkins to do the full thing once more.

@wds15
Copy link
Contributor Author

wds15 commented Feb 20, 2019

Finally! Green... this is hopefully good to go in. @seantalts ... do you need anything to review this?

@seantalts
Copy link
Member

Which part(s) am I to review? Any tips for looking at just the stuff we changed vs. the vendored unchanged sundials library?

@wds15
Copy link
Contributor Author

wds15 commented Feb 20, 2019

I think @syclik was fine with this PR except the upgrade script. So maybe you try that out (grab the script and run it over a fresh branch off from develop)? @syclik did invest quite some work into this script, I have to say; all bits are automatic (unless sundials moves things around which makes our makefiles fail over).

The changes to stan-math sources are reviewed and the makefile changes were also waived, I think. So it's down to the script.

As a last sanity check you should execute the bdf+adams integrate ode tests.

@syclik
Copy link
Member

syclik commented Feb 21, 2019

@seantalts, I can approve it. I think it's clear that both @wds15 and I have looked carefully at the script and merges and are confident that it's readable.

If you have time and want to look carefully, this is the file to check: lib/upgrade-sundials.sh. https://github.com/stan-dev/math/pull/1098/files#diff-1ca3f358ea4ffad65560c3bf9575d698

I just reverted the changes to the makefile and Jenkinsfile so they're not part of this PR. With @wds15's last changes, this shouldn't be a problem.

syclik
syclik approved these changes Feb 21, 2019
View reviewed changes
Copy link
Member

@syclik syclik left a comment

Choose a reason for hiding this comment

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

Good to merge.

@wds15, if you see it change to green, go ahead and merge. (I removed the changes to Jenkinsfile and make/libraries that weren't necessary, so it's testing again now.)

@wds15 wds15 merged commit 2850ec2 into develop Feb 21, 2019
@wds15 wds15 mentioned this pull request Mar 5, 2019
Closed
@syclik syclik deleted the feature/issue-sundials-4 branch April 18, 2019 13:56
@rok-cesnovar rok-cesnovar mentioned this pull request Oct 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syclik syclik syclik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wds15 @seantalts @bob-carpenter @syclik @bgoodri @riddell-stan @ahartikainen