Cmdstanr: simplify/generalize model compilation options

[mitzimorris]

Is your feature request related to a problem? Please describe.

CmdStanModel constructor and compile method take a series of named arguments for the CmdStan makefile variables, currently:

• arg threads for Makefile var STAN_THREADS
• arg opencl for stanc3 compiler arg --use-opencl
• args opencl_device_id. for Makefile vars OPENCL_DEVICE_ID
• ditto opencl_platform_id

We need to minimize dependencies between CmdStanX wrappers and the Stanc3 compiler and CmdStan makefiles.

Describe the solution you'd like

Implement a more general organization of user-facing compiler options instead of having many specific named options which will:

• Differentiate between arguments for options for bin/stanc vs. options to calls to make at the user-facing API.
• Provide getter/setter methods for compiler and makefile options. These options should be object-specific, therefore, don't rewrite user's make/local file.
• Check bin/stanc compiler options, because compiler throws an error on unrecognized options
• Pass make options into compiler unchecked
• disallow Stanc3 compiler debug flags - debugging the compiler is outside of the scope of the CmdStanX wrappers.

Additional context

We need an extensible solution that provides similar user-facing API for CmdStanPy and CmdStanR and future CmdStanX wrappers. If the Stanc3 compiler introduces new arguments, then the CmdStanX wrappers need to change accordingly but otherwise, the CmdStanX wrappers should just work with new CmdStan releases.

[rok-cesnovar]

I am not completely sure how this generalization would work, I would still like to keep a simplified API of enable stuff like threads, opencl and potentially mpi if possible.

Something like this?

