Use parallel standalone generated quantities #1256
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@SteveBronder your last commit here deleted a variable that seems to still be used elsewhere. It would be nice to get this into 2.35 if you can take a quick look |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
There was a problem hiding this comment.
- The argument parsing of the fitted_params filename expects the
_#suffix, which is a bit odd. I think the same argument name forsample output fileandgenerate_quantities fitted_paramsshould work - There is no check that the input filenames and the output filenames don't clobber, so you can overwrite your inputs and get an error about failing to read params
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
runCmdStanTests.py
Outdated
| @@ -130,7 +131,7 @@ def runTest(name, mpi=False, j=1): | |||
| executable = mungeName(name).replace('/',os.sep) | |||
| xml = mungeName(name).replace(WIN_SFX, '') | |||
| command = '%s --gtest_output="xml:%s.xml"' % (executable, xml) | |||
| if mpi: | |||
| if mpi is True: | |||
There was a problem hiding this comment.
This doesn't really matter as it's just a test harness but if x is True is not idiomatic in python, just if x
There was a problem hiding this comment.
Had a trickle down bug that caused this. Fixed now
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
| @@ -213,3 +213,16 @@ TEST(CommandHelper, check_filename_config_bad3) { | |||
|
|
|||
| EXPECT_THROW(check_file_config(parser), std::invalid_argument); | |||
| } | |||
|
|
|||
| TEST(CommandHelper, check_same_file_test) { | |||
There was a problem hiding this comment.
Thanks for the test. Adding at least one EXPECT_TRUE seems like a good idea to show the cases it does actually handle. This test would pass with a function that always returned false 😉
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@WardBrian I think I fix up the issues |
| if line.strip().startswith("#"): | ||
| continue | ||
| else: | ||
| stan_mpi = line.find('STAN_MPI=true') != -1 |
There was a problem hiding this comment.
[not sure if it's important] This won't accurately detect things like STAN_MPI=1, which also enable MPI
There was a problem hiding this comment.
It wasn't detecting that before. I think if we want to do that I'd rather do it in a different PR
There was a problem hiding this comment.
It was searching for STAN_MPI, so it would have. you've just traded false positives for false negatives here
There was a problem hiding this comment.
Oh I thought you meant an environment variable the user set. I can open another PR to just make this search for STAN_MPI
Submisison Checklist
./runCmdStanTests.py src/testSummary:
Note the line change # here is just from adding the test files. Theres about 100 code line changes in total
This swaps out the old
generated_quantitiesfunction call for the new parallel version.This is pretty straightforward besides the changes to
get_num_chains. For generated quantities, we don't have the user pass the number of chains, so we need to do some deduction to figure out how many chains the program will be running. We use similar logic to how we figure out the number of inits. If the user supplies just amy_file_name.csvthen we assume it's just 1 chain. But if a user suppliesmy_file_name_{id}.csvwe search formy_file_name_{{id} + 1, {id} _ 2,..., {id} + N}sequentially until we cannot find anymore files. The total number of files is the total number of chains.Intended Effect:
Allow users to call generated quantities in parallel.
How to Verify:
I only wrote one test in
generated_quantities_test.cppthat checks that the program does not error out. Is that sufficient?Side Effects:
Documentation:
Docs added for new file
file::exists()Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Simon's Foundation
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: