DebugAccumulator (plus tiny bits and pieces) #976

penelopeysm · 2025-07-08T22:36:34Z

Closes #974.

My comments in review.

github-actions · 2025-07-08T22:37:59Z

Benchmark Report for Commit `40eddde`

Computer Information

Julia Version 1.11.6
Commit 9615af0f269 (2025-07-09 12:58 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results

|                 Model | Dimension |  AD Backend |      VarInfo Type | Linked | Eval Time / Ref Time | AD Time / Eval Time |
|-----------------------|-----------|-------------|-------------------|--------|----------------------|---------------------|
| Simple assume observe |         1 | forwarddiff |             typed |  false |                  8.3 |                 1.6 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                642.5 |                41.2 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                406.7 |                50.1 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1198.2 |                28.3 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               6039.1 |                24.8 |
|           Smorgasbord |       201 | reversediff |             typed |   true |               1438.7 |                27.8 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |                983.7 |                 4.4 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               5666.5 |                 4.0 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |                950.4 |                 9.0 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              63862.9 |                 3.5 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               8128.8 |                10.0 |
|               Dynamic |        10 |    mooncake |             typed |   true |                130.4 |                13.3 |
|              Submodel |         1 |    mooncake |             typed |   true |                 13.5 |                 6.0 |
|                   LDA |        12 | reversediff |             typed |   true |               1162.5 |                 2.7 |

github-actions · 2025-07-08T22:47:21Z

DynamicPPL.jl documentation for PR #976 is available at:
https://TuringLang.github.io/DynamicPPL.jl/previews/PR976/

codecov · 2025-07-08T23:07:38Z

Codecov Report

Attention: Patch coverage is 90.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.58%. Comparing base (2074657) to head (40eddde).

Files with missing lines	Patch %	Lines
src/debug_utils.jl	90.00%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           breaking     #976      +/-   ##
============================================
- Coverage     82.67%   82.58%   -0.09%     
============================================
  Files            38       38              
  Lines          4022     4007      -15     
============================================
- Hits           3325     3309      -16     
- Misses          697      698       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

penelopeysm

There are a couple, more general, things I wanted to ask your opinion on @mhauru:

Implementing this was the first time I worked with accs. It was largely a pleasure (I appreciated having good docstrings), but I got a bit confused between setaccs!! (replaces all the accumulators) and setacc!! (adds to the existing accumulators). Is there a way we could disambiguate?
My pipe dream for DynamicPPL's folder structure would be something like this:

$ tree
.
├── accs
│   ├── debug.jl
│   ├── interface.jl
│   ├── logprob.jl
│   └── values_as_in_model.jl
├── contexts
│   ├── conditionfix.jl
│   ├── interface.jl
│   └── prefix.jl
├── DynamicPPL.jl
├── model.jl
└── varinfo.jl

(Not all files included.) We kind of have something like this already in that accumulators.jl is what I call accs/interface.jl, and default_accumulators.jl is what I call accs/logprob.jl (modulo NumProduce), but I'd like to go one step further and use directories. I want to do the same with contexts, and I feel like perhaps you mentioned something similar for varinfos? Shall we maybe put this on the list as the last thing to do before releasing 0.37?

penelopeysm · 2025-07-08T22:40:15Z

HISTORY.md

 ## 0.37.0

-**Breaking changes**


0.37 is turning into a bit of a monster. I'm personally quite happy with this, even a little bit excited! 😄

But I think we might need to start treating it a bit more seriously. I started by separating the changelog into more public vs more internal changes. (For example, most people really don't need to care about accs; even if you're using something like values_as_in_model, you don't need to care about whether it was implemented using a context or an acc.)

Apart from this, maybe we should probably have a definition of done for 0.37 (ie which PRs/features do we want to get in for that release)? If you agree, then I'll start putting together a checklist on the breaking PR.

For me the only real question is, do we want the simplifications afforded by accumulators to be in v0.37 or not. Either all of them or most of them. I would like VariableOrderAccumulator, because that's kinda where this whole thing started, getting rid of num_produce. Haven't thought carefully about what else might be on the fence of whether it's in or out.

I have nothing that would be entirely unrelated to accumulators that I would like to put in v0.37.

Very happy with the improvements to the changelist.

In terms of accs, it's just VariableOrder left, isn't it? And we could keep it as a default acc in 0.37, and maybe later work out how to make it opt-in for PG only?

I think it might be just VariableOrder, though I haven't looked at this in a few weeks, so may forget something. We could keep it as default, but I would also consider leaving it out and immediately moving it Turing.jl once it's functional.

It seems that we've actually converged to PG on two fronts, one with VariableOrderAcc and removing num_produce, the other with removing SamplingContext and the del flag #982.

If you asked me to be opinionated: I wonder if it may be easier for us to leave all the PG-related stuff to 0.38, partly because 0.37 is getting very big, and partly so that we can compartmentalise PG and non-PG stuff?

I don't really mind v0.37 getting big, when we do PRs one by one, as long as it doesn't start to hold up releasing something of use. I guess it might make the integration work in Turing.jl more painful. Leaving things like removing code that is being added to Turing.jl for v0.38 would make sense, to have one release where it's all in place but not yet gone. Generally not bothered if you want to make an intermediate release. When you say "leave all the PG-related stuff to 0.38", would that include implementing VariableOrderAccumulator?

When you say "leave all the PG-related stuff to 0.38", would that include implementing VariableOrderAccumulator?

Yup.

src/accumulators.jl

src/debug_utils.jl

penelopeysm · 2025-07-08T23:48:56Z

src/accumulators.jl

-# When showing with text/plain, leave out information about the wrapper AccumulatorTuple.
-Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple) = show(io, mime, at.nt)
+# When showing with text/plain, leave out type information about the wrapper AccumulatorTuple.
+function Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple)
+    print(io, "AccumulatorTuple(")
+    show(io, mime, at.nt)
+    print(io, ")")
+    return nothing
+end


