Fix dims coercion to list in model_from_fgraph by williambdean · Pull Request #8465 · pymc-devs/pymc

williambdean · 2026-10-05T16:48:18Z

`add_named_variable` is declared to take a tuple and is passed a list

pymc/model/core.py#L2196

def add_named_variable(self, var, dims: tuple[str | None, ...] | None = None):

pymc/model/fgraph.py#L381

-        dims = list(op.dims) if op.dims else None
+        dims = tuple(op.dims) if op.dims else None
         model.add_named_variable(var, dims=dims)

ModelVar coerces on the way in regardless, pymc/model/fgraph.py#L37, so the
graph never held a list:

self.dims = tuple(dims) if dims is not None else ()

Consequence: the round trip stops agreeing with itself

fgraph_from_model and model_from_fgraph are a pair, so every path built on
clone_model inherits the mismatch:

>>> model_from_fgraph(fgraph_from_model(m)[0]).named_vars_to_dims == m.named_vars_to_dims
False   # before
True    # after

Path	Before	After
`Model.copy()`, `copy.copy`, `copy.deepcopy`	False	True
`pm.do`	False	True
`pm.observe`	False	True

Invisible to CI: both files sit in the FAILING mypy baseline, scripts/run_mypy.py#L25.

No consumer breaks on a list

Reference	Behaviour
`pymc/backends/arviz.py#L226`	`list(dvals)`, normalizes
`pymc/backends/mcbackend.py#L57`	`list(...)`, normalizes
`pymc/printing.py#L430`	`.get()`, ignores type
`pymc/variational/opvi.py#L1174`	`.get()`, ignores type
`pymc/model_graph.py#L355`	`in` guard, then iterates
`pymc/testing.py`	`assert_equivalent_model` never reads dims

So this is a contract fix, not a crash fix.

The tests were the defect

Assertions hardcoded the round trip's output instead of comparing to the original:

assert m_new.named_vars_to_dims == {"z": ["test_dim"]}

Reference	Now
`tests/model/test_fgraph.py#L65`	`== m_old.named_vars_to_dims`
`tests/model/transform/test_conditioning.py#L136`	`== m_old.named_vars_to_dims`
`tests/model/transform/test_conditioning.py#L234`	`== m.named_vars_to_dims`
`tests/model/transform/test_conditioning.py#L241`	`== m.named_vars_to_dims`

Added tests/model/test_fgraph.py#L404, #L428, #L451, #L475, #L498,
#L520 for each isinstance branch, each dims spelling, clone_model, ModelVar
subclasses, and the unhandled fallthrough. 13 fail without the one-line change.

Incidental

pymc/model/fgraph.py#L378 raised TypeError naming the Variable, so it
reported a TensorVariable and said nothing about which Op was unrecognised.
Now names the Op.

Not fixed

pymc/model/fgraph.py#L258 reads .get(name, ()) while pymc/model/fgraph.py#L37
defaults dims=(), so omitted and explicit dims=() collapse in the graph.
Strict xfail at tests/model/test_fgraph.py#L545. Needs a sentinel or a second
prop on ModelVar, which touches __props__ and all five model_*_rv
constructors. Separately, else None to else () trades one silent loss for
another.

read-the-docs-community · 2026-10-05T16:57:12Z

Documentation build overview

📚 pymc | 🛠️ Build #34949462 | 📁 Comparing 0e663fc against latest (abaadf3)

🔍 Preview build

2 files changed

± glossary.html
± _modules/pymc/model/fgraph.html

codecov · 2026-10-05T16:57:39Z

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.95%. Comparing base (abaadf3) to head (f17b181).

Files with missing lines	Patch %	Lines
pymc/model/fgraph.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8465   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files         128      128           
  Lines       21276    21276           
=======================================
  Hits        19564    19564           
  Misses       1712     1712

