-
Notifications
You must be signed in to change notification settings - Fork 228
Improve error message for initialization failures with troubleshootin… #2637
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: main
Are you sure you want to change the base?
Conversation
Turing.jl documentation for PR #2637 is available at: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2637 +/- ##
=======================================
Coverage 84.88% 84.88%
=======================================
Files 22 22
Lines 1475 1475
=======================================
Hits 1252 1252
Misses 223 223 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks Aoife, I left a few suggestions for the code.
For the test failures, none of them seem related to this PR, although one of them I would like to understand. Anything that has pre
in the name is running on the upcoming release candidate version of Julia, v1.12-rc1. They may keep failing, and we should pay attention because we'll eventually have to fix those failures, but they are not a blocker for merging since v1.12 isn't out yet. The Inference
tests running on v1 (e.g. latest stable release of Julia) seem to fail because coveralls is bugging out, hopefully will be fixed be rerunning. The one that troubles me is Inference
tests running on min
, i.e. the oldest version of Julia we claim to support, because that one segfaults. I've seen a couple of segfaults in tests over the last few weeks that have been fixed be rerunning, I'm starting to suspect that we have some nasty indeterministic test somewhere, probably to do with one of the AD backends.
Let's see what happens with the tests if we just rerun them. Regardless, they seem clearly unrelated to this PR.
src/mcmc/hmc.jl
Outdated
@@ -168,7 +168,7 @@ function find_initial_params( | |||
|
|||
# if we failed to find valid initial parameters, error | |||
return error( | |||
"failed to find valid initial parameters in $(max_attempts) tries. This may indicate an error with the model or AD backend; please open an issue at https://github.com/TuringLang/Turing.jl/issues", | |||
"failed to find valid initial parameters in $(max_attempts) tries. This may indicate an error with the model or AD backend. See https://turinglang.org/docs/usage/troubleshooting/#initial-parameters for common causes and solutions. If the issue persists, please open an issue at https://github.com/TuringLang/Turing.jl/issues", |
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.
Maybe the sentence "This may indicate an error with the model or AD backend" can go? The message is quite long, and I think the link gives a more nuanced explanation of what might be wrong.
test/mcmc/hmc.jl
Outdated
err = nothing | ||
try | ||
sample(failing_model(), NUTS(), 10; progress=false) | ||
catch e | ||
err = e | ||
end | ||
|
||
@test err isa ErrorException | ||
@test contains( | ||
string(err), | ||
"https://turinglang.org/docs/usage/troubleshooting/#initial-parameters", | ||
) | ||
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.
This works, but check out Test.@test_throws
, which can do this for for you.
test/mcmc/hmc.jl
Outdated
μ ~ Uniform(-π, π) | ||
κ ~ InverseGamma(2, 3) | ||
return x ~ VonMises(μ, κ) |
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.
This model might get fixed at some point, independently of us (the issue is in AD packages/Distributions.jl). Would something like any super simple model but with @addlogprob!!(-Inf)
thrown in at the end trigger the same error more reliably? @addlogprob!!
is a thing you can call within a model to add any number you want to the log likelihood, and I think @addlogprob!!(-Inf)
should make all samples be rejected. @addlogprob!!(NaN)
might work too.
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.
line 208 through 215 already contains the same test.
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.
with @addlogprob! -Inf
…t information and enhance clarity
Pull Request Test Coverage Report for Build 16832126241Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
Looks good. One final nit about a simplification to the tests.
test/mcmc/hmc.jl
Outdated
# Verify the error message contains the troubleshooting link | ||
err = try | ||
sample(failing_model(), NUTS(), 10; progress=false) | ||
catch e | ||
e | ||
end | ||
|
||
@test contains( | ||
string(err), | ||
"https://turinglang.org/docs/usage/troubleshooting/#initial-parameters", | ||
) |
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.
This, too, can in fact be done with @test_throws
, since its first argument can be a string that must be contained in the error message. E.g.
julia> f() = throw(ErrorException("this is an error message that contains the phrase 'this must be in the error'"))
f (generic function with 1 method)
julia> @test_throws "this must be in the error" f()
Test Passed
Message: "this is an error message that contains the phrase 'this must be in the error'"
I'm not sure if you can check for both the substring and the exception type in one call to @test_throws
. There may be a way, or you may need to call @test_throws
twice.
@@ -168,7 +168,7 @@ function find_initial_params( | |||
|
|||
# if we failed to find valid initial parameters, error | |||
return error( | |||
"failed to find valid initial parameters in $(max_attempts) tries. This may indicate an error with the model or AD backend; please open an issue at https://github.com/TuringLang/Turing.jl/issues", | |||
"failed to find valid initial parameters in $(max_attempts) tries. See https://turinglang.org/docs/usage/troubleshooting/#initial-parameters for common causes and solutions. If the issue persists, please open an issue at https://github.com/TuringLang/Turing.jl/issues", |
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.
Can we set up a more robust URI for this link, e.g. via https://turinglang.org/docs/uri/initial-parameters
? This URI redirects to the corresponding docs link, and would never change if we restructure the docs.
Quarto supports this via redirects
: https://quarto.org/docs/websites/website-navigation.html#redirects
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.
Sure, let me add a redirects template in docs repo so we can easily create permanent redirects for docs.
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.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…g link