Skip to content

Conversation

@devmotion
Copy link
Member

Reverts #1568 since now a version of Libtask_jll is available that is compatible with Julia 1.6 (segfaults caused by multithreading on Julia 1.5.4 require a different version of Libtask_jll which hopefully is available soon).

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

LGTM (as long as tests pass)!

Btw, you're the best @devmotion ❤️

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1577 (67d1d25) into master (b9db77c) will increase coverage by 4.71%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1577      +/-   ##
==========================================
+ Coverage   73.37%   78.08%   +4.71%     
==========================================
  Files          24       23       -1     
  Lines        1292     1424     +132     
==========================================
+ Hits          948     1112     +164     
+ Misses        344      312      -32     
Impacted Files Coverage Δ
src/variational/VariationalInference.jl 53.84% <0.00%> (-3.30%) ⬇️
src/Turing.jl 100.00% <0.00%> (ø)
src/core/Core.jl 100.00% <0.00%> (ø)
src/contrib/inference/dynamichmc.jl 100.00% <0.00%> (ø)
src/utilities/Utilities.jl
src/core/ad.jl 84.81% <0.00%> (+0.08%) ⬆️
src/contrib/inference/sghmc.jl 98.41% <0.00%> (+0.19%) ⬆️
src/inference/gibbs.jl 97.33% <0.00%> (+0.23%) ⬆️
src/inference/AdvancedSMC.jl 96.52% <0.00%> (+0.56%) ⬆️
src/inference/ess.jl 93.75% <0.00%> (+0.89%) ⬆️
... and 13 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 bffc8be...67d1d25. Read the comment docs.

@devmotion
Copy link
Member Author

I get the impression that some tests are take significantly more time to complete than what I am used to (e.g. 75 minutes for Turing CI whereas I thought it usually was 35-45 minutes). Also the integration tests in DynamicPPL timed out earlier today. Could it be that the compiler change in DynamicPPL introduced these regressions @torfjelde?

Also tests on Julia 1.3 seem slower, which would indicate that it is not caused by Julia 1.6.

@torfjelde
Copy link
Member

I get the impression that some tests are take significantly more time to complete than what I am used to (e.g. 75 minutes for Turing CI whereas I thought it usually was 35-45 minutes). Also the integration tests in DynamicPPL timed out earlier today. Could it be that the compiler change in DynamicPPL introduced these regressions @torfjelde?

Also tests on Julia 1.3 seem slower, which would indicate that it is not caused by Julia 1.6.

Hmm, maybe that change + APPL change really did have an impact, since now that check will occur at every assume-statement rather than just the observe statement and be compiled away (if possible).

How would be go about checking this properly? Revert the changes and run tests for that?

It does kind of suck if we were to do that though, as it would not make it possible to take the submodel-approach we discussed, since now args need to be known at compile-time 😕

@torfjelde
Copy link
Member

And you're right btw, it used to take 35mins a couple of PRs back 😕

@devmotion
Copy link
Member Author

devmotion commented Apr 8, 2021

How would be go about checking this properly? Revert the changes and run tests for that?

I think the best approach would be to create a new branch from the PR in DynamicPPL (the one that switches tests to Julia 1.6) and revert the compiler changes in this branch. And then run bors try in the branch and compare it with the PR that timed out twice now.

@devmotion
Copy link
Member Author

I don't think the APPL change causes these regressions, it should rather improve type inference.

@torfjelde
Copy link
Member

I don't think the APPL change causes these regressions, it should rather improve type inference.

But improving type-inference will put more pressure on the compiler, right? When it doesn't know the type, it will leave all the checks for observe to be dealt with at runtime rather than trying to compile them away.

Even so, I am worried (and think you're right here) that it's the fact that we're performing this check at every ~ now which causes the insane slow-down. I'll make a PR as you said and try.

@torfjelde
Copy link
Member

#1578

@torfjelde torfjelde merged commit 2eab753 into master Apr 8, 2021
@torfjelde
Copy link
Member

Still waiting for 1.6 over at #1578, but notice that 1.3 tests over there are only a couple of minutes slower.

Also, why is 1.6 slower than 1.3? o.O

@devmotion
Copy link
Member Author

Also, why is 1.6 slower than 1.3? o.O

Maybe Libtask became slower? Or there are some regressions in 1.6? Did you observe any differences in your experiments?

@devmotion
Copy link
Member Author

Or some AD backend is slower on 1.6?

@yebai yebai deleted the dw/julia16 branch December 16, 2021 20:34
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