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

Updated HMC implementation for new AHMC version #1660

Merged
merged 19 commits into from
Jul 17, 2021
Merged

Conversation

torfjelde
Copy link
Member

This PR makes Turing.jl compatible with the AdvancedHMC.jl's master branch, which will soon become a new release.

@yebai
Copy link
Member

yebai commented Jul 17, 2021

@torfjelde It seems that the CI time increased due to some recent PRs, which leads to more often timeouts. Maybe splitting the Turing-CI workflow into two workflows to resolve the time limits of a single workflow?

@torfjelde
Copy link
Member Author

@torfjelde It seems that the CI time increased due to some recent PRs, which leads to more often timeouts. Maybe splitting the Turing-CI workflow into two workflows to resolve the time limits of a single workflow?

Am fully aware; been trying to fix it for a couple of days now. I'm also not certain it's due to any of the recent PRs; it might be some other dependencies that slow things down.

@torfjelde
Copy link
Member Author

torfjelde commented Jul 17, 2021

E.g. integration tests in DPPL passed within an hour in our last PR-merge just 3 days ago: https://github.com/TuringLang/DynamicPPL.jl/actions/runs/1031412449. Then it suddenly started timing out without any changes made to master: https://github.com/TuringLang/DynamicPPL.jl/actions/runs/1034731795.

And Turing's full test-suite started timing out just 2 days ago: https://github.com/TuringLang/DynamicPPL.jl/actions/runs/1034731795

@torfjelde
Copy link
Member Author

torfjelde commented Jul 17, 2021

Uhhh I have no idea why but it seems like JuliaStats/Distributions.jl#1362 is causing the freeze. I noticed it would halt when testing

@model function gdemo3(x = 10 * ones(2))
# Multivariate `assume` and `observe`
m ~ MvNormal(length(x), 1.0)
x ~ MvNormal(m, 0.5 * ones(length(x)))
end

for ESS sampler. Upper-bounding Distributions.jl to 0.12.10 and everything runs as per usual, it seems.

@codecov
Copy link

codecov bot commented Jul 17, 2021

Codecov Report

Merging #1660 (041e620) into master (0168d84) will increase coverage by 11.06%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1660       +/-   ##
===========================================
+ Coverage   68.49%   79.56%   +11.06%     
===========================================
  Files          26       23        -3     
  Lines        1479     1414       -65     
===========================================
+ Hits         1013     1125      +112     
+ Misses        466      289      -177     
Impacted Files Coverage Δ
src/core/Core.jl 100.00% <ø> (ø)
src/stdlib/RandomMeasures.jl 34.28% <0.00%> (-3.46%) ⬇️
src/variational/VariationalInference.jl 53.84% <37.50%> (-35.52%) ⬇️
src/stdlib/distributions.jl 56.98% <58.13%> (+4.60%) ⬆️
src/core/container.jl 76.19% <76.19%> (-15.40%) ⬇️
src/variational/advi.jl 61.11% <76.92%> (-2.13%) ⬇️
src/inference/Inference.jl 84.55% <80.35%> (-6.79%) ⬇️
src/core/compat/reversediff.jl 95.34% <83.33%> (+8.86%) ⬆️
src/modes/ModeEstimation.jl 84.29% <84.74%> (+15.46%) ⬆️
src/inference/hmc.jl 76.33% <85.39%> (-8.54%) ⬇️
... and 26 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 d029198...041e620. Read the comment docs.

@yebai
Copy link
Member

yebai commented Jul 17, 2021

Uhhh I have no idea why but it seems like JuliaStats/Distributions.jl#1362 is causing the freeze. I noticed it would halt when testing

cc @devmotion

@yebai yebai merged commit 7cb94d6 into master Jul 17, 2021
@delete-merged-branch delete-merged-branch bot deleted the tor/advancedhmc-update branch July 17, 2021 16:53
@torfjelde
Copy link
Member Author

David is one vacation so not sure he'll respond 😕

Also, instead look at #1663 where I've added some more info.

Should we maybe upper-bound Turing.jl to Distributions@0.12.10 too, rather than just the tests? Worried peoples code might be running into similar scenarios (though I still don't understand why it fails).

@yebai
Copy link
Member

yebai commented Jul 17, 2021

Should we maybe upper-bound Turing.jl to Distributions@0.12.10 too, rather than just the tests? Worried peoples code might be running into similar scenarios (though I still don't understand why it fails).

Happy to upper-bound Distributions before we figure out why CI times out.

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.

2 participants