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 global AD flag "ADBACKEND" and function suite #2134

Merged
merged 15 commits into from
Dec 13, 2023

Conversation

sunxd3
Copy link
Member

@sunxd3 sunxd3 commented Nov 24, 2023

Addressing #2132, ref #2047 (comment)

This is likely to be quite hairy, as the ADBACKEND goes back.

Steps towards completion:

  • Replace Turing.ADBackend() with AutoForwardDiff as default for adtype argument in samplers
  • Remove setadbackend function, the main issue here is that tests use it quite extensively, so some refactoring is needed
  • Remove ADSAFE, CHUNKSIZE, and RDCache and related functions
  • Update documentation and register a breaking release

@sunxd3
Copy link
Member Author

sunxd3 commented Nov 24, 2023

For the second item, adtype needs to be thread into tests like

Turing.setrdcache(false)
for adbackend in (:forwarddiff, :reversediff)
@timeit TIMEROUTPUT "inference: $adbackend" begin
Turing.setadbackend(adbackend)
@info "Testing $(adbackend)"
@testset "inference: $adbackend" begin
@testset "samplers" begin
@timeit_include("mcmc/gibbs.jl")
@timeit_include("mcmc/gibbs_conditional.jl")
@timeit_include("mcmc/hmc.jl")
@timeit_include("mcmc/Inference.jl")
@timeit_include("mcmc/sghmc.jl")
@timeit_include("mcmc/abstractmcmc.jl")
@timeit_include("mcmc/mh.jl")
@timeit_include("ext/dynamichmc.jl")
end
end
@testset "variational algorithms : $adbackend" begin
@timeit_include("variational/advi.jl")
end
@testset "mode estimation : $adbackend" begin
@timeit_include("optimisation/OptimInterface.jl")
@timeit_include("ext/Optimisation.jl")
end
end
end

@sunxd3
Copy link
Member Author

sunxd3 commented Dec 3, 2023

Related documentation PR TuringLang/docs#430

@sunxd3
Copy link
Member Author

sunxd3 commented Dec 4, 2023

@yebai yebai requested a review from torfjelde December 4, 2023 16:40
@torfjelde
Copy link
Member

@sunxd3 can you undo the formatting changes? It makes it somewhat annoying to review + makes version history a less clear

@coveralls
Copy link

coveralls commented Dec 6, 2023

Pull Request Test Coverage Report for Build 7194200743

  • 0 of 9 (0.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage remained the same at 0.0%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/essential/ad.jl 0 2 0.0%
src/mcmc/hmc.jl 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
ext/TuringOptimExt.jl 1 0.0%
src/essential/ad.jl 1 0.0%
Totals Coverage Status
Change from base Build 6890657606: 0.0%
Covered Lines: 0
Relevant Lines: 1390

💛 - Coveralls

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (6649f10) 0.00% compared to head (db8197c) 0.00%.

❗ Current head db8197c differs from pull request most recent head 816f1f9. Consider uploading reports for the commit 816f1f9 to get more accurate results

Files Patch % Lines
src/mcmc/hmc.jl 0.00% 7 Missing ⚠️
src/essential/ad.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2134   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1421    1390   -31     
======================================
+ Misses       1421    1390   -31     

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

@sunxd3
Copy link
Member Author

sunxd3 commented Dec 6, 2023

@torfjelde another look? I am not familiar with the test failure, would love to take a closer look if not obvious why.

src/essential/Essential.jl Outdated Show resolved Hide resolved
src/mcmc/hmc.jl Outdated Show resolved Hide resolved
src/mcmc/hmc.jl Outdated Show resolved Hide resolved
src/mcmc/hmc.jl Outdated Show resolved Hide resolved
src/mcmc/hmc.jl Outdated Show resolved Hide resolved
src/mcmc/sghmc.jl Outdated Show resolved Hide resolved
src/mcmc/sghmc.jl Outdated Show resolved Hide resolved
result = optimize(model, MAP(), optimizer)
vals = result.values
@testset "$(nameof(typeof(optimizer)))" for optimizer in [LBFGS(), NelderMead()]
result = optimize(model, MAP(), optimizer)
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add adtypes interface to optimisation-based algorithms, e.g. MLE, MAP?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I am not mistaken, Optim.jl seems to only support ForwardDiff (https://github.com/JuliaNLSolvers/NLSolversBase.jl/blob/78af38393b14992ea996899c6486d971b5bfa612/src/NLSolversBase.jl#L5). If that's the case, it may not make much sense to add ADtypes now

test/mcmc/sghmc.jl Show resolved Hide resolved
@yebai
Copy link
Member

yebai commented Dec 7, 2023

Thanks, @sunxd3 -- it looks good overall. I left a few minor comments above.

@yebai
Copy link
Member

yebai commented Dec 12, 2023

The remaining failing test is reproducible on the master branch so unlikely related to this PR @sunxd3 can you update the docs so we can merge this PR?

@sunxd3
Copy link
Member Author

sunxd3 commented Dec 13, 2023

@yebai the documentation is updated here, although the building will error with the master branch version of Turing

@sunxd3 sunxd3 marked this pull request as ready for review December 13, 2023 10:55
@sunxd3
Copy link
Member Author

sunxd3 commented Dec 13, 2023

Also @yebai @devmotion, the current v0.30.0 is not registered yet, I did bump the minor version to 0.31.0, but maybe we can treat the PR as a part of 0.30 release?

Project.toml Outdated Show resolved Hide resolved
@yebai yebai merged commit 239263e into master Dec 13, 2023
3 of 11 checks passed
@yebai yebai deleted the sunxd/remove_ad_backend branch December 13, 2023 15:23
@devmotion devmotion mentioned this pull request Jan 19, 2024
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.

4 participants