Add cmdstan #15203

maresb · 2021-06-06T12:31:55Z

Checklist

Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
License file is packaged (see here for an example).
Source is from official source.
Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
If static libraries are linked in, the license of the static library is packaged.
Build number is 0.
A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
When in trouble, please check our knowledge base documentation before pinging a team.

conda-forge-linter · 2021-06-06T12:32:09Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found it was in an excellent condition.

conda-forge-linter · 2021-06-30T13:31:07Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found some lint.

Here's what I've got...

For recipes/cmdstan:

Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [38, 39, 40, 41]

conda-forge-linter · 2021-06-30T13:55:03Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found it was in an excellent condition.

wolfv · 2021-07-09T08:36:06Z

@maresb it looks like tbbmalloc_proxy is compiled, but not installed?

x86_64-apple-darwin13.4.0-clang -fPIC -o libtbbmalloc.dylib backend.o large_objects.o backref.o  tbbmalloc.o  itt_notify_malloc.o frontend.o  -ldl -lpthread -dynamiclib -install_name @rpath/libtbbmalloc.dylib -stdlib=libc++ -m64 -L/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/system -L/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/ -mmacosx-version-min=10.9 -Wl,-L,"$SRC_DIR/stan/lib/stan_math/lib/tbb" -Wl,-rpath,"$SRC_DIR/stan/lib/stan_math/lib/tbb"   -Wl,-exported_symbols_list,tbbmalloc.def
x86_64-apple-darwin13.4.0-clang++ -fPIC -o libtbbmalloc_proxy.dylib proxy.o tbb_function_replacement.o  -ldl -lpthread libtbbmalloc.dylib -dynamiclib -install_name @rpath/libtbbmalloc_proxy.dylib -stdlib=libc++ -m64 -L/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/system -L/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/ -mmacosx-version-min=10.9 -Wl,-L,"$SRC_DIR/stan/lib/stan_math/lib/tbb" -Wl,-rpath,"$SRC_DIR/stan/lib/stan_math/lib/tbb"

which then results in an overlinking warning

wolfv · 2021-07-12T16:14:44Z

Can you check that the file is installed properly (copied into the prefix?)

WardBrian · 2021-07-12T17:45:17Z

Should it be copied into $prefix/lib? With the file structure or without?

Additionally, do you know why the OSX and Win tests seem to be taking orders of magnitude longer than the linux test? I've been unable to run anything other than the linux test locally, so I'm relying on the CI build

wolfv · 2021-07-12T19:27:49Z

