Drop Transducers/Folds in favor of OhMyThreads and drop Optimization umbrella by devmotion · Pull Request #369 · mlcolab/Pathfinder.jl

devmotion · 2026-05-25T10:07:36Z

Fixes #144

Context

While adding Pathfinder to an environment, I noticed it pulled in both Transducers.jl and the full Optimization.jl umbrella, despite Pathfinder only using a handful of things from each. This PR shrinks the dependency footprint on both fronts.

This is meant as a suggestion — I'm very happy to iterate on the proposed API (in particular the ntasks / ntasks_per_run naming, defaults, and dispatch semantics) and welcome feedback on whether this is the right shape for Pathfinder.

Transducers/Folds → OhMyThreads

I like the abstractions and functional style of the Transducers ecosystem. That said, in my opinion the lack of active maintenance — even after the move to JuliaFolds2 — has made it increasingly difficult to keep it as a dependency in downstream packages. I've been working on similar removals elsewhere; for examples of how I've approached it, see:

Remove Transducers.jl dependency TuringLang/AbstractMCMC.jl#201 — Remove Transducers.jl dependency (merged)
Replace FLoops with manual Threads.@spawn JuliaML/MLUtils.jl#215 — Replace FLoops with manual Threads.@spawn (open)

In this PR, parallel iteration is routed through OhMyThreads.jl. The public-facing executor::Transducers.Executor keyword on pathfinder / multipathfinder is replaced by a plain ntasks::Int=1 (and ntasks_per_run on multipathfinder). Per-element RNG seeds are pre-drawn from the user's rng upfront so output is reproducible regardless of ntasks — covered by new tests that compare serial vs. threaded runs for both MersenneTwister and the default RNG.

As a side benefit, ProgressLogging now drives a progress bar in the multithreaded multipathfinder path too. On master this only worked for the serial path; the threaded branch dropped the progress wrapper entirely.

Optimization → OptimizationBase

Pathfinder only used two things from Optimization:

Optimization.solve — which is the same CommonSolve.solve exposed via SciMLBase (already a direct dep).
OptimizationState — which is provided directly by OptimizationBase since v5.

Dropping the umbrella in favor of OptimizationBase (plus tightening the compat floors so Optim, OptimizationBase, and OptimizationOptimJL actually intersect) shaves a handful of further transitive deps (AbstractTrees, LeftChildRightSiblingTrees, ProgressMeter, TerminalLoggers, ...) that Pathfinder never touched.

Result

Net: ~10% fewer packages, both directly and transitively (20 → 18 direct deps; 129 → 116 resolved packages).

Full test suite passes with --threads=4 (4162 / 4162). Includes new explicit-reproducibility tests for both single- and multi-path runs.

Breaking changes

executor and executor_per_run keywords are replaced by ntasks and ntasks_per_run. Hard break, version bumped 0.9.31 → 0.10.0.
Numerical outputs for a given seed may change because the per-element RNG seeding scheme is new; reproducibility within 0.10.x is guaranteed.

🤖 Generated with Claude Code

Drop the unmaintained Transducers.jl and Folds.jl dependencies and route parallel iteration through OhMyThreads.jl instead. The public-facing `executor::Transducers.Executor` keyword of `pathfinder` and `multipathfinder` is replaced by `ntasks::Int=1` (and `ntasks_per_run` on `multipathfinder`); per-element RNG seeds are pre-drawn from the user's `rng` so output is reproducible regardless of `ntasks`. Adds explicit reproducibility-across-`ntasks` tests for both explicit and default RNGs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

`Optimization.solve` is the same `CommonSolve.solve` exposed by `SciMLBase` (which Pathfinder already depends on), and `OptimizationState` is now defined directly in `OptimizationBase 5`. Switching to `SciMLBase.solve` and importing `OptimizationState` from `OptimizationBase` lets us drop the `Optimization` umbrella and its `AbstractTrees`/`ProgressMeter`/`TerminalLoggers`/etc. tail. Compat floors for `Optim`, `OptimizationBase`, and `OptimizationOptimJL` are tightened so the declared ranges actually intersect. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

codecov · 2026-05-25T10:17:09Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.55%. Comparing base (5b68d86) to head (6723ba3).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
+ Coverage   92.42%   93.55%   +1.13%     
==========================================
  Files          13       13              
  Lines         607      652      +45     
==========================================
+ Hits          561      610      +49     
+ Misses         46       42       -4

☔ View full report in Codecov by Harness.
📢 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.

sethaxen · 2026-06-08T13:56:49Z

Thanks for the PR! Before I review in detail, I wonder if you could comment on what is lost by this PR: namely, process-based parallelism. That is, via Transducers, a user could provided not only a ThreadedEx (or PreferParallel) to use thread-based parallelism, but they could also pass a DistributedEx to use process-based parallelism. I don't have a good sense for which use cases would benefit most from the latter and thus would no longer be covered by Pathfinder with this PR, but perhaps you might?

devmotion · 2026-06-09T06:30:56Z

Since there's neither code nor tests for any other executor beyond SequentialEx and ThreadedEx, I assumed that these are the only two that are currently officially supported.