This is also opinionated. I like that you can (usually) copy the output of show, paste it into the REPL and have it generate the same object. Happy to revert if you disagree.

I think that should be the case for show with no MIME type specified, and this is in fact in Julia docs. However, I view MIME"text/plain" as a request to be more human-readable and pretty/slick at the expense of completeness and machine-parseability.

Although show with no MIME defaults to text/plain, doesn't it? So it seems like the same thing to me.
Part of the reason why I'd like to include this is e.g. when trying to debug (say, Enzyme issues) then it printing the same as a NamedTuple feels a bit misleading (I'd have to check typeof to realise that it is, in fact, not a NamedTuple). I'm not hugely opinionated because I will probably remember, but maybe it might help somebody down the line.

I'm not sure how the method cascade is implemented, but even if you define the MIME"text/plain" version, the plain call to show still uses the default implementation:

julia> struct Dada end julia> Base.show(io::IO, mime::MIME"text/plain", ::Dada) = print(io, "three arg text/plain") julia> Dada() three arg text/plain julia> show(Dada()) Dada() julia> display(Dada()) three arg text/plain julia> @show Dada(); Dada() = Dada()

Do stacktraces end up using the display/three arg show thing somewhere? Because if yes then I see your point about debugging, and it becomes a question of ease of debugging vs neatness of user-facing output. I was hoping the three arg MIME"text/plain" would only come into play in the REPL and if one calls display.

Stack traces would show the type so it would be alright there. And oh, okay, it seems that it's actually defined something like this:

display(x) = Base.show(stdout, MIME"text/plain"(), x) Base.show(io, ::MIME"text/plain", x) = Base.show(io, x) Base.show(x) = Base.show(stdout, x)

I think I still prefer the consistency of it always printing AccumulatorTuple. Maybe the problem is that I actually do use the user-facing output for debugging?

