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

Fixing linking Sundials as an external library #2861

Merged
merged 4 commits into from
Feb 7, 2023
Merged

Fixing linking Sundials as an external library #2861

merged 4 commits into from
Feb 7, 2023

Conversation

syclik
Copy link
Member

@syclik syclik commented Jan 10, 2023 •
edited
Loading

Summary

This PR allows the user to link against a locally installed Sundials. To do this, their make/local should include 4 things:

  1. INC_SUNDIALS is what needs to be provided to the c++ compiler for includes. So it should look like -I <path-to-sundials-source>
  2. LDFLAGS_SUNDIALS should be the linker flags for sundials. This should look like -L <path-to-sundials-libraries>
  3. LDLIBS_SUNDIALS is the linker instruction to include sundials libraries. This should look like -lsundials_cvodes -lsundials_idas -lsundials_kinsol -lsundials_nvecserial (this works with LDFLAGS_SUNDIALS to pick the right libraries)
  4. SUNDIALS_TARGETS should be set to empty so the Stan Math library stops trying to build them.

Full example (using brew to install sundials):

INC_SUNDIALS = -I /opt/homebrew/include
LDFLAGS_SUNDIALS = -L /opt/homebrew/lib
LDLIBS_SUNDIALS = -lsundials_cvodes -lsundials_idas -lsundials_kinsol -lsundials_nvecserial
SUNDIALS_TARGETS =

Edit: changed LIBSUNDIALS to SUNDIALS_TARGETS. This is non-breaking... we could never set this from the outside anyway.

Tests

Our tests should pick up whether this works.

Side Effects

None. This should be backwards compatible and only allows for additional functionality.

Release notes

Allows Sundials to be linked to separately installed version.

Checklist

  • Copyright holder: Daniel Lee

    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)
    • dependencies checks pass, (make test-math-dependencies)
    • 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

@syclik
Copy link
Member Author

syclik commented Jan 10, 2023

@WardBrian, can you check to see if this works?

Also, where can I add more documentation for this? It'd be a shame if it just stayed in this PR.

@WardBrian
Copy link
Member

Re: documentation
CmdStan has a make/local.example file to document the use of make/local, we could do the same thing here?

For the rest of the PR:

I don't like how this is inconsistent with how we currently support external versions of TBB. We use different style names there, (e.g. TBB_INC, rather than INC_TBB), and you just need to specify

   TBB_INC=/path/to/include/
   TBB_LIB=/path/to/lib/

(e.g., without the -I or -L flags).

There is also (as far as I know) no reason to not have LDLIBS_SUNDIALS = -lsundials_cvodes -lsundials_idas -lsundials_kinsol -lsundials_nvecserial be the default? This will still work with the static .a things we build right now

@WardBrian
Copy link
Member

I can confirm that the changes do work, if I set up my make/local as described, I can build and run test/unit/rev/functor/algebra_solver_fp_test.cpp (it passes) and it is dynamically linked:

$ ldd test/unit/math/rev/functor/algebra_solver_fp_test 
        ...
        libsundials_cvodes.so.6 => /home/brian/miniconda3/envs/libtest/lib/libsundials_cvodes.so.6 (0x00007f1c8eec7000)
        libsundials_kinsol.so.6 => /home/brian/miniconda3/envs/libtest/lib/libsundials_kinsol.so.6 (0x00007f1c8ee8b000)

So it's just naming/convenience issues to resolve

@syclik
Copy link
Member Author

syclik commented Jan 10, 2023

Cool. I'll work on the doc inside CmdStan as you suggested.

Thanks for making comments; responses below.

I don't like how this is inconsistent with how we currently support external versions of TBB. We use different style names there, (e.g. TBB_INC, rather than INC_TBB)...

Heard, but I think the consistent thing to do is to go the other way to use INC_TBB instead of switch everything the other way.

The makefiles are currently inconsistent and INC_SUNDIALS already exists (along with INC_GTEST). TBB_INC is the only one with _INC as a suffix.

and you just need to specify

