feat(mlx): pt.random support with mlx backend - #1979
Conversation
b620bb1 to
d602268
Compare
ricardoV94
left a comment
There was a problem hiding this comment.
missing rng outputs /updates (so consecutive calls get updated rng)
There should be tests in numba/jax you can use as template. Jax is going to be more similar
| thunk_inputs = [] | ||
| for n in self.fgraph.inputs: | ||
| sinput = storage_map[n] | ||
| if isinstance(sinput[0], Generator): |
There was a problem hiding this comment.
you need to do the same dance jax linker does with shared Generator variables
|
#2010 caused conflicts for this PR. You will need to rebase. |
e6f7371 to
0b4fb85
Compare
| def sample_fn(rng_key, size, dtype, p): | ||
| p = mx.array(p) | ||
| if size is None: | ||
| shape = p.shape |
There was a problem hiding this comment.
you always need the shape? You didn't need it in the categorical. I would assume you only need when one of the parameters doesn't go in the random function. If so that would take a lot of boilerplate away from your dispatches
There was a problem hiding this comment.
my comment wasn't about Bernoulli specifically, I would expect you don't need to define shape explicitly (when the user didn't do it themselves) most of the time
0af680e to
5d450de
Compare
|
why? |
|
Accident |
|
@williambdean what enticements are required to have you get this over the line? It seems to be the last major blocker to mlx-only workflows in pymc |
|
I will take look tonight and tomororw |
|
Changes since last review:
Remaining distributions (Gamma, Poisson, StudentT, Beta, etc.) are more involved and can be discussed as follow-up. |
|
Whats the deal with the benchmark? |
|
Can you take look when you get the time @jessegrabowski 🙏 |
|
Yes and I recognize the hypocrisy of asking you to do it then not providing a review :) re: benchmark, there's something about we already used 256 slots or something. @ricardoV94 knows what is wrong but can't be bothered to fix it. |
|
ha, all good 😄 |
|
Hey @williambdean 👋 — went through this and found a few things while testing the dispatches, so I pushed a commit on top (
Added regression tests for each. Branch merges cleanly with |
|
did you just add a bunch of stuff and then approve your own code with a "lgtm" comment? |
|
The vibes look good |
|
I'll give a review today, sorry for being slow |
kinda 😅 I saw a few minor issues (only 2 files to adjust), and solve them, everything was almost there. But ofc, let's wait your reviews! |
|
What is needed to get this over the finish line? |
jessegrabowski
left a comment
There was a problem hiding this comment.
Looks good on my end, but I made some small suggestions on the tests. I say we put it in and see what happens. Highest risk for correctness is in fgraph_convert, I'd ask that you make sure that's carefully tested -- check that you're doing at least as much as the jax suite is doing.
| pytensor.function([], rv, mode="MLX", updates=srng.updates()) | ||
|
|
||
|
|
||
| def test_beta_not_implemented(): |
There was a problem hiding this comment.
These "not implemented" tests can be removed
| ) | ||
|
|
||
|
|
||
| def test_gumbel_shape_dtype(): |
There was a problem hiding this comment.
All of the test_shape_dtype tests could be collapsed to a single parameterized test, would be a bit easier to maintain in the long term
jessegrabowski
left a comment
There was a problem hiding this comment.
Rebased this onto main in a worktree — 12 commits, clean, no conflicts. With that on top it trains a convolutional VAE on MLX end to end (reparameterization trick and all), 30 epochs in 1.7s against numba's 23.8s for the same final loss. The AttributeError: 'int' object has no attribute 'astype' I hit before the rebase was version skew, not this PR.
Four small comments below, none blocking. Checked the MvNormal factorizations numerically — A Aᵀ == cov for all three methods.
| def mlx_sample_fn_loc_scale(op, node): | ||
| """Loc-scale families: MLX names the standard sampler like the Op, so draw | ||
| it and apply ``loc + scale * z`` (mirrors the JAX dispatch).""" | ||
| mlx_op = getattr(mx.random, op.name) |
There was a problem hiding this comment.
Looking the MLX function up by op.name ties PyTensor's Op names to MLX's API surface. Works today, but a rename either side turns into an AttributeError here instead of the NotImplementedError the singledispatch fallback already gives you. An explicit {ptr.NormalRV: mx.random.normal, ...} is the same length and fails predictably.
| def mlx_sample_fn_bernoulli(op, node): | ||
| def sample_fn(rng_key, size, dtype, p): | ||
| p = mx.array(p) | ||
| shape = mlx_to_list_shape(size) if size is not None else None |
There was a problem hiding this comment.
Everything else goes through _shape_from_size, which broadcasts the parameters when size is None; bernoulli and categorical pass None and let MLX derive it from p. If that's deliberate a one-liner saying so would help — otherwise it reads as drift from the helper. Same on line 130.
|
|
||
| # Factor ``cov = A @ A.T`` so that ``mean + A @ z`` has covariance ``cov``. | ||
| if method == "cholesky": | ||
| A = mx.linalg.cholesky(cov, stream=mx.cpu) |
There was a problem hiding this comment.
Worth a word on why these three run on the CPU stream — I assume MLX's GPU linalg doesn't cover cholesky/svd/eigh, but the next reader has to guess.
| scale = mx.array(scale, dtype=mlx_dtype) | ||
| shape = _shape_from_size(size, scale) | ||
| u = mx.random.uniform(shape=shape, dtype=mlx_dtype, key=rng_key) | ||
| return -scale * mx.log(u) |
There was a problem hiding this comment.
mx.random.uniform is [low, high), so u = 0 gives -inf here and at the logistic on 233. I couldn't produce one in 200M draws — smallest u was 7.9e-9 — so this is insurance rather than a fix, but -scale * mx.log1p(-u) costs nothing.
|
Thanks for the feedback, @jessegrabowski . Should be all addressed! |
|
tests are failing :( |
|
@williambdean i think some of these test failures aren't you and were solved by #2376 . A simple rebase will clear many/most/all. |
…ethods, permutation error at dispatch time
- LogNormal: exp(normal(mu, sigma)) - HalfNormal: abs(normal(loc, scale)) - Exponential: -scale * log(uniform) (inverse CDF) - Logistic: loc + scale * log(u / (1 - u)) (inverse CDF) - Cauchy: loc + scale * tan(pi * (u - 0.5)) (inverse CDF) - Normal: pass loc/scale directly to MX when size is None - Add tests for all new distributions
- Consolidate Normal/Laplace/Gumbel into a single loc-scale dispatch via getattr(mx.random, op.name) and add a _shape_from_size helper, removing the per-distribution shape/affine boilerplate (mirrors the JAX dispatch). - HalfNormal: sample loc + scale*|z| instead of abs(loc + scale*z); the latter was wrong for nonzero loc (support must be [loc, inf)). - MvNormal: replace the reshape-based size branch with a single broadcasting matmul (mean + (A @ z[..., None])[..., 0]) so batched parameters combined with an explicit size no longer crash; guard empty batch dims, which segfaulted the MLX compiled matmul path. - Integers: sample at full int64 width then cast the result, so narrow output dtypes wrap (matching numpy) and wide bounds aren't clipped to MLX's default int32 sampling dtype. - Bernoulli/Categorical: cast the bool/uint32 draw to the declared int dtype. - Raise a clear NotImplementedError for non-PCG64 bit generators instead of an opaque KeyError. Adds regression tests for each fix. Developed with AI assistance (Cursor). Co-authored-by: Cursor <cursoragent@cursor.com>
- _shape_from_size: return a concrete tuple from mx.broadcast_shapes so mypy no longer reports no-any-return. - Tighten docstrings/comments to one-line intent (why, not what). Co-authored-by: Cursor <cursoragent@cursor.com>
mx.random primitives reject float64 regardless of device, while upstream convert_dtype_to_mlx now preserves float64 on CPU. Sample in float32 and widen the draw centrally; use float dtype family in remaining test assertions.
2c5a2ba to
d6f1884
Compare
|
Rebase should be done! |
Description
Basic support for
mlxrandom generation.They have limited support. Missing Gamma distribution. Could support additional ones
with basic transformations. i.e.
pt.abs(pt.random.normal(...))~ Half NormalMLX Reference: https://ml-explore.github.io/mlx/build/html/python/random.html
Related Issue
Checklist
Type of change