Skip to content

Refactor existing samplers behind a shared Sampler interface#8358

Open
twiecki wants to merge 3 commits into
mainfrom
sampler-refactor
Open

Refactor existing samplers behind a shared Sampler interface#8358
twiecki wants to merge 3 commits into
mainfrom
sampler-refactor

Conversation

@twiecki

@twiecki twiecki commented Jul 13, 2026

Copy link
Copy Markdown
Member

What

The refactor-only unit requested in #8353 review: every existing way of sampling a model becomes a Sampler with two equivalent surfaces, no new sampling capability is added. The generic blackjax algorithm support in #8353 will be rebased on top of this once merged.

# the funnel: constructor takes algorithm configuration, pm.sample takes run configuration
pm.sample(sampler=pm.nutpie.nuts(target_accept=0.9), draws=1000, chains=4)
pm.sample(sampler=pm.StepSampler(step=pm.Metropolis()))

# the flat entry points: algorithm and run arguments in one call
pm.nutpie.nuts.sample(model=m, target_accept=0.9, draws=1000)
pm.numpyro.nuts.sample(model=m, chain_method="vectorized")
pm.blackjax.nuts.sample(model=m)

How

Following the class sketch from the #7880/#8353 discussion:

  • Sampler ABC (pymc/sampling/samplers/base.py): constructors are algorithm configuration only — nothing model-bound, nothing compiled. The model and the shared run configuration arrive at sample time via sample_from_init, whose signature is exactly pm.sample's run subset (kept in sync by a test, no **kwargs escape hatch). Deliberately not named sample so it can't be confused with the flat entry points.
  • ExternalSampler is implementation inheritance only: a package/version_range dependency check with an install hint.
  • SamplerEntry makes the two surfaces one implementation: pm.nutpie.nuts(...) returns a configured sampler for the funnel; pm.nutpie.nuts.sample(...) is the flat call, defined in terms of the first so they cannot disagree.
  • StepSampler: the native machinery (step assignment, CompoundStep, the python loop) as a regular sampler. The engine was extracted verbatim from pm.sample into _sample_with_step_methods and is shared by both paths — a test asserts pm.sample() and pm.sample(sampler=pm.StepSampler()) produce identical draws. NUTS initialization configuration (init, n_init, jitter_max_retries) lives on its constructor, which is also the first step toward fixing init_nuts not used when CompoundStep is used #5658.
  • Nutpie, NumpyroNUTS, BlackjaxNUTS wrap the existing external NUTS implementations. Each documents an explicit disposition per run argument: nutpie consumes compile_kwargs/backend (numba/jax) and honors discard_tuned_samples; the jax pair raises on a non-jax compile mode instead of silently ignoring it, and warns on discard_tuned_samples=False/keep_warning_stat.

pm.sample(sampler=...) forwards only the run configuration and rejects the NUTS-specific arguments with pointers to their new homes.

Deprecations (DeprecationWarning, not FutureWarning yet)

  • nuts_sampler=pm.sample(sampler=pm.nutpie.nuts()) / pm.numpyro.nuts() / pm.blackjax.nuts() / pm.StepSampler(), or the flat entry points.
  • Explicit init/n_init/jitter_max_retriespm.sample(sampler=pm.StepSampler(init=...)).
  • Untouched defaults stay silent (tested), so plain pm.sample() is unaffected.

The acceptance criteria from the discussion hold by construction: the internal NUTS is expressible as a regular sampler through the same interface with no special-casing in pm.sample, and every nuts_sampler= string value has a class equivalent, so the argument is removable at the end of the deprecation cycle.

