Conversation
…bmodels
Inline where { constraints = ... } (and other context options) were silently
dropped for multi-output and named-output submodel calls because the generated
make_node! built the child Context without NodeCreationOptions. Mirror the
single-output path.
The 'already has functional form constraint' warnings referenced undefined locals opt and constraint_data inside lazy"...", producing UndefVarError instead of the intended informative warning. Use bound getconstraint(...).
Unused function referenced an unbound local 'ref' (latent UndefVarError).
GraphViz 0.3.0 (Graphviz_jll v15) is the current release; the `[compat]` entry pinned us to the 0.2 series. Verified on Julia 1.10, 1.11 and 1.12 that 0.3.0 resolves, the `GraphPPLGraphVizExt` extension precompiles and loads, `GraphViz.Graph(::String)` constructs and `GraphViz.load` is still an extensible generic, so `ext/GraphPPLGraphVizExt.jl` needs no changes. Full test suite passes on Julia 1.12 with 0.3.0, including the `ext/graphviz_integration_tests.jl` test item. Refs #313. Note this is a compat bump, not a fix for that report: the `UndefVarError: libgvc` there could not be reproduced here — GraphViz 0.2.0, 0.2.1 and 0.3.0 all initialize correctly on Julia 1.12. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ut submodels
Mirrors the existing "inline constraints on submodel calls" test item for both
LHS forms the fix touches: Tuple LHS `(a, b) ~ sub(...)` and NamedTuple LHS
`(a = ma, b = mb) ~ sub(...)`.
Asserts that `where { constraints = ... }` reaches the child context
(`get(context_options(inner_context), :constraints, nothing) isa Constraints`)
and that the requested factorization actually materializes on the inner nodes.
Verified the test fails on `main` (4 failures, 2 per LHS form) and passes with
the `__options__` propagation fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…warning The warning is meant to say "this node already has constraint X, so the new constraint Y will not be applied". As written it interpolated the *new* constraint into the slot for the existing one, producing "already has functional form constraint Y applied, therefore it will not be applied". Read the existing constraint from the node via `getextra` and name the skipped one, and restore the `lazy"..."` string. Also adds the regression test the PR was missing: applies a form constraint twice (marginal and message variants) and asserts a warning is emitted and the original constraint is preserved. Verified the test errors against the pre-fix `@warn` (UndefVarError: opt) and passes with the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Base.iterate` indexed by the reported (max-extent) `size` without an `isassigned` guard, so iterating a ragged or sparsely filled array raised a bare `BoundsError` pointing at an internal vector, with no hint about the real problem or the supported alternative. `iterate` now checks `isassigned` and raises an error naming the unassigned index and pointing at `GraphPPL.vec`, which already guards with `isassigned` and is the supported way to traverse ragged arrays. Note it deliberately does *not* skip unassigned slots. `ResizableArray <: AbstractArray` and there is no `length`/`IteratorSize` override, so `length == prod(size)`; skipping would break the iteration protocol and leave `collect`/`map` results holding uninitialized memory. Failing loudly is the safer contract for a package that must build factor graphs deterministically. Also guards the empty case in the 0-argument method, which previously destructured `iterate(CartesianIndices(...))` unconditionally. Closes #308. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er of LHS outputs A multi-output submodel call must provide exactly as many outputs on the LHS as there are interfaces left unspecified on the RHS. When it did not, the total arity matched no generated `make_node!` method and dispatch failed with a `MethodError` that said nothing about the real problem. Note this happens one layer earlier than #310 assumed: `prepare_interfaces_multi` is never reached, because the dispatch sites pass `static(length(rhs_interfaces) + length(lhs_interface))` and the generated methods only exist for the model's own `StaticInt{num_interfaces}`. Adds a less specific `make_node!` fallback for `Composite` nodes with `Tuple` or `NamedTuple` LHS that reports the mismatch, and updates the test that previously pinned the `MethodError` text. Closes #310. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Anonymous variables were all registered under the constant key `VariableNameAnonymous`, and `Context`'s `setindex!` is insert-or-update, so a context creating more than one anonymous variable kept only the last in `individual_variables`. The graph itself was correct, but anything enumerating the context registry (notably `VarDict`) silently missed the earlier ones. Adds `add_anonymous_node!`, mirroring `add_constant_node!`, which registers under `to_symbol(VariableNameAnonymous, label.global_counter)`. The node property `name` is unchanged, so `is_anonymous` and `as_variable(VariableNameAnonymous)` are unaffected. Closes #311. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keyword arguments in a `@model` function signature were parsed and then silently dropped, with only a `@warn`. The body never saw them, so a user writing `@model function m(x; a = 1)` got no `a` and a later `UndefVarError` as the only real signal, and the warning is easy to miss in a noisy REPL. Raise an error at macro expansion instead, consistent with unsupported positional arguments at the call site, and name the offending keyword arguments plus the supported form. BREAKING: `@model` definitions carrying keyword arguments now fail to expand rather than silently ignoring them. Nothing in this repository relied on the old behaviour: no test or doc asserted the warning, and no `@model` signature with keyword arguments exists in `src/`, `test/`, `docs/` or `README.md`. Downstream packages that defined such signatures will need to drop them, but their keyword arguments were never in effect to begin with. Closes #307. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ents `combine_broadcast_args(args::Vector, kwargs::Vector)` built both halves of the `MixedArguments` wrongly, not just the keyword half as #309 reports: - the positional half spliced the original argument expressions, which refer to the outer, un-broadcast collections rather than the per-element slots; - the keyword half called `NamedTuple{keys}(args)` on the *whole* broadcast closure tuple, whose arity is `length(positional) + length(keywords)`. Inside the broadcasted expression `args` is the varargs tail of the closure, holding positional slices first and keyword values after, so both halves are now sliced out of it by index. They are emitted as a tuple literal and a named-tuple literal, mirroring the non-broadcast `combine_args`, so `proxy_args` wraps each element individually and the halves stay a `Tuple` and a `NamedTuple` as `MixedArguments{A <: Tuple, K <: NamedTuple}` requires. Updates the two tests that pinned the buggy generated shape. Closes #309. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three changes to the workflows ahead of the 4.9.0 batch: - Extend the test matrix to Julia 1.12 and 1.13. 1.12 in particular exercises the GraphViz extension load path reported in #313, which was previously never covered by CI. - Add `opened` to the `pull_request` trigger types. It was missing, so a pull request opened non-draft and never pushed to again received no CI at all -- #315 sat for six weeks with zero runs because of this, and #322-#327 each needed a manual `workflow_dispatch`. - Add a concurrency group so a new run supersedes one still in flight for the same ref. `main` is exempt, since its runs are the record of what actually passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
compat: allow GraphViz 0.3
fix: remove dead `variableref_checked_collection_typeof` (unbound `ref`)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
==========================================
+ Coverage 90.72% 91.04% +0.32%
==========================================
Files 16 16
Lines 2263 2301 +38
==========================================
+ Hits 2053 2095 +42
+ Misses 210 206 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fix: repair duplicate form-constraint `@warn` (UndefVarError `opt`)
fix: propagate `__options__` to child Context for multi-/named-output submodels
…error fix: throw a descriptive error when iterating a sparse `ResizableArray`
fix: clear error when a multi-output submodel call has the wrong number of LHS outputs
fix: register each anonymous variable under a unique context key
…xed-args # Conflicts: # test/graph_construction_tests.jl
fix!: reject keyword arguments in `@model` signatures instead of warning
This was referenced Sep 21, 2026
fix: correct the lowering of mixed positional/keyword broadcast arguments
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 21, 2026
This was referenced Sep 21, 2026
@warn in apply_constraints! throws UndefVarError: opt when a node already has a form constraint
#305
Open
bvdmitri
marked this pull request as ready for review
September 21, 2026 08:24
… b = c)` In Julia `f(a, b = c)` and `f(a; b = c)` are the same call. Inside a `@model` body they were not: only the semicolon form worked, and the comma form failed during macro expansion with `syntax: invalid named tuple element ...`, pointing at generated code rather than at the user's line. `convert_to_kwargs_expression` was all-or-nothing -- it rewrote a call into the keyword form only when `is_kwargs_expression(args)` held, which requires *every* argument to be `:kw`/`:parameters`. Anything mixed fell through untouched, so the `:kw` node stayed among the positional arguments and `combine_args` emitted a tuple literal containing it, which is not valid syntax. Both spellings are now normalized to the same `(positional, keywords)` split. The trap here is that Julia puts an explicit `;` group *first*, in a `:parameters` node, so a naive "split off the trailing keywords" breaks calls like `Normal(0, 1; a = 1, b = 2)`; `split_positional_and_keyword_args` handles both shapes, and rebuilding an already-correct semicolon call reproduces it exactly. The same normalization is applied in `convert_to_anonymous`, which runs *after* `convert_to_kwargs_expression` in the pipeline and generates fresh tilde expressions for nested calls -- splicing the captured arguments back verbatim reintroduced the comma form there. This is the path the original report hit: a nested deterministic call such as `mixed_det(1.0, s = 2.0)` now builds. What this does and does not buy: - A nested deterministic call over constants is evaluated directly, so mixed arguments genuinely work there. That is the shape reported in the issue. - A node that has to be materialized still cannot take both, by design. It now reaches that stated limitation instead of failing as invalid syntax. The materialized-node error is also rewritten. It said "MixedArguments not supported for rhs_interfaces when node has to be materialized" -- `rhs_interfaces` means nothing to someone writing a model, and this change makes the error easier to reach. It now names the function, reports what it got, and says what to do. Tests assert the contract directly: the comma and semicolon forms are indistinguishable, across `~`, `.~` and `:=`, including the `Normal(0, 1; a = 1, b = 2)` case a naive fix would break. Closes #328 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`makedocs` runs with `missing_docs` as an error, so every docstring in the module has to appear in a canonical `@docs` block. The three new helpers had docstrings but no entry, which failed the Documentation job. Listed next to `is_kwargs_expression` and `convert_to_kwargs_expression`, which is where the rest of the model macro pipeline lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Building the docs required `Pkg.develop(PackageSpec(path=pwd()))` first, which writes an absolute, machine-specific path into `docs/Project.toml`. A `[sources]` entry pointing at `".."` does the same job, is portable, and makes `julia --project=docs docs/make.jl` work directly from a fresh checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix: split mixed positional/keyword arguments in the comma form `f(a, b = c)`
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Collects the batch of fixes from @docxology's August 2026 audit, together with the CI changes
needed to land them, into a single release branch.
Issues closed
Closes #304 — multi-/named-output submodels silently ignored
where { ... }optionsCloses #305 —
UndefVarError: optin the duplicate form-constraint@warnCloses #306 — dead
variableref_checked_collection_typeofreferencing an unboundrefCloses #307 —
@modelkeyword arguments are now rejected instead of silently droppedCloses #308 — iterating a ragged/sparse
ResizableArraynow errors descriptivelyCloses #309 — broadcast lowering of mixed positional/keyword arguments
Closes #310 — clear error when a multi-output submodel call has the wrong LHS arity
Closes #311 — multiple anonymous variables in one context no longer collapse to one key
Closes #328 — the comma form
f(a, b = c)is split into keyword arguments(Closing keywords are listed here rather than relying on the individual pull requests, since
GitHub only fires them on merge into the default branch.)
Deliberately not closed
release.
#313 (GraphViz
libgvcInitError) was closed separately as not reproducible — GraphViz0.2.0, 0.2.1 and 0.3.0 all initialize correctly on Julia 1.12, and the full suite passes there.
The compat bump in #322 lands on its own merits, not as a fix for it. The reporter's underlying
point was still valid, though, and is addressed here: the extension was never exercised on 1.12
by CI, and now is.
Breaking
#307 / #326: keyword arguments in an
@modelsignature now raise an error. Previously theywere parsed, silently dropped, and warned about, so the only real signal was a later
UndefVarErrorin the body. This is the reason for a minor rather than a patch bump.CI changes
openedadded to thepull_requesttrigger types — it was missing, so a pull request openednon-draft and never pushed to again received no CI at all
CIandTest dependent packagesso superseded runs are cancelled,with
mainexemptIncluded pull requests
#314, #315, #316 (@docxology), and #322, #323, #324, #325, #326, #327, #330.
Supersedes #319 and #320 (CompatHelper duplicates of #322).
🤖 Generated with Claude Code