Skip to content

Conversation

@penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Sep 8, 2025

what

As title. Essentially, this deletes the entirety of src/sampler.jl.

why

Because the type Sampler is gone, and also because DynamicPPL has no more actual samplers (in the past there were SampleFromPrior() and SampleFromUniform() that were actually samplers) that means we don't need to declare generic methods for AbstractMCMC.sample in DynamicPPL. That also means that the supporting interface used to define e.g. keyword argument defaults can also go.

Of course, it doesn't go completely: it just gets shifted upwards to Turing. In my opinion that makes a ton more sense anyway because that's where the concrete implementations of sample() are written. The current status is that these functions are defined in DynamicPPL but no methods are defined anywhere in DPPL, only in Turing. So may as well move the actual function there.

On top of this, the default implementation for sample was actually too restrictive in that it doesn't allow for AbstractMCMC.step_warmup to call DynamicPPL.initialstep. This means that you can't have a custom step_warmup for Turing samplers.

where now

My plan for which function goes where is outlined in the changelog:

DynamicPPL.jl/HISTORY.md

Lines 45 to 55 in be0d2ad

### Removal of `DynamicPPL.Sampler`
`DynamicPPL.Sampler` and **all associated interface functions** have also been removed entirely.
If you were using these, the corresponding replacements are:
- `DynamicPPL.Sampler(S)`: just don't wrap `S`; but make sure `S` subtypes `AbstractMCMC.AbstractSampler`
- `DynamicPPL.initialstep`: directly implement `AbstractMCMC.step` and `AbstractMCMC.step_warmup` as per the AbstractMCMC interface
- `DynamicPPL.loadstate`: `Turing.loadstate` (will be introduced in the next version)
- `DynamicPPL.default_chain_type`: `Turing.default_chain_type` (will be introduced in the next version)
- `DynamicPPL.initialsampler`: `Turing.init_strategy` (will be introduced in the next version; note that this function must return an `AbstractInitStrategy`, see above for explanation)
- `DynamicPPL.default_varinfo`: `Turing.default_varinfo` (will be introduced in the next version)

For the upcoming release of Turing v0.41 I will just do a straight copy paste of the code here, instead of doing any larger refactoring. But I would like this PR to be in DPPL 0.38 so that we don't have to cut a new minor version just to delete this.

@penelopeysm penelopeysm changed the base branch from breaking to py/remove-samplingcontext September 8, 2025 16:26
@penelopeysm penelopeysm closed this Sep 8, 2025
@penelopeysm penelopeysm reopened this Sep 8, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2025

Benchmark Report for Commit 35d0be2

Computer Information