On closer inspection, I also noticed (and was able to reproduce) that the current implementation of multipathfinder can lead to identical fits on the workers as the RNG is captured by value and serialization/deserialization to workers creates an exact copy of it, with the same RNG state. Moreover, in contrast to this PR there are no reproducibility guarantees.

As expected, parallelization across workers seems to help if the log density function is computationally expensive. For cheap log densities, it seems it can be even slower than sequential runs as the serialization/deserialization overhead seems to outweigh any potential benefit. For expensive log densities it's different, e.g. I saw a ~3.85x speed-up on 4 processes - but on that example multithreaded execution (both on the main branch and this PR) also gave a ~3.65x speed-up.

So my take-away is that currently multi-process parallelization is not properly supported and has a silent bug but in principle for expensive log densities it could be beneficial. If you actually want to support it, I think I could add support for it in a similar way (ie using reproducible pre-run seeding) using pmap, either in this PR or a follow-up PR.

sethaxen

@devmotion thanks for the explanation! Yes, it's true we neither tested for nor advertised multiprocessing support, so I think it's fine if this PR is not constrained to support it.

After an initial review, I had a few questions/comments.

Both maximize_elbo and multipathfinder open-coded the same reproducible, seeded, chunked parallel-map idiom (RNG-pool Channel + tmapreduce over chunks + sequential fallback), and the importance resampling repeated a maybe-parallel map/reduce twice. Add internal helpers to src/utils.jl: - _nchunks: single source of truth for chunk/task count from ntasks - _chunk_tmap: generic chunked parallel map over equal-axes arrays with per-chunk setup state; seeding now lives at the call sites - _maybe_tmap / _maybe_tmapreduce: rng-free resampling reductions The RNG pool is replaced by a per-chunk copy(rng) built in setup; this is result-identical since every chunk RNG is re-seeded per element before use. Per-chunk optimizer deepcopy is preserved via setup. Algorithm logic and reproducibility across ntasks are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

sethaxen

Thanks, with the new code in utils.jl, the implementations are much more readable. I just have some minor suggestions.

sethaxen · 2026-06-17T09:53:33Z

        multipathfinder(ℓ, 10; nruns=2)
    end
+
+    @testset "reproducibility across ntasks" begin


Maybe we should only run this if Threads.nthreads() > 1?

Even for Threads.nthreads() == 1, I think the tests are useful: They ensure that rerunning with the same seed or RNG yields the same results.

Ah, I see now. Agreed.

sethaxen · 2026-06-17T09:55:15Z

        pathfinder(build_logdensityproblem(logp, 3, 2))
    end
+
+    @testset "reproducibility across ntasks" begin


Maybe only run if Threads.nthreads() > 1?

Same as above, I think even in the Threads.nthreads() == 1 case the tests are useful for ensuring reproducibility.

sethaxen · 2026-06-17T09:59:56Z

    optimizer=default_optimizer(history_length),
-    executor::Transducers.Executor=Transducers.SequentialEx(),
-    executor_per_run=Transducers.SequentialEx(),
+    ntasks::Int=1,


Here and for single-path Pathfinder, would it make sense to warn the user if any ntasks is greater than 1 but Threads.nthreads()==1?

Maybe? I guess it could be surprising to users that do not know about how to start Julia multithreaded if ntasks > 1 doesn't give any speedup.

Co-authored-by: Seth Axen <seth@sethaxen.com>

sethaxen

LGTM! Thanks!

sethaxen · 2026-06-17T10:42:02Z

        multipathfinder(ℓ, 10; nruns=2)
    end
+
+    @testset "reproducibility across ntasks" begin


Ah, I see now. Agreed.

devmotion and others added 4 commits May 25, 2026 11:46

Update environments of integration tests

83df232

Update docs environment

749ef22

Change seed of Turing example

4270ee7

devmotion marked this pull request as ready for review May 25, 2026 16:01

Merge branch 'main' into pr/369

e6e5e44

sethaxen requested changes Jun 15, 2026

View reviewed changes

Comment thread ext/PathfinderTuringExt.jl

Comment thread src/elbo.jl Outdated

Comment thread src/multipath.jl

Comment thread src/elbo.jl Outdated

sethaxen requested changes Jun 17, 2026

View reviewed changes

Apply suggestions from code review

6723ba3

Co-authored-by: Seth Axen <seth@sethaxen.com>

sethaxen approved these changes Jun 17, 2026

View reviewed changes

sethaxen merged commit 2622112 into mlcolab:main Jun 17, 2026
16 checks passed

devmotion deleted the dmw/ohmythreads branch June 17, 2026 10:48

Uh oh!

Conversation

devmotion commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Transducers/Folds → OhMyThreads

Optimization → OptimizationBase

Result

Breaking changes

Uh oh!

codecov Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sethaxen commented Jun 8, 2026

Uh oh!

devmotion commented Jun 9, 2026

Uh oh!

sethaxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethaxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethaxen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

devmotion commented May 25, 2026 •

edited

Loading

codecov Bot commented May 25, 2026 •

edited

Loading