Files with missing lines	Coverage Δ
pymc/model/fgraph.py	`97.87% <50.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ricardoV94

I started and then stopped. My posterior is beta(1, 0), that all new tests are overkill, and changing the assertion on the original tests suffices. We can't add 20 tests everytime we fix something like list -> tuple ...

ricardoV94 · 2026-10-06T12:22:39Z

    np.testing.assert_allclose(ip["y_cholesky-cov-packed__"], new_ip["y_cholesky-cov-packed__"])
+
+
+def test_clone_model_preserves_dims():


overkill test

ricardoV94 · 2026-10-06T12:23:02Z

+    ],
+    ids=["omitted", "single_str", "single_tuple", "two_dims"],
+)
+def test_dims_forms_roundtrip(dims, shape):


overkill test

ricardoV94 · 2026-10-06T12:23:12Z

+    "kind",
+    ["free_rv", "observed_rv", "deterministic", "potential", "data"],
+)
+def test_dims_preserved_for_every_var_kind(kind):


overkill test

add_named_variable is declared to take a tuple and was passed a list. It stores dims verbatim, so the list reached named_vars_to_dims and the round trip stopped agreeing with itself: >>> model_from_fgraph(fgraph_from_model(m)[0]).named_vars_to_dims == m.named_vars_to_dims False # before True # after Every path built on the pair inherits it, so Model.copy(), copy.copy, copy.deepcopy, pm.do and pm.observe all disagreed with their input too. The tests were part of the defect. They asserted the round trip's own output, ["test_dim"], rather than comparing to the original model, which is why this went unnoticed. They now compare against the original, and that is the assertion that catches the type. Invisible to CI because both files sit in the FAILING mypy baseline. No consumer depends on a list here: backends/arviz.py and backends/mcbackend.py normalise with list(), printing.py and variational/opvi.py ignore the type, and testing.py never reads dims. So this is a contract fix, not a crash fix.

williambdean · 2026-10-06T20:34:03Z

Agreed. Cut it down to the fix plus the assertion changes: list(op.dims) -> tuple(op.dims), and the tests now compare to the original model instead of hardcoding ['test_dim']. All 8 new tests are gone, 13 insertions / 9 deletions.

Kept out as unrelated: the dims=() vs omitted dims xfail, since ModelVar cannot tell them apart and that is a different bug. Can open it separately.

Say the word if you want the adjacent type(model_var) -> type(op) change dropped too, for a strict single-character diff.

williambdean · 2026-10-06T21:56:23Z

Thanks for the review @ricardoV94 !

A bundle is a directory holding manifest.json and graph.cloudpickle, and load returns a real pm.Model. The model and the inference data are stored separately: the bundle records a DataRef under a fixed set of keys, so a consumer finds the samples without knowing your conventions, and the draws stay readable by anything that speaks zarr. Embedded data is recorded relative to the bundle, so a directory can be copied and still find its own. save_model writes a local directory, which is the common case. The two steps are public, so build_manifest and serialize_graph hand back a dict and bytes, and the same bundle can be written to an object store, a registry, or a database row and read back through ModelBundle.from_parts. Nothing here touches a filesystem, so the package pulls in no storage library. audit() reports what will not round trip rather than letting it fail later. Three things it caught while being written: * _customdist_blockers only inspected observed RVs, so a free CustomDist with random= and no explicit logp= saved happily into a model whose compile_logp raises. pm.sample rejects that model too, so it is easy to reach by saving something never sampled. * validate() reported ok with an empty check set when no reference_logp was recorded, because ok was all({}). A bundle that verified nothing printed PASS. It now computes logp even with no reference to compare against, so ok=True always means something was checked. * cloudpickle_required promised a portability guarantee it did not give: a class defined in __main__ pickles by reference, so the flag was False for a graph that still needed its classes elsewhere. Renamed to stdlib_pickle_fails, which is what it measures. Embedding defaults to zarr. Rather than silently defaulting a missing suffix to zarr and failing later, the format is inferred from the suffix when the caller does not name one, and the error says what to do about it. test_restored_model_preserves_dims is marked xfail(strict=False): it needs pymc/model/fgraph.py to hand add_named_variable a tuple as its signature declares, fixed by pymc-devs/pymc#8465. It xpasses once that lands.

williambdean requested a review from ricardoV94 October 5, 2026 16:51

ricardoV94 requested changes Oct 6, 2026

View reviewed changes

williambdean mentioned this pull request Oct 6, 2026

feat(bundle): persist and restore a pm.Model as a bundle pymc-labs/pymc-marketing#3111

Open

4 of 6 tasks

williambdean force-pushed the use-tuple-fgraph branch from 0e663fc to f17b181 Compare October 6, 2026 20:30

ricardoV94 approved these changes Oct 6, 2026

View reviewed changes

ricardoV94 changed the title ~~Fix dims type in model_from_fgraph, and test dims preservation~~ Fix dims coercion to list in model_from_fgraph Oct 6, 2026

ricardoV94 added bug fgraph labels Oct 6, 2026

ricardoV94 merged commit 19a783f into pymc-devs:main Oct 6, 2026
42 checks passed

williambdean deleted the use-tuple-fgraph branch October 6, 2026 21:56

Navigation Menu

Repository navigation

Fix dims coercion to list in model_from_fgraph - #8465

Fix dims coercion to list in model_from_fgraph#8465
ricardoV94 merged 1 commit into
pymc-devs:mainfrom
williambdean:use-tuple-fgraph

williambdean commented Oct 5, 2026

Uh oh!

read-the-docs-community Bot commented Oct 5, 2026

Uh oh!

codecov Bot commented Oct 5, 2026 •

edited

Loading

Uh oh!

ricardoV94 left a comment

Uh oh!

ricardoV94 Oct 6, 2026

Uh oh!

ricardoV94 Oct 6, 2026

Uh oh!

ricardoV94 Oct 6, 2026

Uh oh!

williambdean commented Oct 6, 2026

Uh oh!

Uh oh!

williambdean commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		np.testing.assert_allclose(ip["y_cholesky-cov-packed__"], new_ip["y_cholesky-cov-packed__"])


		def test_clone_model_preserves_dims():

Conversation

williambdean commented Oct 5, 2026

add_named_variable is declared to take a tuple and is passed a list

Consequence: the round trip stops agreeing with itself

No consumer breaks on a list

The tests were the defect

Incidental

Not fixed

Uh oh!

read-the-docs-community Bot commented Oct 5, 2026

Documentation build overview

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ricardoV94 left a comment

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

williambdean commented Oct 6, 2026

Uh oh!

Uh oh!

williambdean commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`add_named_variable` is declared to take a tuple and is passed a list

codecov Bot commented Oct 5, 2026 •

edited

Loading