Also included

  • pyarrow<25 pin in the alternative_backends CI job — pyarrow 25.0.0 (unpinned transitive dependency of the nutpie@main install) segfaults XLA CPU compilation when loaded in the same process; without this pin the job fails on every PR (root-cause analysis in #8353).

Tests

tests/sampling/test_samplers.py: StepSampler via funnel/flat/explicit-step plus the identical-draws equivalence test, all three external NUTS samplers via both surfaces, funnel forwarding/rejections, the signature-sync test across all sampler classes, deprecation warnings (and their absence for defaults), and dependency-check errors. Full tests/sampling/test_mcmc.py passes unchanged (106 tests).

🤖 Generated with Claude Code

Every way of sampling a model is now a Sampler with two equivalent
surfaces: a configured object for pm.sample(sampler=...) and a flat
entry point (pm.nutpie.nuts.sample(...)), under the rule "constructor
takes algorithm configuration, pm.sample takes run configuration".
This is the refactor-only unit agreed in pymc#8353; new blackjax
algorithm support builds on it in a follow-up.

- Sampler ABC: constructors are algorithm configuration only (nothing
  model-bound, nothing compiled); the model and the shared run
  configuration arrive at sample time via sample_from_init, whose
  signature is kept in sync with pm.sample by a test. The method is
  deliberately not named `sample` to distinguish it from the flat
  entry points. ExternalSampler adds only a dependency check.
- StepSampler wraps the native machinery (step assignment,
  CompoundStep, the python loop) as a regular sampler. The engine was
  extracted verbatim from pm.sample into _sample_with_step_methods and
  is shared by both, so pm.sample() and
  pm.sample(sampler=pm.StepSampler()) produce identical draws
  (asserted by a test). NUTS initialization configuration (init,
  n_init, jitter_max_retries) lives on its constructor.
- Nutpie, NumpyroNUTS and BlackjaxNUTS wrap the existing external
  NUTS implementations, each documenting per run argument whether it
  is honored, reinterpreted, warned about, or rejected (e.g. a
  non-jax compile mode raises for the jax samplers while nutpie
  consumes it). Exposed as pm.nutpie.nuts, pm.numpyro.nuts and
  pm.blackjax.nuts.
- pm.sample grows sampler=, forwarding only run configuration and
  rejecting NUTS-specific arguments; nuts_sampler= and explicit
  init/n_init/jitter_max_retries now emit DeprecationWarning (not yet
  FutureWarning) pointing at their new homes.
- Pin pyarrow<25 in the alternative_backends CI job (transitive
  nutpie dependency that segfaults XLA CPU compilation in-process).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.94009% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.71%. Comparing base (56384e5) to head (049cf0f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pymc/sampling/samplers/jax_nuts.py 78.00% 11 Missing ⚠️
pymc/sampling/samplers/base.py 83.33% 7 Missing ⚠️
pymc/sampling/mcmc.py 95.45% 2 Missing ⚠️
pymc/sampling/samplers/nutpie.py 93.10% 2 Missing ⚠️
pymc/sampling/samplers/step.py 94.87% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8358      +/-   ##
==========================================
- Coverage   91.73%   91.71%   -0.02%     
==========================================
  Files         128      135       +7     
  Lines       20672    20910     +238     
==========================================
+ Hits        18963    19178     +215     
- Misses       1709     1732      +23     
Files with missing lines Coverage Δ
pymc/__init__.py 100.00% <100.00%> (ø)
pymc/sampling/__init__.py 100.00% <100.00%> (ø)
pymc/sampling/samplers/__init__.py 100.00% <100.00%> (ø)
pymc/sampling/samplers/blackjax.py 100.00% <100.00%> (ø)
pymc/sampling/samplers/numpyro.py 100.00% <100.00%> (ø)
pymc/sampling/mcmc.py 92.09% <95.45%> (+0.18%) ⬆️
pymc/sampling/samplers/nutpie.py 93.10% <93.10%> (ø)
pymc/sampling/samplers/step.py 94.87% <94.87%> (ø)
pymc/sampling/samplers/base.py 83.33% <83.33%> (ø)
pymc/sampling/samplers/jax_nuts.py 78.00% <78.00%> (ø)

... and 2 files with indirect coverage changes

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

@twiecki

twiecki commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

I found four API-equivalence regressions on a245862 that aren't covered by the current tests:

  1. Array-like random seeds are rejected by the recommended replacement. In jax_nuts.py:144, _get_seeds_per_chain(random_seed, 1) interprets an array as a one-chain seed list. Thus pm.sample(nuts_sampler='numpyro', chains=2, random_seed=[1, 2]) succeeds, while pm.sample(sampler=pm.numpyro.nuts(), chains=2, random_seed=[1, 2]) raises Number of seeds (2) does not match the number of chains (1). The same pattern appears in nutpie.py:93. The adapters should derive one master seed without reinterpreting the documented array-like input as a one-chain list.

  2. StepSampler bypasses step-specific tuning defaults. step.py:135 converts tune=None to 1000 before the shared engine can call get_default_tune_steps. A custom Metropolis step with default_tune_steps = 0 gets 0 tuning iterations through ordinary pm.sample(step=...), but 1,000 through pm.sample(sampler=pm.StepSampler(step=...)). None should reach the shared engine.

  3. The flat entry point misroutes backend as algorithm configuration. Because SamplerEntry.sample has no named backend parameter, pm.numpyro.nuts.sample(model=m, backend='jax', ...) passes it into NumPyro's NUTS constructor and raises TypeError: NUTS.__init__() got an unexpected keyword argument 'backend'. The funnel form works because pm.sample first resolves backend into compile_kwargs. The flat entry should do the same.

  4. quiet=True does not suppress the nutpie flat-entry progress bar. nutpie.py:104 forwards progressbar=True even when quiet=True; _sample_external_nuts only uses quiet for logging, so the progress manager remains enabled. The funnel hides this because pm.sample clears progressbar before dispatch.

All current GitHub checks are green, but I reproduced these cases locally against the PR head.

- Array-like random seeds: derive one master seed via get_random_generator
  instead of misreading them as one-chain seed lists (numpyro/blackjax/nutpie).
- tune=None now reaches get_default_tune_steps so step-specific tuning
  defaults apply through StepSampler and the sampler= funnel.
- The flat entry points resolve backend= into compile_kwargs like pm.sample.
- quiet=True disables the nutpie progress bar on the flat entry point.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@twiecki

twiecki commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

All four fixed in 049cf0f, each with a regression test (TestAPIEquivalenceRegressions):

  1. Array-like seeds: the numpyro/blackjax/nutpie adapters now derive one master seed via get_random_generator(random_seed).integers(...), which accepts everything pm.sample documents (None/int/array-like/Generator) instead of misreading arrays as one-chain seed lists.
  2. tune=None is now forwarded untouched through both the funnel and StepSampler, so get_default_tune_steps resolves it per step method exactly as legacy pm.sample(step=...) does. The run contract's annotation is int | None accordingly.
  3. Flat entry points accept backend= and resolve it into compile_kwargs via the same resolve_backend_compile_kwargs as pm.sample (signature-sync test updated to document the deliberate extra surface).
  4. quiet=True disables the nutpie progress bar on the flat entry, mirroring what pm.sample does before dispatch.

@ricardoV94

Copy link
Copy Markdown
Member

It's on my stack. Sidenote don't open branches on the main repo, use your fork

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.

2 participants