-
Notifications
You must be signed in to change notification settings - Fork 37
Enable opting out of TSVI #1128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: py/fastldf
Are you sure you want to change the base?
Conversation
Benchmark Report for Commit 9a74692Computer InformationBenchmark Results |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## py/fastinit #1128 +/- ##
===============================================
+ Coverage 81.72% 81.73% +0.01%
===============================================
Files 41 41
Lines 3956 3959 +3
===============================================
+ Hits 3233 3236 +3
Misses 723 723 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| """ | ||
| use_threadsafe_eval(context::AbstractContext, varinfo::AbstractVarInfo) | ||
| Return `true` if evaluation of a model using `context` and `varinfo` should | ||
| wrap `varinfo` in `ThreadSafeVarInfo`, i.e. threadsafe evaluation, and `false` otherwise. | ||
| """ | ||
| function use_threadsafe_eval(context::AbstractContext, varinfo::AbstractVarInfo) | ||
| return Threads.nthreads() > 1 | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, Turing overloads this function, so releasing this in a patch would cause upstream breakage. I don't feel bad about this though because this function is not exported.
On top of that, the method that Turing defines for this is bogus. It silently disables TSVI for PG/SMC, because Libtask can't handle it, and it generally doesn't work for SMC anyway because observations for all particles need to be in step. The problem is that for models that do need threadsafe eval, this will lead to silent wrong results (TuringLang/Turing.jl#2658).
The correct solution for this should be to emit an error saying:
PG/SMC can't run with models that require threadsafe eval. If you know your model doesn't need it, then you should call
DynamicPPL.use_threadsafe_eval!(false).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that FastLDF is going into breaking instead, we should just rebase this on breaking.
|
DynamicPPL.jl documentation for PR #1128 is available at: |
62c6351 to
9a74692
Compare
699aa23 to
c35dff5
Compare
Performance gains in #1113, #1125, etc. are only available when not using TSVI.
TSVI gets used more than it should (#1086).
This PR provides a mechanism to opt out of TSVI, in a way that is fully backwards-compatible.