STAN_THREADS in make/local not respected due to capitalisation conflict

[jaburgoyne]

Describe the bug

If threading is enabled globally by way of STAN_THREADS=true in make/local (as opposed to locally via cpp_options = list(stan_threads = TRUE) in a call to model$compile()), cmdstanr does not respect the setting. Although model$cpp_options is TRUE for the key STAN_THREADS, assert_valid_threads() only checks for stan_threads in lowercase.

Although threading is perhaps the most common use case, the same bug would apply to STAN_OPENCL and any other C++ option. To fix the bug, CmdStanModel needs to standardise on all-caps or lowercase for its internal cpp_options representation. It seems to me that lowercase would be the most consistent (and thus model_compile_info() should use tolower() instead of toupper() here), but in case this has further-reaching consequences than I can see, I’m making an issue instead of a pull request.

To Reproduce

Set STAN_THREADS=true in make/local, compile a model, and then try to run model$sample() with threads_per_chain set to some integer greater than 1.

Expected behavior

The model should sample using multi-threading instead of complaining (erroneously) that the model has not been compiled with threading enabled.

Operating system

Although this should be platform-agnostic, I am running macOS 13.3.1 on an Apple M1 architecture.

CmdStanR version number

2.32.1

Additional context

None

[katrinabrock]

Hello,

I hit this one as well with open_cl. I did not even set STAN_THREADS or STAN_OPENCL globally. The steps were simply

• create and compile a model with cpp_options = list(stan_opencl = TRUE)
• create a new R object from the model using the exe_file option without recompiling
• try to sample

(If these steps are not clear enough, I'd be happy to write up an MRE with code.)

Sampling will fail with the erroneous claim that the model was not compiled with stan_opencl = true. I think @jaburgoyne has correctly identified the root cause. I believe switching the assertions to check the uppercase version of the options would solve the issue without side effect.

@jgabry would you accept a PR to resolve this issue? If so, would you like to switch the checks to uppercase, model_compile_info setting to lowercase, or something else?

@jaburgoyne (and anyone else who is having this problem), I was able to workaround this issue with this ugliness:

mod$.__enclos_env__$private$cpp_options_$stan_opencl <- TRUE

[katrinabrock]

I went ahead and created a failing testcase for this issue: master...katrinabrock:cmdstanr:issue-765 (Just for the opencl part. We could create similar cases for the stan_threads part.)

[jgabry]

Thanks @jaburgoyne for reporting this. And thanks @katrinabrock for doing more digging and creating the failing test case. A PR would be great, thank you!

It seems to me that lowercase would be the most consistent (and thus model_compile_info() should use tolower() instead of toupper() here), but in case this has further-reaching consequences than I can see, I’m making an issue instead of a pull request.

---

This seems ok to me, but maybe @andrjohns has a different preference or can think of some issue with making this change.

[katrinabrock]

The opencl tests seem to be not working in general. Some are failing, and some, I believe, are passing but not checking the thing they are trying to check. Is there a reason these tests are not included in CI? (I understand why you would not want them to run on CRAN, but is there a reason not to run them in the github actions?)

I've identified two issues so far:

• This issue is related to the issue we are discussing in this thread. It appears that when a model object is created from a stan file and a binary already exists, it does not re-compile even if the build-related args that the binary was compiled with are different from the ones provided in creating the new model object. (At least for the opencl arg, it's not checking.) I'm not sure if this behavior is intentional in general (maybe documented somewhere?). As a result, within the opencl test script, the model that's being run within a given tests was not not necessarily compiled with the args specified in the test. Therefore, the tests aren't testing what they're intending to.
• The tests are looking for a metadata attribute opencl_ids_name that I can't find referenced anywhere else in the code either in the current version or in the version where this test was introduced: https://github.com/stan-dev/cmdstanr/blob/master/tests/testthat/test-opencl.R#L85

There are possibly other issues with this test script. I think before (or maybe just in parallel to) addressing the issue mentioned in this thread, it would be good to get that test script to a state where tests are passing and actually testing the existing functionality. Maybe I should create a separate issue for this effort?

[katrinabrock]

Made a PR to fix the tests: #1022

I'd also be happy to help if there are steps needed to add this to CI e.g. find/configure a docker container that can run this.