The problem I have with that is that when someone calls e.g. display(svi) I take that to mean "give me a human-readable, pretty, not-necessarily-exhaustive summary of what is in this SimpleVarInfo". In which case if it prints out

Transformed SimpleVarInfo((x = -1.0,), AccumulatorTuple((LogPrior = LogPriorAccumulator(0.0), LogLikelihood = LogLikelihoodAccumulator(0.0), NumProduce = NumProduceAccumulator(0))))

I find the word AccumulatorTuple to be unnecessary bloat that makes the output uglier and harder to read. If you care about AccumulatorTuple then I presume you also care about things like "is that an Int64 or Int32?" and you should use show(svi), which should give you all the details. Really what I would like for display(svi) to print out might be

Transformed SimpleVarInfo((x = -1.0,), (LogPrior = 0.0, LogLikelihood = 0.0, NumProduce = 0))

though I don't think I ever got to making a nice implementation that would do that.

mhauru

Happy with the gist of it, some localised comments.

One change that this brings is that if your context does something weird and e.g. fails to call accumulate_obssume!!, then nothing will be captured by the DebugAccumulator. Previously, since DebugContext arrested the call stack higher up, things like record_pre_tilde_assume! were being called no matter what. Not sure if this change is a pro or a con.

mhauru · 2025-07-09T12:24:33Z

HISTORY.md

 ## 0.37.0

-**Breaking changes**


For me the only real question is, do we want the simplifications afforded by accumulators to be in v0.37 or not. Either all of them or most of them. I would like VariableOrderAccumulator, because that's kinda where this whole thing started, getting rid of num_produce. Haven't thought carefully about what else might be on the fence of whether it's in or out.

I have nothing that would be entirely unrelated to accumulators that I would like to put in v0.37.

mhauru · 2025-07-09T12:28:26Z

HISTORY.md

 ## 0.37.0

-**Breaking changes**


Very happy with the improvements to the changelist.

src/accumulators.jl

mhauru · 2025-07-09T12:44:15Z

src/accumulators.jl

-# When showing with text/plain, leave out information about the wrapper AccumulatorTuple.
-Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple) = show(io, mime, at.nt)
+# When showing with text/plain, leave out type information about the wrapper AccumulatorTuple.
+function Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple)
+    print(io, "AccumulatorTuple(")
+    show(io, mime, at.nt)
+    print(io, ")")
+    return nothing
+end


I think that should be the case for show with no MIME type specified, and this is in fact in Julia docs. However, I view MIME"text/plain" as a request to be more human-readable and pretty/slick at the expense of completeness and machine-parseability.

src/debug_utils.jl

mhauru · 2025-07-09T13:37:55Z

src/simple_varinfo.jl

