Update DynamicPPL.jl version #1726

torfjelde · 2021-11-09T17:27:13Z

This is a sibling-PR of TuringLang/DynamicPPL.jl#309.

The aim here is to make the bare-minimum changes to ensure that everything works as before. I've not made any significant attempts at making samplers compatible with immutable implementations of AbstractVarInfo, e.g. SimpleVarInfo, but rather just marked these lines with a # TODO: make compatible with immutable vi which we can then address in a separate PR.

Related: #1725.

EDIT: Tests are passing locally, but they won't pass here until the PR in DPPL has been merged and released.

@macroexpand

On master we have the following behavior for a test-case in Turing.jl: ```julia julia> @macroexpand @model empty_model() = begin x = 1; end quote function empty_model(__model__::DynamicPPL.Model, __varinfo__::DynamicPPL.AbstractVarInfo, __context__::DynamicPPL.AbstractContext; ) #= REPL[5]:1 =# begin #= REPL[5]:1 =# #= REPL[5]:1 =# return (x = 1, __varinfo__) end end begin $(Expr(:meta, :doc)) function empty_model(; ) #= REPL[5]:1 =# return (DynamicPPL.Model)(:empty_model, empty_model, NamedTuple(), NamedTuple()) end end end ``` Notice the `return` statement: it converted the statement `x = 1` which returns `1` into an attempt at a `NamedTuple{(:x, :__varinfo__)}`. On Julia 1.6 we don't really notice much of difference, because `first` and `last` will have the same behavior, but on Julia 1.3 the tests would fail in TuringLang/Turing.jl#1726 since "implicit" names in construction of `NamedTuple` isn't supported. This PR addresses this issue by simply capturing the return-value in separate variable, which is then combined with `__varinfo__` in a `Tuple` at the end. This should both fail and succeed whenever standard Julia code would.

torfjelde · 2021-12-15T16:46:15Z

src/inference/AdvancedSMC.jl

+        acclogp!!(vi, lp)
    end
-    return r, 0
+    return r, 0, _vi


Is this the right thing to do @yebai @devmotion ?

Essentially the issue is that when we're using PG with threads, _vi is a ThreadSafeVarInfo while vi = AdvancedPS.current_trace().f.varinfo is not. And so if we return vi instead of _vi, subsequent code in evaluate_threadsafe!! will fail.

I'm not too familiar with the AdvancedPS stuff so it's a bit unclear to me what I should be returning here, e.g. is the above okay?

Or should I overload evaluate!! and all the corresponding methods?

If we're doing this, we might want to consider making the "outer" calls, e.g. (::Model)(...) a bit more general, e.g. dispatch on AbstractProbabilisticProgram instead and then just overloading evaluate_threadsafe!! and evaluate_threadunsafe!! for TracedModel.

Suggested change

return r, 0, _vi

return r, 0, vi

Let's perhaps disallow the use of ThreadSafeVarInfo with PG/SMC for now. These samplers are not yet ready to run in threads parallelism due to the interaction of particles (i.e. the resampling step). I suggest that we simply throw an error here explaining that ThreadSafeVarInfo support for PG is to be supported in the near future.

Well, there currently isn't a way to allow/disallow specific combinations like this. And erroring or even just telling people not to use PG/SMC just because they are running with --threads=4 seems wrong since they might not even be using threading inside of the model.

We could add a isthreadsafe(varinfo, context) check in https://github.com/TuringLang/DynamicPPL.jl/blob/57c50f1f46aa5e51ec2125f3ae4883c61e59e9c3/src/model.jl#L391 and then implement this specifically for SamplingContext{<:PG}, etc.

We could add a isthreadsafe(varinfo, context) check in https://github.com/TuringLang/DynamicPPL.jl/blob/57c50f1f46aa5e51ec2125f3ae4883c61e59e9c3/src/model.jl#L391 and then implement this specifically for SamplingContext{<:PG}, etc.

Yes, I think this would be a better approach. Then we can always choose the non-threadsafe VarInfo for these samplers instead of erroring if they run Julia with multiple threads (e.g. I always do, so I think it would be very inconvenient to throw an error here).

The only possible problem is that then it would be possible to use @threads with a non-threadsafe VarInfo which can lead to incorrect result silently. I don't think there's a clean way to detect if a model tries to use multiple threads (since it is not as simple as detecting a @threads macro call). I wonder if it is possible to add a locking mechanism to the default non-threadsafe VarInfo that makes it threadsafe while not impacting performance if it is run with a single thread. Surely it would be slower if multiple threads are used but with multiple threads we would use the more performant ThreadSafeVarInfo anyway - if we don't use PG etc. The main motivation would be to avoid silent problems with multithreaded models when PG + a non-threadsafe VarInfo is used - it seems fine if it is less performant since it is not recommended.

Yes, I think this would be a better approach. Then we can always choose the non-threadsafe VarInfo for these samplers instead of erroring if they run Julia with multiple threads (e.g. I always do, so I think it would be very inconvenient to throw an error here).

I'll do that then 👍

