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

DynamicPPL -> 0.29; Julia -> 1.10; Tapir -> Mooncake #2341

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Sep 25, 2024

This PR bumps DynamicPPL to 0.29.

Julia

This necessitates dropping support for Julia < 1.10

Mooncake

Likewise, I've replaced Tapir -> Mooncake as only the latter has true support for DynamicPPL 0.29.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (40a0d84) to head (742364e).

Files with missing lines Patch % Lines
src/mcmc/mh.jl 0.00% 20 Missing ⚠️
src/mcmc/ess.jl 0.00% 1 Missing ⚠️
src/mcmc/particle_mcmc.jl 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (40a0d84) and HEAD (742364e). Click for more details.

HEAD has 25 uploads less than BASE
Flag BASE (40a0d84) HEAD (742364e)
56 31
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #2341       +/-   ##
==========================================
- Coverage   86.67%   0.00%   -86.68%     
==========================================
  Files          24      22        -2     
  Lines        1599    1535       -64     
==========================================
- Hits         1386       0     -1386     
- Misses        213    1535     +1322     

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

@coveralls
Copy link

coveralls commented Sep 25, 2024

Pull Request Test Coverage Report for Build 11218401757

Details

  • 16 of 22 (72.73%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 87.516%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/mcmc/mh.jl 14 20 70.0%
Files with Coverage Reduction New Missed Lines %
src/stdlib/RandomMeasures.jl 1 34.72%
Totals Coverage Status
Change from base Build 11180921169: 0.6%
Covered Lines: 1381
Relevant Lines: 1578

💛 - Coveralls

@penelopeysm penelopeysm marked this pull request as draft September 25, 2024 16:49
@penelopeysm penelopeysm changed the title CompatHelper: DynamicPPL -> 0.29 (Blocked) CompatHelper: DynamicPPL -> 0.29 Sep 25, 2024
@penelopeysm penelopeysm marked this pull request as ready for review September 26, 2024 22:25
@penelopeysm penelopeysm changed the title (Blocked) CompatHelper: DynamicPPL -> 0.29 DynamicPPL -> 0.29; Julia -> 1.9 Sep 26, 2024
@penelopeysm
Copy link
Member Author

Tests still aren't passing, there are still a few changes that need to be made to bring it up to date with TuringLang/DynamicPPL.jl#575.

I don't mind handling this, but it will take me a long time, so if anybody else feels like expediting it please feel free to jump in:)

@penelopeysm penelopeysm marked this pull request as draft September 26, 2024 23:28
@mhauru
Copy link
Member

mhauru commented Sep 30, 2024

I'll ask around if anyone objects to dropping <1.9.

@mhauru mhauru self-requested a review September 30, 2024 14:44
@mhauru mhauru self-assigned this Sep 30, 2024
@penelopeysm
Copy link
Member Author

In the meantime I can work to get the PR ready to merge, we will need it at some point anyway whether it's sooner or later:)

src/mcmc/mh.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@penelopeysm penelopeysm changed the title DynamicPPL -> 0.29; Julia -> 1.9 DynamicPPL -> 0.29; Julia -> 1.10 Oct 3, 2024
@penelopeysm
Copy link
Member Author

penelopeysm commented Oct 3, 2024

As it turns out, Julia 1.9 won't work either, because SciMLBase.

(see e.g. https://github.com/TuringLang/Turing.jl/actions/runs/11153561181/job/31001300867)

setting a real number as σ is deprecated
@penelopeysm penelopeysm changed the title DynamicPPL -> 0.29; Julia -> 1.10 DynamicPPL -> 0.29; Julia -> 1.10; Tapir -> Mooncake Oct 3, 2024
@penelopeysm
Copy link
Member Author

penelopeysm commented Oct 3, 2024

I think everything almost works, except for a Mooncake-or-further-upstream bug which I'll report tomorrow.

Edit: compintell/Mooncake.jl#283

@penelopeysm
Copy link
Member Author

@yebai / @mhauru Since we're making fairly sweeping changes here, is it an opportune time to remove Tracker support as well? #2356

@penelopeysm penelopeysm marked this pull request as ready for review October 4, 2024 09:25
@penelopeysm penelopeysm marked this pull request as draft October 4, 2024 09:25
@penelopeysm
Copy link
Member Author

@torfjelde Could I get you to take a look at my changes to src/mcmc please? The tests all pass, but I'm a little anxious 😄

Also, this PR fixes the following bug, but not entirely sure what's the best way to write a test for it. (Other samplers already work correctly with this, it's just MH that fails.) Would it make sense to extend test/test_utils/models.jl and test/test_utils/numerical_tests.jl?

julia> using Turing

julia> @model f() = x ~ LKJCholesky(2, 1)
f (generic function with 2 methods)

julia> sample(f(), MH(), 100)
ERROR: MethodError: no method matching length(::LinearAlgebra.Cholesky{Float64, Matrix{Float64}})
[...]

@torfjelde
Copy link
Member

torfjelde commented Oct 4, 2024

(Other samplers already work correctly with this, it's just MH that fails.) Would it make sense to extend test/test_utils/models.jl and test/test_utils/numerical_tests.jl?

I'd just add it to the MH specific tests for now 👍

mhauru and others added 4 commits October 7, 2024 16:02
* Bring more files into autoformatting fold

* Run JuliaFormatter

* Remove trailing whitespace

* Update comment in .JuliaFormatter.toml
@yebai
Copy link
Member

yebai commented Oct 8, 2024

@yebai / @mhauru Since we're making fairly sweeping changes here, is it an opportune time to remove Tracker support as well? #2356

It sounds good to me.

@penelopeysm
Copy link
Member Author

This upgrade is also blocked by TuringLang/docs#521 – ideally we want to make sure any relevant docs build with Mooncake. Because we don't want to upgrade to 0.35.0, patch Mooncake to work for the docs, and then release 0.35.1 with a new compat entry for Mooncake just so that the docs work.

However, the Bayesian NN example errors with Mooncake. Will create specific upstrea issues etc when I get the time

@penelopeysm penelopeysm mentioned this pull request Oct 9, 2024
Comment on lines +14 to +15
using ADTypes:
ADTypes, AutoForwardDiff, AutoReverseDiff, AutoZygote, AutoMooncake
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
using ADTypes:
ADTypes, AutoForwardDiff, AutoReverseDiff, AutoZygote, AutoMooncake
using ADTypes: ADTypes, AutoForwardDiff, AutoReverseDiff, AutoZygote, AutoMooncake

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.

5 participants