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

Merged
merged 35 commits into from
Oct 23, 2024
Merged

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

merged 35 commits into from
Oct 23, 2024

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Sep 25, 2024

Description

This PR bumps DynamicPPL to 0.29.

Julia

This necessitates dropping support for Julia < 1.10

AD backends

  • Tapir has been replaced with Mooncake as only the latter has true support for DynamicPPL 0.29. Since we no longer support Julia < 1.10, the conditional import of Tapir in the test suite has been removed (Mooncake is now always imported).
  • The re-export of AutoTracker, and any tests to do with Tracker, have been removed.

Tests

  • Shortened the name of the tests in GitHub Actions because the one which went --skip a b c d e took up too much screen width, making it hard to see the other parameters (OS, julia version etc.).
  • Some tests were originally disabled for specific AD backends. Attempted to re-enable as many tests as possible without breaking CI.
  • Removed conditionals which restricted tests to recent Julia versions.

Other package version bumps

Compat entries for some other packages have been bumped to take advantage of bug fixes.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.39%. Comparing base (24e6870) to head (9fb9e95).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/mcmc/mh.jl 70.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2341       +/-   ##
===========================================
+ Coverage    0.00%   86.39%   +86.39%     
===========================================
  Files          22       22               
  Lines        1533     1573       +40     
===========================================
+ Hits            0     1359     +1359     
+ Misses       1533      214     -1319     

☔ 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 11466651176

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 (+86.4%) to 86.395%

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 33.33%
Totals Coverage Status
Change from base Build 11314008798: 86.4%
Covered Lines: 1359
Relevant Lines: 1573

💛 - 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 and others added 2 commits October 11, 2024 15:11
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@penelopeysm
Copy link
Member Author

penelopeysm commented Oct 22, 2024

Once tests pass (hopefully soon 🙏), still TODO:

  • Update API docs
  • Changelog note

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@penelopeysm penelopeysm marked this pull request as ready for review October 22, 2024 18:53
@penelopeysm
Copy link
Member Author

penelopeysm commented Oct 22, 2024

CI is finally passing, so it's time to ask for you all to take a final look!

To be honest, there are no major code changes since the last round of reviews. Most of the CI errors were related to upstream issues which have since been fixed (mostly due to @willtebbutt's heroic efforts 🙌 ). Julia 1.11 coming out while this PR was open definitely also made it more fun 😄

There are a few outstanding TODOs which I've opened issues for:

However, this PR makes some sweeping changes to the library. To protect this PR from scope creep, I think we should address these in separate PRs. Notably, all of these issues are about tests, not the library code itself.

Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

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

I'm happy. Great work @penelopeysm!

.github/workflows/Tests.yml Show resolved Hide resolved
var .= r
return var, sum(logpdf_with_trans(dist, r, istrans(vi, vns[1]))), vi
# Just defer to `SampleFromPrior`.
retval = DynamicPPL.dot_assume(rng, SampleFromPrior(), dist, vns[1], var, vi)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this business with vns being a vector and just getting the first element, or some of the updategid! things. However, since they came from #2360 and were reviewed there, I'm okay with that.

@yebai yebai merged commit f388e61 into master Oct 23, 2024
62 checks passed
@yebai yebai deleted the ch branch October 23, 2024 16:09
@yebai
Copy link
Member

yebai commented Oct 23, 2024

Great work. Many thanks, @penelopeysm and all!

@sunxd3
Copy link
Collaborator

sunxd3 commented Oct 23, 2024

great work! thanks guys

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.

6 participants