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

Remove CTask #123

Merged
merged 2 commits into from
Feb 20, 2022
Merged

Remove CTask #123

merged 2 commits into from
Feb 20, 2022

Conversation

rikhuijzer
Copy link
Contributor

@rikhuijzer rikhuijzer commented Feb 20, 2022

Dropping CTask fully in response to #121 (comment). CTask is the name of the old implementation. The new name is TapedTask. Also, this PR changes the type from TapedTask to TapedTask{F}.

This PR is related to

I'll be doing some performance tests before marking this PR ready for review.

@rikhuijzer
Copy link
Contributor Author

I'm having trouble benchmarking this against Turing because it seems that the current state of master of AdvancedPS is not compatible with Turing.

I did benchmark this PR against AdvancedPS.jl. It doesn't matter so much it seems.

Libtask.jl#master and AdvancedPS.jl#master

$ julia --project

julia> using Pkg

julia> @time Pkg.test()
[...]
 33.438645 seconds (5.31 M allocations: 326.480 MiB, 0.60% gc time, 10.18% compilation time)

Libtask.jl#rh/tapedtask and AdvancedPS.jl#rh/tapedtask

$ julia --project

julia> using Pkg

julia> @time Pkg.test()
[...]
 33.002745 seconds (5.33 M allocations: 328.248 MiB, 0.59% gc time, 10.20% compilation time)

Let's move towards merging this and continue improving performance once all packages moved over to this new Libtask version?

@rikhuijzer rikhuijzer marked this pull request as ready for review February 20, 2022 10:05
@rikhuijzer rikhuijzer requested a review from devmotion February 20, 2022 10:05
@yebai
Copy link
Member

yebai commented Feb 20, 2022

@rikhuijzer The most recent release of AdvancedPS is based on the releases branch. The master branch contains some breaking changes which are not compatible with Turing yet. So we need to

  1. update both the releases and master branches of AdvavncedPS,
  2. make a new release from the release branch
  3. then consider removing CTask from Libtask

@rikhuijzer
Copy link
Contributor Author

rikhuijzer commented Feb 20, 2022

For what it's worth, I've managed to benchmark this against a subset of the Turing testsets:

Libtask.jl#master, AdvancedPS#0.3.3 and Turing.jl#rh/tapedtask

inference/AdvancedSMC.jl 65.428537 seconds (124.91 M allocations: 7.790 GiB, 3.29% gc time, 96.67% compilation time)

backend: forwarddiff
inference/gibbs.jl 560.376812 seconds (1.45 G allocations: 88.314 GiB, 3.44% gc time, 18.15% compilation time)
contrib/inference/sghmc.jl 17.546238 seconds (39.22 M allocations: 2.369 GiB, 4.86% gc time, 98.21% compilation time)

backend: tracker
inference/gibbs.jl 625.175642 seconds (1.37 G allocations: 81.856 GiB, 3.53% gc time, 7.08% compilation time)
contrib/inference/sghmc.jl 8.747512 seconds (36.67 M allocations: 1.670 GiB, 6.77% gc time, 73.33% compilation time)

backend: reversediff
inference/gibbs.jl 652.075847 seconds (1.33 G allocations: 80.221 GiB, 3.12% gc time, 6.87% compilation time)
contrib/inference/sghmc.jl 8.816225 seconds (21.36 M allocations: 1.158 GiB, 4.94% gc time, 75.90% compilation time)

stdlib/RandomMeasures.jl 16.064905 seconds (23.89 M allocations: 1.440 GiB, 3.89% gc time, 67.57% compilation time)

Libtask.jl#this PR, AdvancedPS#releases with some fixes and Turing.jl#rh/tapedtask

inference/AdvancedSMC.jl 61.880572 seconds (125.02 M allocations: 7.795 GiB, 3.28% gc time, 95.91% compilation time)

backend: forwarddiff
inference/gibbs.jl 527.666963 seconds (1.43 G allocations: 87.700 GiB, 3.16% gc time, 15.61% compilation time)
contrib/inference/sghmc.jl 13.170983 seconds (39.23 M allocations: 2.369 GiB, 4.97% gc time, 98.49% compilation time)

backend: tracker
inference/gibbs.jl 611.141085 seconds (1.37 G allocations: 81.836 GiB, 3.25% gc time, 6.12% compilation time)
contrib/inference/sghmc.jl 8.443527 seconds (36.67 M allocations: 1.670 GiB, 6.98% gc time, 72.35% compilation time)

backend: reversediff
inference/gibbs.jl 594.619928 seconds (1.33 G allocations: 80.202 GiB, 2.84% gc time, 6.40% compilation time)
contrib/inference/sghmc.jl 6.370756 seconds (21.36 M allocations: 1.158 GiB, 4.99% gc time, 79.21% compilation time)

stdlib/RandomMeasures.jl 12.487840 seconds (24.52 M allocations: 1.476 GiB, 4.24% gc time, 68.20% compilation time)

So, the change from TapedTask to TapedTask{F} should not introduce any problems in running time.

@rikhuijzer rikhuijzer requested a review from yebai February 20, 2022 11:38
@yebai yebai merged commit 01c2727 into master Feb 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the rh/tapedtask branch February 20, 2022 20:20
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