cmdstan_model(file,
                           make_options = list(stan_opencl = TRUE, opencl_device_id = 0),
                           stanc_flags = list(allow_undefined = TRUE, ... )

For the stanc flags I have started a PR awhile ago and it uses the second approach.

• don't rewrite user's make/local file

I agree, I have started a branch to rework this, but have focused on other stuff with the current backlog of PRs on cmdstanr. So my approach I am working on is to use the flags with make. Basically call

make STAN_OPENCL=true model_name for example

What would be nice is to enable having the same model with different compiler options (stanc or make). My initial idea on which I havent started to work yet so dont know if it falls short anywhere, is to make a hash of all the make options and stanc flags. If the user supplies no options, then the executable is model_name.exe, otherwise its model_name_compileHash.exe. We also need to handle the main.o in this case for make flags (if someone adds stan_threads, stan_opencl, or march=native we need to recompile main.o also).

If we dont add that, this will keep recompiling on each run. And I would like to avoid that.

a <- cmdstan_model(file_name,
                           make_options = list(some_flag = TRUE))
b <- cmdstan_model(file_name)

A minor correction:

• arg opencl for stanc3 compiler arg --use-opencl

opencl sets the STAN_OPENCL makefile option and cmdstan makefiles take care of the stanc3 flag.

[mitzimorris]

What would be nice is to enable having the same model with different compiler options (stanc or make). My initial idea on which I havent started to work yet so dont know if it falls short anywhere, is to make a hash of all the make options and stanc flags. If the user supplies no options, then the executable is model_name.exe, otherwise its model_name_compileHash.exe. We also need to handle the main.o in this case for make flags (if someone adds stan_threads, stan_opencl, or march=native we need to recompile main.o also).

how often are users likely to do this?
why not let users rename exe files that they want to save with names that are meaningful to them (i.e. not hashcode)?

because you can instantiate a CmdStanModel object for an exe_file without specifying a stan_file, and such objects don't let you recompile, which is what you'd want in this situation.
does this make sense?

[mitzimorris]

Something like this?

cmdstan_model(file,
                           make_options = list(stan_opencl = TRUE, opencl_device_id = 0),
                           stanc_flags = list(allow_undefined = TRUE, ... )

yes!
agreed that we should have good argument names and check them for the user.

[mitzimorris]

parallel naming - after discussion with Bob, suggestion is stanc_options and cpp_options
@jgabry?

[rok-cesnovar]

how often are users likely to do this?

I think this is a theoretical issue everytime, even for

cmdstan_model(file)

if there is already an executable present.

How does cmdstanr know that the existing executable was not compiled with some different flags but the model was not touched? Make wont recompile it if no files were touched.

Or do we just tell the users to use force_recompile = TRUE in the case of changes to the make file? We should then also have an option to force_recompile_all that would also delete main.o if there was a changed for a flag that also requires recompiling that file.

That is why I resorted to saving make compile options to make/local because that way we at least knew if the make options were changed since the last time we compiled the model. It had other deficiencies for which I think we should not use it.

I know that this is probably not an everyday problem as users tend to set their flags and forget them. And that the make options are probably more in the domain of "advanced user". But we are bound to have more non-advanced users facing flags like the pedantic mode, there is already the allow_undefined, etc.

The options I see that we have for this are:

• ignore this and tell the users to use force_recompile flags and be aware that they have to do this for the first time after the change of flags. That is probably the most "thin wrapper" thing to do. Though its not very user friendly.
• make the executable with a hash suffix. We calculate the hash based on the set flags and if there is am executable with the matching suffix we then check the modified time of stan file and exe file to know whether to recompile or not
• make a cmdstanX model compile options file that would specify with which flags the current executable was compiled. model_name.cso (cso = cmdstan options)

[mitzimorris]

Or do we just tell the users to use force_recompile = TRUE in the case of changes to the make file? We should then also have an option to force_recompile_all that would also delete main.o if there was a changed for a flag that also requires recompiling that file.

yes, tell users to use force_recompile = TRUE. add logic to which checks if option also requires recompile of main.o, because how do users know which options require recompile of main.o?

trying to keep things as simple as possible, both for users and maintaining code base.

[rok-cesnovar]

Ok, I am fine with that.

Could we possibly have a model_name argument that would enable having 2+ executables from the same model name?

yes, tell users to use force_recompile = TRUE. add logic to which checks if option also requires recompile of main.o, because how do users know which options require recompile of main.o?

Agree.

Will add what was agreed upon to #102 if @jgabry agrees with stanc_options and cpp_options for names.

[rok-cesnovar]

@bob-carpenter change in which make flags or options require a recompilation of the main.o file in cmdstan ?

I know that STAN_MPI, STAN_THREADS and STAN_OPENCL definitely require that, not sure which of the others also require that.

Should main.o be recompiled each time there is a change in the make options?

[mitzimorris]

Could we possibly have a model_name argument that would enable having 2+ executables from the same model name?

where would this be used? when you create output file names for sample, etc commands?
or? just not seeing the use case.

[mitzimorris]

adding a model_name argument to CmdStanModel constructor is no problem - this would allow you to specify model name.

optional argument, if unspecified, will derive from basename of .stan file or model exe file. @rok-cesnovar is this what you're thinking?

[rok-cesnovar]

Something like

m1 <- cmdstan_model(file,
make_options = list(stan_opencl = TRUE, opencl_device_id = 0), model_name = "model_something")

m2 <- cmdstan_model(file, model_name = "model")

[rok-cesnovar]

I have thought about this a bit more and have another suggestion. Sorry to flip-flop so much on this.

The short version is: make cpp_options part of cmdstan (use for all models until stated otherwise) and handle stanc_options model specific.

I think this also best resembles how they are used in cmdstan. At least how I see it. I do tend to think more as a developer then a user so please do comment. But if my thinking is right, this also best fits the thin-wrapper idea.

Users mostly set their make/local flags and leave them as is. Except those that work with MPI/threading occasionally. Those set the flags, run make clean and compile the models. Then comment out the flags, make-clean again and proceed as before.

I also think that it would be very difficult to handle when to recompile the main.o file and when to not.

So I propose we have a set_cmdstan_cpp_options(options = list()) function.
That would store the options to a internal variable like .cmdstanr$CPP_OPTIONS. If the options match CPP_OPTIONS do nothing, just continue. If it doesnt, rebuild main.o. That is the most clean solution. Also write a message to the user that the models need to be recompiled in order for the changes to have an effect.

But otherwise m2 <- cmdstan_model(file, model_name = "model") after set_cmdstan_cpp_options wont necessarily recompile. I would keep the model_name argument also.

Thoughts?

[mitzimorris]

I like this - for Python this totally makes sense. Jonah and I have been talking about what to hide and what to expose.
on the stanc3 compiler:

• 'allow_undefined' , 'include_paths' will be recognized and respected
• 'warn-uninitialized' is always on

for the makefile args there's only these:

• 'STAN_OPENCL',
• 'OPENCL_DEVICE_ID',
• 'OPENCL_PLATFORM_ID',
• 'STAN_MPI',
• 'STAN_THREADS',

for the cpp flags there's: threads (TBB) and GPU (OpenCL) and for the latter, the device and platform IDs - the question is do we use names like TBB-threads and OPENCL-GPUs` and then translate them to the correct MAKEFILE var names.

[rok-cesnovar]

the question is do we use names like TBB-threads and OPENCL-GPUs` and then translate them to the correct MAKEFILE var names.

I think it will be easier to maintain the documentation and everything if we use the same names used in cmdstan. The only exception is that we promote to all caps automatically. Meaning stan_threads = TRUE is used as STAN_THREADS=TRUE.

So this would work:

cpp_options = list(stan_threads = TRUE,
                             cxxflags = " -march-native",
                             "CXXFLAGS += -O2",
)

set_cmdstan_cpp_options(cpp_options)

Does that look good? So we respect any possible make flags. This would all work in R and its easy to implement. If you both confirm this with @jgabry I will update the open PR.

[mitzimorris]

I think it will be easier to maintain the documentation and everything if we use the same names used in cmdstan. The only exception is that we promote to all caps automatically. Meaning stan_threads = TRUE is used as STAN_THREADS=TRUE.

So this would work:

cpp_options = list(stan_threads = TRUE,
cxxflags = " -march-native",
"CXXFLAGS += -O2",
)

set_cmdstan_cpp_options(cpp_options)

taking a closer look - how does "CXXFLAGS += -O2" fit into this proposal?

[rok-cesnovar]

Ah, never mind that. I will make a PR tomorrow to go into more detail and have some examples to work with. I think this is pretty close.

[mitzimorris]

@rok-cesnovar - @bbbales2 and I did some checking - on Linux we compiled the Stan threaded map_rect example with -STANC_THREADS and linked to a main.o compiled without and it worked.
I looked at the symbol tables of main.o compiled with/without flag STANC_THREADS and it contained the same set of tbb functions names. so I don't understand what the issue with STANC_THREADS as a compiler flag is. it seems to me that if there's a problem, you need to recompile more than just main.cpp - TBB_LIBS as well?

[mitzimorris]

regarding 'CXXFLAGS += -O2' - I don't think we should let users play with optimization levels at all - optimization level is set in file stan_math/make/compiler_flags : O ?= 3
the only reason to set it lower is to speed up compilation times, but since 2.21, compilation times are much faster, so we're good.

[rok-cesnovar]

Below is an example that fails on my Ubuntu with the following error:

--- Compiling, linking C++ code ---
g++ -std=c++1y -pthread -D_REENTRANT -Wno-sign-compare     -DSTAN_THREADS -I stan/lib/stan_math/lib/tbb_2019_U8/include -O3 -I src -I stan/src -I lib/rapidjson_1.1.0/ -I stan/lib/stan_math/ -I stan/lib/stan_math/lib/eigen_3.3.3 -I stan/lib/stan_math/lib/boost_1.72.0 -I stan/lib/stan_math/lib/sundials_5.1.0/include    -DBOOST_DISABLE_ASSERTS      -c  -x c++ -o examples/bernoulli/bernoulli.o examples/bernoulli/bernoulli.hpp
g++ -std=c++1y -pthread -D_REENTRANT -Wno-sign-compare     -DSTAN_THREADS -I stan/lib/stan_math/lib/tbb_2019_U8/include -O3 -I src -I stan/src -I lib/rapidjson_1.1.0/ -I stan/lib/stan_math/ -I stan/lib/stan_math/lib/eigen_3.3.3 -I stan/lib/stan_math/lib/boost_1.72.0 -I stan/lib/stan_math/lib/sundials_5.1.0/include    -DBOOST_DISABLE_ASSERTS            -Wl,-L,"/home/rok/Desktop/cmdstan/stan/lib/stan_math/lib/tbb" -Wl,-rpath,"/home/rok/Desktop/cmdstan/stan/lib/stan_math/lib/tbb"  examples/bernoulli/bernoulli.o src/cmdstan/main.o         stan/lib/stan_math/lib/sundials_5.1.0/lib/libsundials_nvecserial.a stan/lib/stan_math/lib/sundials_5.1.0/lib/libsundials_cvodes.a stan/lib/stan_math/lib/sundials_5.1.0/lib/libsundials_idas.a stan/lib/stan_math/lib/sundials_5.1.0/lib/libsundials_kinsol.a  stan/lib/stan_math/lib/tbb/libtbb.so.2 -o examples/bernoulli/bernoulli
/usr/bin/ld: _ZN4stan4math22AutodiffStackSingletonINS0_4variENS0_15chainable_allocEE9instance_E: TLS definition in examples/bernoulli/bernoulli.o section .tbss._ZN4stan4math22AutodiffStackSingletonINS0_4variENS0_15chainable_allocEE9instance_E[_ZN4stan4math22AutodiffStackSingletonINS0_4variENS0_15chainable_allocEE9instance_E] mismatches non-TLS reference in src/cmdstan/main.o
/usr/bin/ld: src/cmdstan/main.o: error adding symbols: bad value
collect2: error: ld returned 1 exit status
make: *** [make/program:45: examples/bernoulli/bernoulli] Error 1

# move the old make/local to make a clean example
mv make/local make/local_backup
make clean-all
make build -j4
# make a model without stan threads to make sure everything is built
make examples/bernoulli/bernoulli
# remove just the bernoulli files
rm -f examples/bernoulli/bernoulli examples/bernoulli/bernoulli.o examples/bernoulli/bernoulli.d examples/bernoulli/bernoulli.hpp

# now introduce threads
echo "STAN_THREADS=true" > make/local
# uncomment the next line to make it work
# rm src/cmdstan/main.o
make examples/bernoulli/bernoulli

If I uncomment the second to last line above (remove main.o) it compiles without error.

I opened a cmdstan issue on this topic awhile ago, but havent gotten around on making a proposal for a fix. see https://github.com/stan-dev/cmdstan/issues/757

I don't think we should let users play with optimization levels at all

I am not sure why would anyone want to do that either. The only thing that came up in some support questions is that O2 reduces the amount of RAM used for compiling. Not sure tho. It was just the first example for the CXXFLAGS I came up with. I am however not sure we want to go down the rabbit hole of checking flags a users add to CXXFLAGS.

Anyhow, I am going to work on a prototype. I think we are close to a final proposal here and can fine tune one an actual example.

[mitzimorris]

I opened a cmdstan issue on this topic awhile ago, but havent gotten around on making a proposal for a fix. see stan-dev/cmdstan#757

great - thanks - I'll take a look at that issue. assuming we can find a CmdStan makefile fix, this won't be a worry for CmdStanPy, correct?

[rok-cesnovar]

Not sure it will help for cmdstanX interfaces because the fix for #757 will most likely be adding make/local as a dependency to main.o so it will recompile on change to main.o. cmdstanX interfaces wont rely on make/local.