The only possible problem is that then it would be possible to use @threads with a non-threadsafe VarInfo which can lead to incorrect result silently.

Yup 😕 We could potentially address this by using atomics (though it would probably require very recent Julia versions to be able to do this properly) which I expect would behave just like a Ref in the single-threaded case, or even just making the default VarInfo threadsafe by using an array instead of Ref for the logp field.

But it's worth mentioning that currently PG, etc. already silently does the wrong thing, doesn't it?

codecov · 2021-12-15T17:55:30Z

Codecov Report

Merging #1726 (4c1db31) into master (60b7d3f) will increase coverage by 0.12%.
The diff coverage is 84.84%.

@@            Coverage Diff             @@
##           master    #1726      +/-   ##
==========================================
+ Coverage   81.16%   81.29%   +0.12%     
==========================================
  Files          24       24              
  Lines        1476     1470       -6     
==========================================
- Hits         1198     1195       -3     
+ Misses        278      275       -3

Impacted Files	Coverage Δ
src/core/container.jl	`76.19% <0.00%> (ø)`
src/inference/Inference.jl	`84.55% <ø> (ø)`
src/inference/emcee.jl	`91.66% <66.66%> (+3.43%)`	⬆️
src/inference/hmc.jl	`76.36% <71.42%> (+0.03%)`	⬆️
src/modes/ModeEstimation.jl	`83.73% <84.61%> (ø)`
src/inference/mh.jl	`85.82% <86.66%> (+0.10%)`	⬆️
src/inference/ess.jl	`97.91% <90.00%> (-0.05%)`	⬇️
src/contrib/inference/dynamichmc.jl	`100.00% <100.00%> (ø)`
src/core/ad.jl	`81.01% <100.00%> (ø)`
src/inference/AdvancedSMC.jl	`97.43% <100.00%> (+0.02%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60b7d3f...4c1db31. Read the comment docs.

coveralls · 2021-12-15T17:55:30Z

Pull Request Test Coverage Report for Build 1588726417

84 of 99 (84.85%) changed or added relevant lines in 12 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 81.293%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/core/container.jl	0	1	0.0%
src/inference/emcee.jl	2	3	66.67%
src/inference/ess.jl	9	10	90.0%
src/inference/mh.jl	13	15	86.67%
src/modes/ModeEstimation.jl	22	26	84.62%
src/inference/hmc.jl	15	21	71.43%

Totals
Change from base Build 1566645174:	0.1%
Covered Lines:	1195
Relevant Lines:	1470

💛 - Coveralls

See related discussion: TuringLang/Turing.jl#1726 (comment).

…for SMC samplers

torfjelde · 2021-12-16T01:30:37Z

Tests are now passing 👍 This should now be ready for a review.

yebai

Looks good! To clarify, this PR only updates the DynamicPPL API, but does not make use of SimpleVarInfo yet, right?

torfjelde · 2021-12-16T09:49:44Z

Correct!

…

On Thu, Dec 16, 2021, 10:48 AM Hong Ge ***@***.***> wrote: ***@***.**** approved this pull request. Looks good! To clarify, this PR only updates the DynamicPPL API, but does not make use of SimpleVarInfo yet, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1726 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUPZZFG4HZSER2UE7IQEJ3URGYYBANCNFSM5HV3ICIA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

src/inference/hmc.jl

src/inference/emcee.jl

src/inference/hmc.jl

torfjelde · 2021-12-16T10:22:49Z

@devmotion you happy with this?

Btw, should we also re-export condition, decondition, and conditioned from Turing now?

devmotion · 2021-12-16T12:02:33Z

I'll have a look this afternoon 👍

I guess we don't have any prominent documentation yet and don't use it in any tutorials or examples? I think maybe we could add them and export these function in a single PR?

torfjelde · 2021-12-16T12:19:45Z

Bueno 👍

And yeah, that's a fair point. Next on my TODO list is writing a tutorial/some docs on the recent features, e.g. @submodel, condition, etc. and so I guess we can export it then. Though the only thing is that those PRs will be going to different projects than Turing itself. But anyhow, it should probably be a separate PR so let's leave it out of this one 👍

devmotion

Just had some minor questions, but looks good overall 👍 I guess most questions are related to the SimpleVarInfo integration which will be done in a separate PR.

devmotion · 2021-12-16T14:52:18Z

src/core/ad.jl

-        setlogp!(vi, ForwardDiff.value(logp))
+        # Don't need to capture the resulting `vi` since this is only
+        # needed if `vi` is mutable.
+        setlogp!!(vi, ForwardDiff.value(logp))


I guess ideally we would want a no-op here when setlogp!! returns a new object - if we just call setlogp!! with a SimpleVarInfo it would create a new VarInfo object even though it's never used. Maybe one could check if vi === new_vi and only call setlogp!! if it is true (assuming that in this case setlogp!! would not return a new object)?

But I guess this should be left for the SimpleVarInfo PR?

Will this not be optimized away in the case where vi is immutable?
I sort of assumed so. But I guess we might as well give the compiler a helping hand just to be sure.

devmotion · 2021-12-16T14:53:14Z

src/core/container.jl


 function AdvancedPS.reset_logprob!(f::TracedModel)
-    DynamicPPL.resetlogp!(f.varinfo)
+    DynamicPPL.resetlogp!!(f.varinfo)


This also relies on the fact that it mutates f.varinfo.

Yup, but TracedModel isn't compatible with immutable AbstractVarInfo 😕

devmotion · 2021-12-16T14:54:26Z

src/inference/AdvancedSMC.jl

+    resetlogp!!(vi)
+    empty!!(vi)


Does empty!! not call resetlogp!!? Maybe this would be reasonable? In any case, also here it is mandatory that the calls are actually mutating.

Similar to the calls below.

Same response as above: these samplers aren't compatible with immutable VarInfo 😕

devmotion · 2021-12-16T14:56:53Z

src/inference/emcee.jl

+    # Transform to unconstrained space.
+    DynamicPPL.link!.(vis, Ref(spl))


Why did you extract it? It seems there are no performance gains but rather now there are both a broadcasting and a map operation whereas before we just used a single map?

I extracted it because at first I intended to make it compatible with immutable AbstractVarInfo, but then I realized this shouldn't be done in this PR 🙃 I'll revert this change I guess 👍

torfjelde · 2021-12-16T15:32:48Z

Thanks! I'll merge once tests pass.

torfjelde added 5 commits November 9, 2021 13:37

updated deps and tests

852a754

initial work on updating the inference algs

e926b0b

removed failing and possibly incorrect test

181ec0d

switched to non-deprecated implementations

972334c

updated to use evaluate!! instead of evaluate

78e4b47

torfjelde mentioned this pull request Dec 4, 2021

Refactoring Glue Code #1735

Closed

This was referenced Dec 10, 2021

Bump DynamicPPL to 0.16 #1739

Merged

[WIP] VarInfo changes #1453

Closed

torfjelde added 3 commits December 14, 2021 22:52

bump DynamicPPL version

afb817f

Merge branch 'master' into tor/dynamicppl-update

08aa203

fixed some yet-to-be-renamed references

0af43d8

torfjelde mentioned this pull request Dec 15, 2021

[Merged by Bors] - Fixed a bug in replace returns TuringLang/DynamicPPL.jl#352

Closed

bump versions

26dc3d0

fix buggy ThreadSafeVarInfo for AdvancedPS samplers

e9d63d6

torfjelde commented Dec 15, 2021

View reviewed changes

torfjelde mentioned this pull request Dec 15, 2021

[Merged by Bors] - Add requires_threadsafe TuringLang/DynamicPPL.jl#353

Closed

bors bot pushed a commit to TuringLang/DynamicPPL.jl that referenced this pull request Dec 15, 2021

Add requires_threadsafe (#353)

1f8ac7e

See related discussion: TuringLang/Turing.jl#1726 (comment).

bors bot pushed a commit to TuringLang/DynamicPPL.jl that referenced this pull request Dec 15, 2021

Add requires_threadsafe (#353)

1b8e19b

See related discussion: TuringLang/Turing.jl#1726 (comment).

bumped DPPL compat entry and overload DynamicPPL.use_threadsafe_eval …

7fbe6eb

…for SMC samplers

yebai approved these changes Dec 16, 2021

View reviewed changes

torfjelde commented Dec 16, 2021

View reviewed changes

src/inference/hmc.jl Outdated Show resolved Hide resolved

torfjelde commented Dec 16, 2021

View reviewed changes

src/inference/emcee.jl Outdated Show resolved Hide resolved

torfjelde commented Dec 16, 2021

View reviewed changes

src/inference/hmc.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

cf7383c

devmotion reviewed Dec 16, 2021

View reviewed changes

revert change to Emcee step

024f83d

torfjelde mentioned this pull request Dec 16, 2021

Improve usage and support for immutable AbstractVarInfo #1752

Closed

devmotion approved these changes Dec 16, 2021

View reviewed changes

updated DynamicHMC integration

4c1db31

torfjelde merged commit 5c4b4d5 into master Dec 16, 2021

delete-merged-branch bot deleted the tor/dynamicppl-update branch December 16, 2021 19:39

		# Transform to unconstrained space.
		DynamicPPL.link!.(vis, Ref(spl))

Update DynamicPPL.jl version #1726

Update DynamicPPL.jl version #1726

Uh oh!

Conversation

torfjelde commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yebai Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coveralls commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1588726417

💛 - Coveralls

Uh oh!

torfjelde commented Dec 16, 2021

Uh oh!

yebai left a comment

Choose a reason for hiding this comment

Uh oh!

torfjelde commented Dec 16, 2021 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

torfjelde commented Dec 16, 2021

Uh oh!

devmotion commented Dec 16, 2021

Uh oh!

torfjelde commented Dec 16, 2021

Uh oh!

devmotion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

torfjelde commented Nov 9, 2021 •

edited

Loading

yebai Dec 15, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

coveralls commented Dec 15, 2021 •

edited

Loading