If you link against it, you should install it (otherwise it's gonna be missing at execution time )

conda-forge-linter · 2021-07-13T14:18:13Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found some lint.

Here's what I've got...

For recipes/cmdstan:

The "test" section was expected to be a dictionary, but got a NoneType.
There are too few lines. There should be one empty line at the end of the file.
The "test" section was expected to be a dictionary, but got a NoneType.

conda-forge-linter · 2021-07-15T15:29:25Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found it was in an excellent condition.

wolfv · 2021-07-15T18:23:12Z

recipes/cmdstan/build.sh


 make clean-all

-ls $PREFIX/lib
-TBB_LIB=$PREFIX/lib make -j8 build
+make build -j4


it's better to use -j${CPU_COUNT}

maresb · 2021-07-26T23:58:09Z

Hi @WardBrian, sorry for the delay. I'm on the road so it's tough for me to give anything more than a reactionary response.

These C++ packages are really beyond my realm of expertise so take whatever I say with a grain of salt.

The license issues are indeed unfortunate, and I'm not so sure what to do about that.

Licenses aside, I think the big potential problems would be if those libraries would somehow lead to conflicts by overwriting existing libraries. Having a quick look at the artifacts, I see that tbb.dll is included in the bin/ directory on Windows. Could this be problematic?

@wolfv and/or @conda-forge/help-c-cpp, any suggestions?

WardBrian · 2021-07-27T00:02:43Z

My understanding of Windows linking issues means the tbb.dll could be a problem for other users. Luckily, tbb just released a new and not-backwards-compatible version which I believe uses a different filename for it's library files, so I think we could set it up so that you cannot install earlier tbbs (that would conflict with this one?). That's an open question I have for those more experienced. Nothing besides TBB ends up in the PATH, so I don't think there are any concerns besides that one

WardBrian · 2021-07-27T14:54:02Z

Re: the license issues - every included package is used within its license and has said license included with it in the subfolder (mainly in stan/lib/stan_math/lib) so I don't think it is a problem per se, we just may need to indicate this somehow in the metadata, and that's what I'm unsure about.

conda-forge-linter · 2021-07-27T14:59:03Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found some lint.

Here's what I've got...

For recipes/cmdstan:

The requirements section contained an unexpected subsection name. run-constrained is not a valid subsection name.

conda-forge-linter · 2021-07-27T15:01:52Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found it was in an excellent condition.

WardBrian · 2021-08-02T13:29:04Z

@conda-forge-admin, please ping team

conda-forge-linter · 2021-08-02T13:29:05Z

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

isuruf · 2021-08-02T13:31:50Z

recipes/cmdstan/meta.yaml

+    - {{ compiler('cxx') }}  # [not win]
+    - make                   # [not win]


Why do you need this in run?

CmdStan is sort of an odd duck - it is a tool for compiling models in the stan language, so it's one part library and one part build script. The actual usage of the tool at runtime requires Make and a c++ toolchain. The docs go into more detail, if you're curious, but essentially a .stan program is translated into a cpp file, which is then linked with the stan library and compiled to be run. CmdStan handles the first half of this, and provides a build script for the last portion, but it assumes you have a compiler to do so

isuruf · 2021-08-02T13:31:55Z

recipes/cmdstan/meta.yaml

+  build:  
+    - {{ compiler('cxx') }}  # [not win]
+    - make                   # [not win]
+    - tbb-devel =2020.2      # [not win]


Suggested change

- tbb-devel =2020.2 # [not win]

TBB has been an issue for us, so we ended up doing a sort of two-pronged approach: On Unix and MacOS, we depend on the conda-shipped TBB to make compilation simpler. On Windows, due to our dependence on mingw64's gcc, we have to compile our own tbb, hence this line.

We tried more or less every other combination in the above commits, but sadly this is what we found was required. The heavy use of templating in Stan's c++ code does not work under MSVC, so we need to take this somewhat odd route on Windows.

Oh, I think you just meant that this library is in both build and host. My bad, I believe that is unintentional. The above comment can just serve as historical record, then.

isuruf · 2021-08-02T13:32:36Z

recipes/cmdstan/meta.yaml

+    - {{ compiler('cxx') }}  # [not win]
+    - make                   # [not win]
+    - tbb-devel =2020.2      # [not win]
+    - m2w64-toolchain        # [win]


Use {{ compiler("m2w64_cxx") }}

conda-forge-linter · 2021-08-02T13:47:42Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found some lint.

Here's what I've got...

For recipes/cmdstan:

Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [20, 31]

conda-forge-linter · 2021-08-02T13:49:22Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/cmdstan) and found it was in an excellent condition.

WardBrian · 2021-08-02T16:15:03Z

Thank you for the comments @isuruf! We would appreciate any more review you could offer, or approval if it all looks good

isuruf · 2021-08-03T09:38:43Z

recipes/cmdstan/build.sh

+chmod +x "${PREFIX}/bin/cmdstan_model"
+
+# activate script
+echo "export CMDSTAN=${PREFIX}/bin/cmdstan" > "${RECIPE_DIR}/activate.sh"


Activate script should backup any existing CMDSTAN variable and deactivate should restore the backup. Btw, why is CMDSTAN needed?

CMDSTAN is used by the Python and R bindings (‘CmdStanPy’ and ‘CmdStanR’) to find CmdStan if it is not installed in the users home directory. We set it here so that this package is plug and play with those bindings, which we believe will be the majority of the install base based on current usage

I will make the change to set a temporary and restore it

CMDSTAN is now stored to CMDSTAN_OLD by activate, and restored to that value in deactivate

WardBrian · 2021-08-05T14:08:05Z

@wolfv or @isuruf we'd really appreciate if you took another look at this for review or merging. Thank you again for all your help so far!

isuruf · 2021-08-09T14:01:09Z

recipes/cmdstan/meta.yaml

+    - m2-sed                       # [win]
+    - m2-coreutils                 # [win]
+  host:
+    - tbb-devel =2020.2           # [not win]


To use the global pining

Suggested change

- tbb-devel =2020.2 # [not win]

- tbb-devel # [not win]