Julia Version 1.11.6
Commit 9615af0f269 (2025-07-09 12:58 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results

|                 Model | Dimension |  AD Backend |      VarInfo Type | Linked | Eval Time / Ref Time | AD Time / Eval Time |
|-----------------------|-----------|-------------|-------------------|--------|----------------------|---------------------|
| Simple assume observe |         1 | forwarddiff |             typed |  false |                  9.8 |                 1.4 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                655.7 |                49.0 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                417.1 |                56.3 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1068.5 |                33.1 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               8124.1 |                23.7 |
|           Smorgasbord |       201 | reversediff |             typed |   true |               1424.9 |                30.8 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |                991.9 |                 4.6 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               5847.2 |                 4.2 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |                999.7 |                 8.9 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              65432.2 |                 3.9 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               8741.3 |                10.0 |
|               Dynamic |        10 |    mooncake |             typed |   true |                145.4 |                11.7 |
|              Submodel |         1 |    mooncake |             typed |   true |                 13.9 |                 5.1 |
|                   LDA |        12 | reversediff |             typed |   true |               1044.2 |                 2.5 |

@yebai yebai self-requested a review September 9, 2025 20:44
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch 2 times, most recently from 0b87d0d to 992569f Compare September 18, 2025 13:03
Base automatically changed from py/remove-samplingcontext to breaking September 24, 2025 15:48
@github-actions
Copy link
Contributor

DynamicPPL.jl documentation for PR #1037 is available at:
https://TuringLang.github.io/DynamicPPL.jl/previews/PR1037/

@penelopeysm penelopeysm changed the title Remove Sampler and initialstep Remove Sampler and its interface Oct 13, 2025
@codecov
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.13%. Comparing base (e4c5bbd) to head (a3969f7).
⚠️ Report is 1 commits behind head on breaking.

Additional details and impacted files
@@             Coverage Diff              @@
##           breaking    #1037      +/-   ##
============================================
+ Coverage     80.98%   81.13%   +0.15%     
============================================
  Files            42       40       -2     
  Lines          3765     3722      -43     
============================================
- Hits           3049     3020      -29     
+ Misses          716      702      -14     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@penelopeysm penelopeysm marked this pull request as ready for review October 13, 2025 19:38
@penelopeysm penelopeysm requested a review from sunxd3 October 14, 2025 06:36
Copy link
Member

@sunxd3 sunxd3 left a comment

Choose a reason for hiding this comment

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

LGTM

@penelopeysm
Copy link
Member Author

Thanks @sunxd3! Will hold off on merging until we're happy with TuringLang/Turing.jl#2676 so as to not pull the rug from under it...

@sunxd3
Copy link
Member

sunxd3 commented Oct 16, 2025

Absolutely, I was going to mention this, but thought you had better judgement on the ordering of things. So no problem at all.

@penelopeysm penelopeysm merged commit 4addb5f into breaking Oct 20, 2025
16 of 17 checks passed
@penelopeysm penelopeysm deleted the py/no-sampler branch October 20, 2025 10:12
penelopeysm added a commit that referenced this pull request Oct 21, 2025
* Bump minor version

* bump benchmarks compat

* add a skeletal changelog

* `InitContext`, part 3 - Introduce `InitContext` (#981)

* Implement InitContext

* Fix loading order of modules; move `prefix(::Model)` to model.jl

* Add tests for InitContext behaviour

* inline `rand(::Distributions.Uniform)`

Note that, apart from being simpler code, Distributions.Uniform also
doesn't allow the lower and upper bounds to be exactly equal (but we
might like to keep that option open in DynamicPPL, e.g. if the user
wants to initialise all values to the same value in linked space).

* Document

* Add a test to check that `init!!` doesn't change linking

* Fix `push!` for VarNamedVector

This should have been changed in #940, but slipped through as the file
wasn't listed as one of the changed files.

* Add some line breaks

Co-authored-by: Markus Hauru <markus@mhauru.org>

* Add the option of no fallback for ParamsInit

* Improve docstrings

* typo

* `p.default` -> `p.fallback`

* Rename `{Prior,Uniform,Params}Init` -> `InitFrom{Prior,Uniform,Params}`

---------

Co-authored-by: Markus Hauru <markus@mhauru.org>

* use `varname_leaves` from AbstractPPL instead (#1030)

* use `varname_leaves` from AbstractPPL instead

* add changelog entry

* fix import

* tidy occurrences of varname_leaves as well (#1031)

* `InitContext`, part 4 - Use `init!!` to replace `evaluate_and_sample!!`, `predict`, `returned`, and `initialize_values` (#984)

* Replace `evaluate_and_sample!!` -> `init!!`

* Use `ParamsInit` for `predict`; remove `setval_and_resample!` and friends

* Use `init!!` for initialisation

* Paper over the `Sampling->Init` context stack (pending removal of SamplingContext)

* Remove SamplingContext from JETExt to avoid triggering `Sampling->Init` pathway

* Remove `predict` on vector of VarInfo

* Fix some tests

* Remove duplicated test

* Simplify context testing

* Rename FooInit -> InitFromFoo

* Fix JETExt

* Fix JETExt properly

* Fix tests

* Improve comments

* Remove duplicated tests

* Docstring improvements

Co-authored-by: Markus Hauru <markus@mhauru.org>

* Concretise `chain_sample_to_varname_dict` using chain value type

* Clarify testset name

* Re-add comment that shouldn't have vanished

* Fix stale Requires dep

* Fix default_varinfo/initialisation for odd models

* Add comment to src/sampler.jl

Co-authored-by: Markus Hauru <markus@mhauru.org>

---------

Co-authored-by: Markus Hauru <markus@mhauru.org>

* `InitContext`, part 5 - Remove `SamplingContext`, `SampleFrom{Prior,Uniform}`, `{tilde_,}assume` (#985)

* Remove `SamplingContext` for good

* Remove `tilde_assume` as well

* Split up tilde_observe!! for Distribution / Submodel

* Tidy up tilde-pipeline methods and docstrings

* Fix tests

* fix ambiguity

* Add changelog

* Update HISTORY.md

Co-authored-by: Markus Hauru <markus@mhauru.org>

---------

Co-authored-by: Markus Hauru <markus@mhauru.org>

* fix missing import

* Shuffle context code around and remove dead code (#1050)

* Delete the `"del"` flag (#1058)

* Delete del

* Fix a typo

* Add HISTORY entry about del

* Fixes for Turing 0.41 (#1057)

* setleafcontext(model, ctx) and various other fixes

* fix a bug

* Add warning for `initial_parameters=...`

* Remove `resume_from` and `default_chain_type` (#1061)

* Remove resume_from

* Format

* Fix test

* remove initial_params warning

* Allow more flexible `initial_params` (#1064)

* Enable NamedTuple/Dict initialisation

* Add more tests

* fix include_all kwarg for predict, improve perf (#1068)

* Fix `include_all` for predict

* Fix include_all for predict, some perf improvements

* Replace `Metadata.flags` with `Metadata.trans` (#1060)

* Replace Medata.flags with Metadata.trans

* Fix a bug

* Fix a typo

* Fix two bugs

* Rename trans to is_transformed

* Rename islinked to is_transformed, remove duplication

* Change pointwise_logdensities default key type to VarName (#1071)

* Change pointwise_logdensities default key type to VarName

* Fix a doctest

* Fix DynamicPPL / MCMCChains methods (#1076)

* Reimplement pointwise_logdensities (almost completely)

* Move logjoint, logprior, ... as well

* Fix imports, etc

* Remove tests that are failing (yes I learnt this from Claude)

* Changelog

* logpdf

* fix docstrings

* allow dict output

* changelog

* fix some comments

* fix tests

* Fix more imports

* Remove stray n

Co-authored-by: Markus Hauru <markus@mhauru.org>

* Expand `logprior`, `loglikelihood`, and `logjoint` docstrings

---------

Co-authored-by: Markus Hauru <markus@mhauru.org>

* Remove `Sampler` and its interface (#1037)

* Remove `Sampler` and `initialstep`

* Actually just remove the entire file

* forgot one function

* Move sampling test utils to Turing as well

* Update changelog to correctly reflect changes

* [skip ci] Make changelog headings more consistent

---------

Co-authored-by: Markus Hauru <markus@mhauru.org>
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