Tier 1 fixes for #1954 (variable composition review feedback)#1962
Merged
dmontagu merged 13 commits intoMay 25, 2026
Merged
Conversation
Addresses four critical / important items Alex flagged on #1951 (now folded into #1954): ## 1. Transitive reachability in the reference walker `_check_reference_errors` previously only seeded its ref graph from locally-registered variables. References reached transitively through server-only fragments (the value is in `VariablesConfig` but the variable isn't declared in code) were never followed, so cycles and missing-ref errors that pass through a server-only node went silent. Refactored to walk the full graph from each local variable with a FIFO queue: every name visited contributes its outgoing edges to the graph, and any name not already in `local ∪ server_config.variables` is reported as `'@{name}@ does not exist'`. Treats `VariablesConfig` as the self-contained source of truth for substitution, matching the runtime behaviour. Alex r3288517914. Two new tests in `test_push_variables.py`: `test_compute_diff_reference_errors_through_server_only_chain` and `test_compute_diff_detects_cycle_through_server_only_chain`. ## 2. Top-level context-override fast path The #1951 unification of override / provider / code-default through `_lookup_serialized` introduced a `dump_json` / `validate_json` round-trip on the override path. Unserializable overrides (e.g. `var.override(some_object)` where `some_object` doesn't survive JSON) silently fell through to the provider / code default — a behavioural regression vs. the pre-#1951 typed-Python return. Added `Variable._resolve_context_override` and route to it from the top of `_resolve` before `_lookup_serialized` is consulted: serialize when possible (so `render_fn` can run for TemplateVariable) and return the value verbatim otherwise. The `_lookup_serialized` override branch stays in place for *composition-reference* overrides — composition substitutes into strings, so serialization is unavoidable there; falling through on failure remains the right call. Alex r3287492856 / r3289439477, Devin r3286255835. New test in `test_variables.py::TestVariableContext`: `test_override_unserializable_value_returned_typed`. ## 3. Escape-only composition consistency `_expand_and_deserialize` short-circuited on `has_references(value)` being false. The `has_references` regex requires an *unescaped* `@{`, so a value containing only `\@{baz}@` produced no refs, was returned unchanged, and kept its backslash. The same escape combined with a real `@{ref}@` ran through composition (which *does* unescape) and produced literal `@{baz}@`. Observable behaviour depended on whether another reference was present. Dropped the short-circuit. `expand_references()` already handles the no-refs case efficiently (early-return after `_unescape_serialized`), so both code paths now route through the same unescape logic. Alex r3288986490. New test in `test_variable_composition.py::TestCompositionIntegration`: `test_escape_only_value_is_unescaped_consistently`. ## 4. `template_var()` type inference `template_var()` required an explicit `type=` argument even when `default` was a non-callable. `var()` infers in that case; the inconsistency tripped Alex when authoring docs. Added a no-`type=` overload mirroring `var()`'s pattern, with the same runtime check that a resolve-function `default` still requires an explicit `type=`. Alex r3287772951. ## Test count 442 variable / composition / template / push tests pass; pyright + ruff format + ruff check clean.
`_check_reference_errors`'s `missing_reported` dedup is unreachable — each `(current, ref)` pair is unique per walk because `seen` gates re-visiting `current` and `refs` is a set, so the same edge is never yielded twice. Coverage flagged the branch and removing it simplifies the loop body. Add two test cases that exercise the new `template_var()` no-`type=` overload: type inference from a non-callable default and the resolve-function `TypeError` guard. These cover lines 2702–2707 in `_internal/main.py`.
Six items Alex flagged as important-but-not-blocking on the original 1951/1952 stack, folded in before #1954 merges: ## Docs - `composition.py` module docstring + `expand_references` docstring no longer claim block helpers are restricted to top-level identifiers — the native handlebars path accepts `@{#if user.active}@` and helper sub-expression headers. - `docs/.../templates-and-composition.md` Control Flow section rewritten to match: drops the "must be top-level" caveat and adds rows for dotted-path conditions and `../` parent-scope access from inside a block. ## Caching - New `compile_composition_template(source)` in `_handlebars` wraps the parse step in an `lru_cache(maxsize=1024)` (#1952 r3289095058). `reference_syntax.render_once` now compiles once per distinct source and reuses the compiled program across resolutions — managed-variable values are typically stable, so the hit rate should be high. - `extract_composition_dependencies` no longer re-runs the `try: from pydantic_handlebars import …` block on every call — the import is moved into a `@cache`d helper that returns the function object directly. Matches the pattern of the existing `_get_template_compatibility_checker` (#1952 r3288194502). ## Warning text - `_composition_failure` renamed to `_fallback_to_default` and takes a `failure_stage` argument (`'composition'` or `'template rendering'`) so the `RuntimeWarning` text reflects the actual failed step. A pydantic-handlebars parse error during `{{...}}` rendering used to surface as `"composition failed"` even though composition succeeded (codex finding). - Updated `test_remote_render_error_records_exception` accordingly. ## Test naming - `test_override_render_failure_falls_back` renamed its locals: `bad_override` → `templated_config` (it's a *valid* template), and introduced `invalid_inputs` for the inputs that actually fail the pattern constraint. Alex r3289312130. ## Escape-detection coupling - Added a comment on the `_HAS_REFERENCE` regex flagging that the lookbehind encodes pydantic-handlebars' current escape semantics ("any preceding `\` escapes") and noting it'll need to count preceding backslashes if pydantic-handlebars adopts Handlebars.js's odd-vs-even-backslash spec behaviour (#1952 r3289062247).
alexmojaki
reviewed
May 24, 2026
alexmojaki
reviewed
May 24, 2026
alexmojaki
reviewed
May 24, 2026
Four items from Alex (r3295454297 / r3295457538 / r3295460481 /
r3295461517):
## Collapse the repeated try/except import dance
Four functions (`get_environment`, `get_handlebars_renderer`,
`_extract_dependencies_fn`, `_get_template_compatibility_checker`) each
carried their own 5-line `try: from pydantic_handlebars import …
except ModuleNotFoundError as exc: if exc.name == … raise … from exc`
block. Replaced with a single cached `_pydantic_handlebars()` accessor
that imports the *whole module* once and returns it; every other
function reaches in via attribute access. Python's own import machinery
caches modules — wrapping each attribute import in a separate
`@cache`'d helper was dead weight Alex called out as eye-hurting.
## Drop `pragma: no cover`
With the dep-error path consolidated into one branch, replaced the
subprocess-only `test_import_logfire_without_pydantic_handlebars` (which
runs under a subprocess and so doesn't contribute to coverage) with an
in-process companion `test_missing_handlebars_raises_helpful_error` that
clears the `_pydantic_handlebars` cache, blocks the import via
`__import__` monkeypatch, and asserts the error. Subprocess test stays
as the integration-level guarantee.
Also removed the defensive `exc.name == 'pydantic_handlebars'` discriminator
— pydantic-handlebars is a small package with minimal deps, and a
transitive ModuleNotFoundError there is pathological enough that surfacing
the dep-error message is acceptable. That eliminates the only branch
that genuinely couldn't be tested.
## LRU-cache runtime `{{...}}` template compilation
New `compile_runtime_template(source)` mirrors
`compile_composition_template(source)`, both `@lru_cache(maxsize=1024)`.
`render_serialized_string` in `abstract.py` now uses it instead of the
module-level `pydantic_handlebars.render`, which re-parses every call.
`get_safe_string_cls()` replaces the SafeString half of the old
`get_handlebars_renderer` return tuple — `_wrap_safe_context` calls it
once per render.
## LRU-cache extract_composition_dependencies
Promoted `extract_composition_dependencies` itself to
`@lru_cache(maxsize=1024)`. Cycle and reference validation walk the
same template strings repeatedly during push / sync.
Coverage on `logfire/variables/_handlebars.py` is now 100% line + branch.
445 variable / composition / template / push tests pass.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Cubic r3295580826: dropping the `exc.name == 'pydantic_handlebars'` check meant a missing transitive dependency would get reframed as "install pydantic-handlebars", hiding the real failure. Valid concern. Restored the check. Also added a new in-process test `test_unrelated_module_not_found_propagates` that monkeypatches `__import__` to raise `ModuleNotFoundError(name='some_other_dep')` from inside `pydantic_handlebars` and asserts the raw error propagates (not wrapped as `HandlebarsDependencyError`). Both branches of the discriminator are now exercised in-process and under coverage, so the file stays at 100% line + branch with no `# pragma: no cover` markers.
The "Cycle Detection" section only mentioned push-time validation — but
the runtime resolution model is the bit that differs most from plain
Handlebars and is worth calling out explicitly. Replaced with two
sections:
- **Recursive Resolution** — composition isn't a one-shot substitution;
referenced variables go through full resolution (including their own
`@{...}@`) before substitution. Shows a `parent → middle → leaf`
example and notes that `composed_from` mirrors the tree.
- **Cycle and depth guards** — describes the two-layer defence:
`variables_validate` / `push(strict=True)` at sync time (catches
cycles whose midpoints are server-only too), and `MAX_COMPOSITION_DEPTH`
/ visited-set at runtime with fall-back to code default + `RuntimeWarning`.
Calls out that an unresolvable `@{ref}@` is treated as a real failure
(unlike a missing `{{field}}` which silently substitutes empty), and
includes a snippet showing how to detect via
`resolved.exception` / `resolved.reason`.
The previous wording mentioned the difference at the end as a "main
semantic difference" line. Promote it: lead with a warning admonition
that spells out the plain-Handlebars behaviour (one-shot string
substitution; inner {{...}} not re-rendered) before showing what
composition does instead. Add a contrast snippet using
`pydantic_handlebars.render` directly to make the difference
concrete — `{{greeting}}` substituting a `'Hello, {{name}}!'` value
emits `'Hello, {{name}}!'` verbatim, no recursive render.
This is a common Handlebars-newcomer footgun; calling it out
explicitly should save people the "wait why isn't this working" loop.
Two issues David called out:
1. The "trips up people coming from normal Handlebars" wording was
presumptuous — this is a new SDK with new users; there's no
pre-existing audience to "trip up". Removed the line; the prose
above it already explains the difference plainly.
2. The composition / template-and-composition section had five
`skip="true"` examples. Skipping is how docs rot — converted all
five to runnable snippets with `#>` output assertions so
pytest-examples actually executes them:
- Composition Tracking — uses a small `report` / `city` pair to
show `composed_from` entries with `name`, `value`, `reason`.
- Combining Templates and Composition — minor reshape to use
`#>` output instead of trailing comments; runs end-to-end.
- Recursive Resolution — `parent → middle → leaf` example now
runs, and prints the `composed_from` chain shape.
- Plain-Handlebars contrast — direct `pydantic_handlebars.render`
call demonstrating that `{{name}}` inside a value is not
re-rendered.
- Cycle-and-depth runtime guard — registers a small
`cycle_left` / `cycle_right` pair, calls `get()`, asserts
`resolved.reason == 'other_error'` and the warning fires.
All 8 snippets in the page now run under `pytest tests/test_docs.py -k
templates-and-composition`. mkdocs build clean.
`from collections import deque` (stdlib), `from pydantic import
TypeAdapter, ValidationError` (hard dep, not circular), and
`from logfire.variables._handlebars import compile_runtime_template,
get_safe_string_cls` (`_handlebars.py` doesn't import abstract) all
moved to module-level. There was no good reason for them to be lazy —
no circular import, no optional dependency at the call site, no
startup-cost concern. Local-imports-in-function-bodies are a cost in
readability; pay it only when there's a real reason.
The remaining local imports are intentional: every
`from logfire.variables.{composition,config,variable} import …` is
inside a function because those modules import `from
logfire.variables.abstract import …` at their top, so hoisting them
would create an import cycle. `import logfire` in
`ResolvedVariable.__enter__` is similarly circular. Left alone.
Net: -7 redundant in-function import lines, no behaviour change. 447
variable / composition / template / push tests pass; pyright + ruff
clean.
Two places were silent on how `Variable.override(...)` interacts with composition and template rendering — important behaviour to spell out, not least because the pre-#1962 silent-fallthrough bug was visible to users without any docs to refer to. ## `override()` docstring (`variable.py`) Expanded the one-sentence summary into three explicit sections: - **Composition is skipped** — overriding with `'hi @{user}@'` yields the literal string; `@{user}@` is not expanded. Reasoning: overrides are the user's literal choice, not a template for the composition graph to resolve. - **Template rendering still applies to `TemplateVariable`** — as long as the override value is JSON-serializable, `{{...}}` rendering against the runtime `inputs` happens the same way as for a provider value. - **Unserializable overrides come back verbatim** — no serialize/deserialize round-trip, no render pass; matches the "literal user choice" intent (and is the pre-#1951 behaviour restored). ## User docs (`configuration-reference.md`) Added a `## Overrides and composition` subsection under "Contextual Overrides" with the same two-bullet summary and a runnable snippet demonstrating both states — composition expanding `@{user}@` from the default vs. override returning `'Hi @{user}@'` literally. Runs under `pytest tests/test_docs.py`.
The hoist of `from pydantic import TypeAdapter, ValidationError` to module scope in 1d3504a broke `import logfire` on Pyodide. Pydantic is only pulled in by the `[variables]` extra, not base logfire, so `abstract.py` must be importable without it. The pyodide_test harness (`pyodide_test/test.mjs`) does exactly that — `await micropip.install(['file:logfire-…whl'])` followed by `import logfire` with no pydantic — and that import failed at the top-level `from pydantic import TypeAdapter, ValidationError`. Reverted: - `TypeAdapter` back to TYPE_CHECKING (annotation-only at module scope; `from __future__ import annotations` keeps it string-only at runtime) and a local import in `push_variable_types` where it's called at runtime. - `ValidationError` back to function-local imports in `_check_label_compatibility` and `_check_type_label_compatibility`. - Added a short comment on the TYPE_CHECKING block explaining why pydantic stays lazy. `deque` and the `_handlebars` accessors stay at module scope — `collections` is stdlib, and `_handlebars.py` is import-safe without pydantic-handlebars (the try/except at its top handles that). Verified locally: `cd pyodide_test && node --experimental-wasm-stack-switching test.mjs` passes (`Logfire Pyodide tests passed 🎉`).
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
dmontagu
merged commit de12509
into
feature/variable-composition-native-handlebars
16 checks passed
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.
Stacked on #1954. Four correctness fixes addressing review feedback Alex left on #1951 that's now folded into #1954.
1. Transitive reachability in the reference walker (r3288517914)
_check_reference_errorsonly iterated locally-registered variables when seeding the ref graph, so a missing reference reachable only via composition into a server-only fragment was never reported, and a cycle whose midpoints were server-only went silent. Refactored to walk the full graph with a FIFO queue from each local variable, pulling values out ofVariablesConfigfor any name reached.VariablesConfigis treated as the self-contained source of truth for substitution —@{name}@whosenameis in neither local registration norserver_configis the same kind of error as a name in nothing at all.Two regression tests in
test_push_variables.py:test_compute_diff_reference_errors_through_server_only_chain— Alex's exact repro: localfoo2→ server-onlyfoo1→ missingfoo3.test_compute_diff_detects_cycle_through_server_only_chain—foo→ server-onlyserver_a→ server-onlyserver_b→foo.2. Top-level context-override fast path (r3287492856 / r3289439477 / Devin r3286255835)
#1951 unified override / provider / code-default through
_lookup_serialized, which added adump_json/validate_jsonround trip on the override path. Unserializable overrides (e.g.var.override(some_object)with a non-JSON-friendly value) silently fell through to provider / code default. Inconsistent with the pre-#1951 typed-Python return, and inconsistent within the SDK itself — unserializable code defaults still take effect.Added
Variable._resolve_context_overrideand route to it from the top of_resolvebefore_lookup_serializedis consulted. Tries serialization (sorender_fncan run forTemplateVariable); on failure, returns the user's value verbatim. The_lookup_serializedoverride branch stays in place for composition-reference overrides — composition substitutes into strings, so serialization is unavoidable there, and falling through on failure remains the right call per the design discussion.New test in
test_variables.py::TestVariableContext::test_override_unserializable_value_returned_typed.3. Escape-only composition consistency (r3288986490)
_expand_and_deserializeshort-circuited onhas_references(value). The regex requires unescaped@{, so a value containing only\@{baz}@produced no refs, was returned unchanged with its backslash; the same escape combined with a real@{ref}@ran through composition (which does unescape) and produced literal@{baz}@. Behaviour depended on whether another reference was present.Dropped the short-circuit.
expand_references()already handles the no-refs case efficiently (early-return after_unescape_serialized), so both code paths now route through the same unescape logic.New test in
test_variable_composition.py::TestCompositionIntegration::test_escape_only_value_is_unescaped_consistently.4.
template_var()type inference (r3287772951)template_var()required an explicittype=even whendefaultwas a non-callable.var()infers in that case; the inconsistency was easy to trip on when authoring docs. Added a no-type=overload mirroringvar()'s pattern, with the same runtime check that a resolve-functiondefaultstill requires an explicittype=.Verification
442 variable / composition / template / push tests pass. Pyright + ruff format + ruff check clean.
Recommend merging this into #1954 (or once #1954 lands, retargeting to main) before continuing with Tier 2 (push-time validation improvements — local-default-vs-latest split,
LabelRefreporting) and Tier 3 (#1958 docstring exception-name fixes).