Will the global pinning ever change? CmdStan can be compiled with more recent versions of TBB but it requires extra flags set in the make command

You'll get a PR to cmdstan-feedstock when it changes.

Great, I've removed the pinning. Thanks for all the review! Happy to address anything else that stands out still

isuruf · 2021-08-09T14:01:20Z

recipes/cmdstan/meta.yaml

+  run:
+    - {{ compiler('cxx') }}        # [not win]
+    - make                         # [not win]
+    - tbb-devel =2020.2            # [not win]


Suggested change

- tbb-devel =2020.2 # [not win]

- tbb-devel # [not win]

isuruf

+1. Please ping when tests are green

maresb · 2021-08-09T14:46:12Z

@isuruf, thanks for the review, all green!

WardBrian · 2021-08-09T15:12:18Z

Just want to echo, thank you @isuruf and @wolfv!

maresb · 2021-08-09T15:16:43Z

Thank you @WardBrian, what an amazing effort!

maresb added 2 commits June 6, 2021 01:40

First attempt, causes segfault

595067e

Switch to cmdstanpy build script, still segfaults

bcec645

maresb added 7 commits June 6, 2021 16:18

Try switching to gxx

94d1801

Copy requirements from r-stan

21aa505

Remove potentially problematic debugging commands

07f2577

Try to set TBB_CXX_TYPE

49d0dac

Try gcc

6dca67f

STAN_MPI=false, add boost-cpp

f7f67ba

Add gcc compiler, and boost-cpp in host

37c5834

maresb mentioned this pull request Jun 28, 2021

FR: Support complete installation via conda stan-dev/cmdstanpy#395

Closed

Try simple build scripts instead

c6d31f7

Linting and another try

7562082

WardBrian added 3 commits June 30, 2021 10:09

Use mingw

a9b11f3

Attempt tbb as dep, no-reloc

436674d

Another pass

6998665

Try dso whitelist

6aa825b

Revert

7ecba76

Move tests, tbb at runtime

e33e639

Install tbb-devel

7c1e8a1

Try proper external tbb

52cfdba

wolfv reviewed Jul 15, 2021

View reviewed changes

Constrain tbb on windows to avoid dll issue

640cd39

Repair typo

a20d6fe

isuruf reviewed Aug 2, 2021

View reviewed changes

Change windows compiler

eb2dc34

Lint fix

b0cb635

Remove tbb from build reqs

1481c48

isuruf reviewed Aug 3, 2021

View reviewed changes

Store and restore CMDSTAN

2ba3c92

WardBrian mentioned this pull request Aug 4, 2021

Update install instructions on documentation stan-dev/cmdstanpy#420

Merged

2 tasks

isuruf reviewed Aug 9, 2021

View reviewed changes

Update tbb pinning

f7d2ce8

isuruf approved these changes Aug 9, 2021

View reviewed changes

wolfv merged commit e22706d into conda-forge:master Aug 9, 2021

Add cmdstan #15203

Add cmdstan #15203

Conversation

maresb commented Jun 6, 2021 • edited

conda-forge-linter commented Jun 6, 2021

conda-forge-linter commented Jun 30, 2021

conda-forge-linter commented Jun 30, 2021

wolfv commented Jul 9, 2021 • edited

wolfv commented Jul 12, 2021

WardBrian commented Jul 12, 2021

wolfv commented Jul 12, 2021

conda-forge-linter commented Jul 13, 2021

conda-forge-linter commented Jul 15, 2021

Choose a reason for hiding this comment

maresb commented Jul 26, 2021

WardBrian commented Jul 27, 2021 • edited

WardBrian commented Jul 27, 2021

conda-forge-linter commented Jul 27, 2021

conda-forge-linter commented Jul 27, 2021

WardBrian commented Aug 2, 2021

conda-forge-linter commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conda-forge-linter commented Aug 2, 2021

conda-forge-linter commented Aug 2, 2021

WardBrian commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WardBrian commented Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isuruf left a comment

Choose a reason for hiding this comment

maresb commented Aug 9, 2021

WardBrian commented Aug 9, 2021

maresb commented Aug 9, 2021

maresb commented Jun 6, 2021 •

edited

wolfv commented Jul 9, 2021 •

edited

WardBrian commented Jul 27, 2021 •

edited