@@ -122,15 +122,15 @@ Evaluation in transformed space of course also works:

 ```jldoctest simplevarinfo-general
 julia> vi = DynamicPPL.settrans!!(SimpleVarInfo((x = -1.0,)), true)
-Transformed SimpleVarInfo((x = -1.0,), (LogPrior = LogPriorAccumulator(0.0), LogLikelihood = LogLikelihoodAccumulator(0.0), NumProduce = NumProduceAccumulator(0)))
+Transformed SimpleVarInfo((x = -1.0,), AccumulatorTuple((LogPrior = LogPriorAccumulator(0.0), LogLikelihood = LogLikelihoodAccumulator(0.0), NumProduce = NumProduceAccumulator(0))))


I think this was one of the reasons why I liked the simplified MIME"text/plain" show: To declutter printing out varinfo types.

test/debug_utils.jl

src/debug_utils.jl

mhauru · 2025-07-09T13:42:41Z

HISTORY.md

+You now need to explicitly pass a `VarInfo` argument to `check_model` and `check_model_and_trace`.
+Previously, these functions would generate a new VarInfo for you (using an optionally provided `rng`).


I'm confused, check_model signature still says

check_model(model::Model, varinfo::AbstractVarInfo=VarInfo(model); error_on_failure=false)

making varinfo optional. The keyword arguments I think have changed though.

Oh, that is true, I didn't realise that. Errrrr, I can't say I like having the VarInfo be optional. In evaluate!! it isn't optional, and check_model basically does evaluate!! with extra steps. I think I will make it mandatory, if that's alright. I don't think anyone uses this directly (it's mostly a pre-sampling thing).

I thought that it was semantically neat that to check a model you only needed to give a model, and then if you wanted to specify more about how that checking of a model is done, that was optional. Not too fussed about it though.

Yeahh, agreed. That's the main reason why I was a bit hesitant in my last comment. The single-argument version did require being restrictive in that it would always use SamplingContext though (there was no way to change this). I guess I'll keep it this way now (with both model and varinfo compulsory) but also keep it in mind as one of the areas where we're not fully sure about the best API.

mhauru · 2025-07-09T13:46:03Z

src/debug_utils.jl

-    record_pre_tilde_assume!(context, vn, right, vi)
-    value, vi = DynamicPPL.tilde_assume(childcontext(context), right, vn, vi)
-    record_post_tilde_assume!(context, vn, right, value, vi)


Was there a reason why pre and post were separate? Just wondering if we are losing something in effectively only having post.

I think the only thing that matters is the missing check.

Which doesn't matter any more because I removed the logp accumulators.

penelopeysm · 2025-07-09T17:30:05Z

One change that this brings is that if your context does something weird and e.g. fails to call accumulate_obssume!!, then nothing will be captured by the DebugAccumulator. Previously, since DebugContext arrested the call stack higher up, things like record_pre_tilde_assume! were being called no matter what. Not sure if this change is a pro or a con.

Indeed that's true. I think it depends on your view of what check_model is supposed to do. In my opinion, it's meant to catch models that will actually execute perfectly fine but will quietly give incorrect results. For example:

same varname is used multiple times -- logp will be meaningless.
NaN in data -- logp will be NaN.

That's kind of similar to the other functions in DebugUtils, e.g. model_warntype, which tells you about potential performance problems but is really intended for models that do already run, i.e. there's no point calling model_warntype if your model errors.

IMO I don't think it's its job to catch:

models that totally cannot be executed (e.g. vector of missing) -- that should be dealt with either at the location where the error is thrown, or using an error hint. I'm happy to add the missing check back since it was there before but I probably still want to implement an error hint for this on top of it.
incorrect implementations of contexts that don't call accumulate_obssume!!. That should be checked by some test_context function which contextualises the model with the context, and then calls evaluate!! with a varinfo that has a test accumulator, whose sole purpose is to confirm that accumulate_obssume!! has been called. I guess in fact DebugAcc could be used for that purpose.

mhauru · 2025-07-11T08:38:27Z

I got a bit confused between setaccs!! (replaces all the accumulators) and setacc!! (adds to the existing accumulators). Is there a way we could disambiguate?

Note that setacc!! also replaces an accumulator if one with the same name already exists. I'm open to suggestions for better names, if because of nothing else then just because how they differ by a single character makes them easy to misread. setacc!! to me seems like the obvious name for its purpose, because it's like setting an element in something like a dictionary. On the other hand, setaccs!! felt like the right thing because it sets the field called accs. But I agree that the end result is not the clearest.

My pipe dream for DynamicPPL's folder structure would be something like this:

I'd be happy with this sort of restructuring. My only change to your proposal would be that, depending on how varinfo source code gets structured, accs would somehow go under varinfo, since they are only ever used within varinfos.

mhauru · 2025-07-11T08:49:13Z

Indeed that's true. I think it depends on your view of what check_model is supposed to do. In my opinion, it's meant to catch models that will actually execute perfectly fine but will quietly give incorrect results.

Happy with that (and thus leaving out the missing and context checking).

penelopeysm · 2025-07-11T16:07:50Z

I'm a bit annoyed that there isn't a better way for setacc{,s}!!. Happy to leave for now since it seems like the sort of thing that will only bite me once, but if someone else complains then maybe we can revisit.

mhauru

Happy to consider this done except for the ongoing conversation about display and show.

src/debug_utils.jl

Co-authored-by: Markus Hauru <[email protected]>

penelopeysm · 2025-07-15T18:03:24Z

We agreed that I should use display only for pretty-printing and that for debugging I should use show. I reverted the change to the show method, so I think it should be ready for formal approval (fingers crossed).

penelopeysm added 2 commits July 8, 2025 23:20

DebugContext -> DebugAccumulator

28e5ba4

Changelog

88da7bd

github-actions bot assigned penelopeysm Jul 8, 2025

Force conditioned to return a dict

8c7aff9

penelopeysm added 2 commits July 8, 2025 23:48

fix conditioned implementation

a85f28d

revert conditioned bugfix (will merge this to main instead)

919cb25

penelopeysm commented Jul 8, 2025

View reviewed changes

penelopeysm requested a review from mhauru July 8, 2025 23:18

fix show

0649972

penelopeysm commented Jul 8, 2025

View reviewed changes

penelopeysm changed the title ~~DebugAccumulator~~ DebugAccumulator (plus tiny bits and pieces) Jul 8, 2025

penelopeysm added 2 commits July 9, 2025 01:49

Fix doctests

0ebb56e

fix doctests 2

e534434

penelopeysm mentioned this pull request Jul 9, 2025

Simplify the context mechanism in DynamicPPL #895

Open

mhauru requested changes Jul 9, 2025

View reviewed changes

penelopeysm added 2 commits July 9, 2025 17:56

Make VarInfo actually mandatory in check_model

d73bb14

Re-implement missing check

cd2f969

penelopeysm added 2 commits July 9, 2025 18:34

Revert combine signature in docstring

1f10b18

Merge branch 'breaking' into py/debug-accu

8a7fea8

This was referenced Jul 10, 2025

Release v0.37 #901

Draft

InitContext, part 5 - Remove SamplingContext, SampleFrom{Prior,Uniform}, {tilde_,}assume #985

Draft

penelopeysm requested a review from mhauru July 11, 2025 16:08

mhauru reviewed Jul 14, 2025

View reviewed changes

src/debug_utils.jl Show resolved Hide resolved

penelopeysm and others added 2 commits July 15, 2025 19:01

Revert changes to Base.show on AccumulatorTuple

0635607

Add TODO comment about VariableOrderAccumulator

40eddde

Co-authored-by: Markus Hauru <[email protected]>

penelopeysm requested a review from mhauru July 15, 2025 18:03

		You now need to explicitly pass a `VarInfo` argument to `check_model` and `check_model_and_trace`.
		Previously, these functions would generate a new VarInfo for you (using an optionally provided `rng`).

DebugAccumulator (plus tiny bits and pieces) #976

Are you sure you want to change the base?

DebugAccumulator (plus tiny bits and pieces) #976

Conversation

penelopeysm commented Jul 8, 2025

Uh oh!

github-actions bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark Report for Commit 40eddde

Computer Information

Benchmark Results

Uh oh!

github-actions bot commented Jul 8, 2025

Uh oh!

codecov bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

penelopeysm left a comment • 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!

penelopeysm Jul 8, 2025 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

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!

mhauru 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

penelopeysm Jul 9, 2025 • 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

github-actions bot commented Jul 8, 2025 •

edited

Loading

Benchmark Report for Commit `40eddde`

codecov bot commented Jul 8, 2025 •

edited

Loading

penelopeysm left a comment •

edited

Loading

penelopeysm Jul 8, 2025 •

edited

Loading

penelopeysm Jul 9, 2025 •

edited

Loading

penelopeysm commented Jul 9, 2025 •

edited

Loading

penelopeysm commented Jul 11, 2025 •

edited

Loading