TBB_INC=/path/to/include/`
TBB_LIB=/path/to/lib/
(e.g., without the -I or -L flags).

I get it, but for Sundials this pattern won't work exactly. For an additional complication, do we want users to control which libraries to link against?

The usage suggested will also make the makefile more complicated (in ways that's not true with other libraries); let's take small steps first and get it working with the existing variables and keep simplifying as we go along. I haven't introduced any new variables with this PR.

(I'm going to leave TBB-related comments out of this PR thread, but I do think it deserves reworking.)

There is also (as far as I know) no reason to not have LDLIBS_SUNDIALS = -lsundials_cvodes -lsundials_idas -lsundials_kinsol -lsundials_nvecserial be the default? This will still work with the static .a things we build right now

Yes, that we should be able to do. I'd need to really double-check whether the linker commands are ok going that way.

There is a little disconnect that would exist between LIBSUNDIALS and LDLIBS_SUNDIALS that we'd have to contend with; let me think if there's a way to consolidate that somehow.

@syclik
Copy link
Member Author

syclik commented Jan 10, 2023

I'm looking at make/tests. Changing LDLIBS_SUNDIALS to be the default libraries would actually change the behavior. Here's how:

  • The current develop branch only links specific tests with Sundials. It's not always linked. It will only build the library if it's used.
  • By changing the default, we'd have to have the linking happening on all tests. Is this a problem? Probably not. It'd simplify a bunch of things. It might slow development slightly.

Is there a way to add the flags to LDFLAGS and LDLIBS conditionally? Yes, but it's a bit complicated and not sure it's worth the effort of adding something like that inside the makefile. I'd lean towards just always linking with Sundials if we wanted to go that way.

(I'd say we separate out changing behavior to a separate PR anyway.)

@WardBrian
Copy link
Member

I'm also happy if TBB naming changes, I just like if they match.

Re the second issue, I believe we'd be able to link conditionally still using a change similar to what @andrjohns did here for #2659.
I think it is possible to port the changes he made the sundials makefiles by only introducing 1 new make variable, called SUNDIALS_TARGETS, which splits the linking and building. There is one disadvantage, I believe, to making the default linker argument -lsundials... instead of just the paths to the .a files, which is I believe if a shared library exists in the user's $PATH and we also built our own, one of them will need to be selected by the linker when the -l flag is used. I think the shared library wins there.

@syclik
Copy link
Member Author

syclik commented Jan 19, 2023

Thanks for making the last set of comments; it helped me think about renaming LIBSUNDIALS to SUNDIALS_TARGETS so it's more consistent with the other libraries.

The conditional linking is a little harder to d, so I'll leave that for a different PR. Some things we could think about:

  • do we need conditional linking of Sundials? People have asked us for this on and off, but by the time it gets to any of the interfaces, it's always linked in. It helps a little with the linking speed, but it is a conditional build that we may or may not really need to worry about.
  • If we set the LDLIBS to always carry around the sundials libraries without having the libraries built, then we run into a linker error (at least on clang++). ld: library not found for -lsundials_cvodes. clang: error: linker command failed with exit code 1 (use -v to see invocation) So by the time we include the library in the linker flags, even if it's not being used, it has to exist.
  • Optional include of the Sundials linker arguments... we could do this in the Math library, but I think it complicates things donwstream. In the Math repo, we only need to link it against the tests that need it. We can add target-specific updates to those variables. But for CmdStan, it would need to take that step proactively and we'd have to put in a little additional logic downstream. (It's just a little bit of logic, but it's still something.)

@drezap
Copy link
Collaborator

drezap commented Jan 19, 2023 via email

@syclik
Copy link
Member Author

syclik commented Jan 19, 2023 via email

@WardBrian
Copy link
Member

But for CmdStan, it would need to take that step proactively and we'd have to put in a little additional logic downstream

Correct, I realized this same thing here. This is worth it, in my opinion, to avoid any user who wants to do this always having to paste the same thing into their make/local - if the flags will be the same every time (-lsundials_cvodes -lsundials_idas ...), we should find a way to make that automatic as much as possible IMO.

No reason that necessarily needs to be done in one PR, though. This is already an improvement over what we currently have

@syclik
Copy link
Member Author

syclik commented Jan 25, 2023

No reason that necessarily needs to be done in one PR, though. This is already an improvement over what we currently have

@WardBrian, that's exactly how I feel about this change. It's a step in the right direction and it'll allow us to get to the point where we can use the linker flags properly... hopefully soon.

@syclik syclik force-pushed the feature/libsundials branch from 6c540ff to c74667f Compare January 27, 2023 14:33
@WardBrian
Copy link
Member

The latest test run failed during clean up of the expression tests with error make/libraries:89: *** unterminated call to function 'wildcard': missing ')'. Stop., and it does indeed look like an extra ) was deleted in the diff

@syclik
Copy link
Member Author

syclik commented Feb 3, 2023

thanks. I'll track it down.

@syclik syclik force-pushed the feature/libsundials branch from c74667f to ab84dfd Compare February 3, 2023 20:18
@WardBrian
Copy link
Member

WardBrian commented Feb 6, 2023 •
edited
Loading

Is it expected that the downstream tests will fail? Do we need to also have a PR against stan or cmdstan? (I assume not, #2834 suggests it is possible to make similar changes and have all tests still passing)

@syclik
Copy link
Member Author

syclik commented Feb 6, 2023

No, it shouldn't need a downstream PR....

but let me check. It's possible that something was relying on one of the renamed variables. In which case, I'll do a safer PR that we can stage.

@syclik
Copy link
Member Author

syclik commented Feb 6, 2023

ok... found LIBSUNDIALS in used in multiple places. Will update this PR so it's safer.

Here's how it'll be staged:

  1. this PR gets updated so there are two variables: SUNDIALS_TARGETS is the variable we want. This gets copied to LIBSUNDIALS
  2. after that, this PR can get merged safely
  3. a PR on Stan and CmdStan and wherever necessary to replace the use of LIBSUNDIALS with SUNDIALS_TARGETS.
  4. a PR on Math to remove the LIBSUNDIALS variable

This won't change anything user-facing... users can't set LIBSUNDIALS right now (it gets overridden in the current build system). So these are all internal changes.

@syclik
Copy link
Member Author

syclik commented Feb 6, 2023

@WardBrian, the last commit allowed this to be used without having to change downstream repos. It was a one-liner. Mind taking a look whenever you get a chance? Thanks!

@WardBrian WardBrian mentioned this pull request Feb 6, 2023
Open
@syclik syclik merged commit 133fd01 into develop Feb 7, 2023
@syclik syclik deleted the feature/libsundials branch February 7, 2023 14:35
@syclik syclik mentioned this pull request Feb 7, 2023
Merged
3 tasks
This was referenced Mar 24, 2023
Merged
Merged
@WardBrian WardBrian mentioned this pull request Aug 28, 2023
Open
@andrjohns andrjohns mentioned this pull request Apr 26, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WardBrian WardBrian WardBrian 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.
3 participants
@syclik @WardBrian @drezap