Skip to content
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

Added check_model and sub-module DebugUtils #540

Merged
merged 48 commits into from
Apr 19, 2024
Merged

Conversation

torfjelde
Copy link
Member

As we're getting more users coming from other languages, who are necessarily not so familiar with Julia, it's becoming more and more important that we avoid the end-user hitting any potentially confusing snags of Turing.jl / DynamicPPL.jl.

Examples are:

  • Usage of missing and differences between the different ways of conditioning variables.
  • Repeated variables will silently lead to incorrect behavior.
  • Etc.

Because of this, I was wondering if it might be a good idea to add some utilities for doing exactly this.

This PR adds a method check_model (which is also exported) which executes the model using a special context (DebugUtils.DebugContext) and performs some checks before, throughout, and after evaluation of the model. See the added tests for some examples.

Moreover, once we improve the documentation (I'm going to add a section on "Syntax"), we can then link to those in the warnings/errors to point the user to where they can figure stuff out.

Here's an example:

julia> using DynamicPPL, Distributions

julia> model = DynamicPPL.TestUtils.DEMO_MODELS[1];

julia> issuccess, (trace, _) = check_model(model);

julia> issuccess
true

julia> trace
3-element Vector{DynamicPPL.DebugUtils.Stmt}:
  assume: VarName[s[1], s[2]] = [5.58585, 0.73498] .~ InverseGamma{Float64}(invd=Gamma{Float64}=2.0, θ=0.333333), θ=3.0)  [5.58585, 0.73498] (logprob = -4.46134)
  assume: VarName[m[1], m[2]] = [0.264206, 0.781137] .~ Normal{Float64}[Normal{Float64}=0.0, σ=2.36344), Normal{Float64}=0.0, σ=0.85731)]  [0.264206, 0.781137] (logprob = -2.96538)
 observe: [1.5, 2.0] ~ DiagNormal=[0.264206, 0.781137], Σ=[5.58585 0.0; 0.0 0.73498]) (logprob = -3.6914)

julia> trace[1].varname
2-element Vector{VarName{:s, Setfield.IndexLens{Tuple{Int64}}}}:
 s[1]
 s[2]

Thoughts?

As an additional thing, though I'm uncertain if we want this or not, I've added the possibility of saving the tilde-statements as we're going through the model + potentially saving the varinfo after every statement. This can be quite useful for both us debugging "remotely" (as in, just ask the user to run the model with this and give us the output) + it makes it quite easy to perform either visual or programmatic checks of the "trace" of a model, which I think will be quite handy.

Btw, throughout this I've discovered a few bugs, which have subsequently been fixed:) If we decide against something like this, then I'll make separate PRs for this.

single record_tilde! + support for dot tilde + return issuccess and
additional info in check_model
convenient show methods to make displaying the trace nicer
de-conditioning is restricted to univariate distributions
SamplingContext by default since we're using an empty VarInfo by default
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

JuliaFormatter

test/debug_utils.jl|59|
test/debug_utils.jl|61|
test/debug_utils.jl|73|
test/debug_utils.jl|81|
test/debug_utils.jl|85|
test/debug_utils.jl|90|

src/debug_utils.jl Outdated Show resolved Hide resolved
src/debug_utils.jl Outdated Show resolved Hide resolved
src/debug_utils.jl Outdated Show resolved Hide resolved
src/debug_utils.jl Outdated Show resolved Hide resolved
src/debug_utils.jl Outdated Show resolved Hide resolved
src/debug_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
test/debug_utils.jl Outdated Show resolved Hide resolved
test/debug_utils.jl Outdated Show resolved Hide resolved
test/debug_utils.jl Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Sep 23, 2023

Pull Request Test Coverage Report for Build 8750716198

Details

  • 109 of 220 (49.55%) changed or added relevant lines in 5 files are covered.
  • 17 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-2.9%) to 78.921%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/test_utils.jl 0 5 0.0%
src/utils.jl 1 7 14.29%
src/debug_utils.jl 102 202 50.5%
Files with Coverage Reduction New Missed Lines %
src/model.jl 1 89.22%
src/threadsafe.jl 16 48.25%
Totals Coverage Status
Change from base Build 8745687397: -2.9%
Covered Lines: 2707
Relevant Lines: 3430

💛 - Coveralls

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Attention: Patch coverage is 49.08257% with 111 lines in your changes are missing coverage. Please review.

Project coverage is 79.16%. Comparing base (816e962) to head (8c47fad).
Report is 3 commits behind head on master.

❗ Current head 8c47fad differs from pull request most recent head c1aa5ea. Consider uploading reports for the commit c1aa5ea to get more accurate results

Files Patch % Lines
src/debug_utils.jl 50.00% 100 Missing ⚠️
src/utils.jl 14.28% 6 Missing ⚠️
src/test_utils.jl 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   83.65%   79.16%   -4.50%     
==========================================
  Files          28       26       -2     
  Lines        3219     3197      -22     
==========================================
- Hits         2693     2531     -162     
- Misses        526      666     +140     

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

@torfjelde
Copy link
Member Author

I went the check_model_and_trace route for now

test/debug_utils.jl Outdated Show resolved Hide resolved
src/debug_utils.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@torfjelde
Copy link
Member Author

Any thoughts on the implementation @devmotion ? I realize I haven't overloaded show much, so am uncertain if what I've done is a good idea or not.

@devmotion
Copy link
Member

I'd say the main rules are to avoid type piracy, to absolutely never define show methods for Types, and otherwise follow the conventions in the docs (https://docs.julialang.org/en/v1/manual/types/#man-custom-pretty-printing; main points: two argument show method is a compact one-line representation whereas show(io::IO, ::MIME"text/plain", x) is used for longer descriptions and will also be used by display(x) in the REPL).

@torfjelde
Copy link
Member Author

But then it seems like my current approach is good?

@torfjelde
Copy link
Member Author

torfjelde commented Apr 19, 2024

Btw @yebai you have any thoughts on this?

Another aspect I think would be useful with this is to extract the trace from the model a few times to determine if the model is "static" (in some sense) or not. We could then warn the user if, say, they're running HMC on a model which has a non-static number of parameters.

EDIT: Added a has_static_constraints method. This is also related to TuringLang/Turing.jl#2195.

EDIT 2: Note that after this PR we can immediately improve the user-experience by just adding a check_model=true to Turing.sample and then run DynamicPPL.check_model(model) before we hit inference.

`has_static_constraints` method to empirically check whether the model
has static constraints or if they are indeed changing dependent on realizations
@torfjelde
Copy link
Member Author

This is pretty much ready to go:)

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to have some debugging tools!

@yebai yebai enabled auto-merge April 19, 2024 18:48
@yebai yebai added this pull request to the merge queue Apr 19, 2024
Merged via the queue into master with commit 824f712 Apr 19, 2024
11 of 12 checks passed
@yebai yebai deleted the torfjelde/model-check branch April 19, 2024 19:21
@yebai
Copy link
Member

yebai commented Jun 12, 2024

We can likely introduce some checks performed after each MCMC step in the future, which can help catch additional issues like changing model dimensionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants