Feature/reduce mem pathfinder #3325
Conversation
…ager and large chunks of memory
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: |
…pathfinder' into feature/reduce-mem-pathfinder
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: |
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
force-pushed
the
feature/reduce-mem-pathfinder
branch
from
06f2a3f to
9a518d3
Compare
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: |
|
Latest push seems to be passing all my tests in tinystan |
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: |
|
Odd, cmdstan tests are passing locally for me. Going to run this one more time and then will investigate if that same failure pops up again |
|
Also odd, this test is running locally for me even with threading on |
|
@serban-nicusor-toptal it looks like Jenkins is hanging when trying to spin up the windows unit job. Can you took a look? Edit: nvm! |
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: |
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: |
| @@ -63,7 +57,8 @@ struct concurrent_writer { | |||
| // Max capacity of queue | |||
| std::size_t max_capacity{1000 + max_threads}; | |||
| // Threshold where the writing threads will wait for the queues to empty | |||
| std::size_t wait_threshold{max_capacity - (max_threads * 2)}; | |||
| std::size_t wait_threshold{ | |||
| (max_threads > 1000) ? 0 : (max_capacity - (max_threads * 2))}; | |||
There was a problem hiding this comment.
I think we actually need this value to always be at least one, right? Otherwise we won't be able to send any messages at all
| namespace stan::callbacks { | ||
| #ifdef STAN_THREADS | ||
| /** | ||
| * Takes a writer and makes it thread safe via multiple queues. |
There was a problem hiding this comment.
optional:
| * Takes a writer and makes it thread safe via multiple queues. | |
| * Enables thread-safe writing of numeric values to a writer |
I think this comment is when the class was trying to be more general
| auto constrain_fun = [](auto&& constrained_draws, auto&& unconstrained_draws, | ||
| auto&& model, auto&& rng) { | ||
| model.write_array(rng, unconstrained_draws, constrained_draws); | ||
| return constrained_draws; | ||
| }; |
There was a problem hiding this comment.
optional: this definition could be moved way down (I think to ~line 268), which would help with clarity a little IMO
| template <typename RNG, typename LPFun, typename AlphaVec, | ||
| typename CurrentParams, typename CurrentGrads, typename GradMat, | ||
| typename ParamMat, typename Logger> |
There was a problem hiding this comment.
The docstring of this function needs some cleanup of the @tparams
There was a problem hiding this comment.
I fixed it in the @return but forgot to put it in the actual tparam. Fixed
| "Multi pathfinder assumes we use a tee writer here, if you " | ||
| "intend to change this " | ||
| "please make it clear why."); |
There was a problem hiding this comment.
Optional: Should this have an "if you are a user please report a bug" type ending?
There was a problem hiding this comment.
No idt so. This assert can only happen at compile time so someone would have to change the C++
There was a problem hiding this comment.
Noting, but optional: Might be nice to add a github actions job which runs some tests with -DSTAN_THREADS and a non-1 number of threads. We can make this and issue rather than do it here, IMO
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: |
Submission Checklist
closes #3323
closes #3322
./runTests.py src/test/unitmake cpplintSummary
This removes the large memory footprint for writing to csv for pathfinder. So large models should be nicer to the system memory. This may decrease performance a bit, but the memory savings for large models will be significant so I think it is worth it.
When running single pathfinder from pathfinder we now just return the pathfinder at the best elbo estimates instead of generating all of the constrained parameters within single pathfinder. The constrained parameters are now generated on the fly before we send them to the parameter writer.
For pathfinder that does not do PSIS resampling we write to the parameter writer from within each single pathfinders thread. To avoid contention when writing these we use a
mutexbefore doing a bulk write for each pathfinder instance. I thought about putting each pathfinders results to a seperate file and then combining them at the end. But at the end of the day I think this would still have the same problems as multiple writes to a single file. We only have one system and it can only handle so many writes at the same time. So idt the effect of splitting the writes to multiple files would be noticable for saving time. One possible idea would be to have a thread that is busy spinning while checking a multi producer single consumer queue of values to write to the parameter writer.Intended Effect
Have pathfinder use less memory when saving parameters to the writer
How to Verify
All previous tests pass
Side Effects
Reduce memory usage
Documentation
Added docs for function that generates parameters and writes them.
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): Simons Foundation
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: