Issue 10 composer performance - #1149
Conversation
bjagg
left a comment
There was a problem hiding this comment.
Overview
Replaces the per-fragment JSON round-trip in the composer with direct dict mutation: one parse at the boundary, one dump at the end. Small, focused, and the diagnosis in the PR body is correct — compose_json_with_fragment_list was calling compose_json_with_single_fragment per fragment, and each call did a full json.loads + json.dumps of the entire growing record. That is quadratic in the number of fragments, and on a wide record it is the dominant cost.
Verified the change is behavior-preserving in the way that matters most here: LIFRecord declares person: LIFPerson = Field(..., alias="Person") with populate_by_name=True (components/lif/datatypes/core.py:69-72), and both model_dump_json() and model_dump() default to by_alias=False. So the key casing fed back into LIFRecord(**dict) is unchanged — no PascalCase/camelCase regression, which is the thing I most expected to find.
Demoting the per-fragment logger.info to debug is a sensible companion change; that line fired once per fragment.
One question
compose_with_single_fragment / compose_with_fragment_list switch from model_dump_json() → json.loads() to plain model_dump(). The old path forced every value through JSON primitives; the new one keeps native Python objects. For the current LIFRecord shape (Dict[str, Any] leaves sourced from Mongo) that appears equivalent, and the results go straight back into LIFRecord(**dict) rather than to json.dumps, so validation still runs.
Worth confirming explicitly: is any consumer relying on the old JSON-primitive coercion — a datetime arriving as an ISO string rather than a datetime object, for instance? If not, this is fine as-is and worth a one-line note in the PR body so the next reader doesn't have to re-derive it.
Test coverage
test/components/lif/composer/test_core.py has 11 existing tests and this PR touches none of them — reasonable for a refactor whose contract is unchanged, and they should cover the behavior. Two additions worth considering:
- A fragment-list case asserting the composed output matches the old implementation's for a multi-fragment record (the actual regression risk).
- Given the stated motivation is complexity, a test with a meaningful number of fragments would pin the win. Not a blocker.
Unrelated change
The .gitignore addition of .claude/plans/ ("never committed") conflicts with a decision already on main: .claude/plans/772-import-transformation-groups.md is tracked, and is Chris Beach's KT artifact for #772 — corrected and merged in #1143 on 2026-08-10. Adding the directory to .gitignore won't untrack that file, but it establishes a convention that contradicts it.
The same hunk appears in #1148, where it does delete that file. Worth settling the convention in one place rather than in three PRs — and worth dropping from this PR either way, since it has nothing to do with composer performance.
Verdict
Approve, with the .gitignore hunk removed and ideally a note on the model_dump change. The core refactor is correct and the reasoning behind it is sound.
…gment equivalence tests
|
Thanks — addressed in cb29702: the |
Description of Change
Previous (current) composer does the composition of each json fragment as it comes back. This means that the time grows on a quadratic. Specifically,
compose_json_with_fragment_list(components/lif/composer/core.py) loops over fragments and callscompose_json_with_single_fragmentfor each one. That function doesjson.loads(lif_record_json)thenjson.dumps(...)— i.e. a full serialization round-trip ofthe entire (growing) record per fragment.
Fix - compose directly on the dict, one parse / one model-dump at the boundary, zero intermediate
JSON. The public API is unchanged — all four functions keep their signatures and return types.
Closes #10
Type of Change
to not work as expected)
Project Area(s) Affected
Checklist
uv run ruff check)uv run ruff format)uv run ty check)Testing
Additional Notes
This change is deceptively small. I worked with Claude until we were able to get the least invasive procedure completed. WE MUST DO EXTENSIVELY end-to-end testing when this change is rolled into main. There shouldn't be any side effects, and I have tested extensively manually and with automated tools.
We should have LLM do additional testing and code verification on this small change after roll into main. There is, though, no suspicions that this will in any way fail.
Note on
model_dump_json()→model_dump()(per review):LIFPersonleaves areDict[str, Any]passthrough, and no consumer relies on JSON-primitive coercion — the sole production caller isquery_cache_service.save(), which writes composed records back to Mongo via$set; its inputs (JSONata transform fragments, API payloads) are JSON-bound, and the read path returns raw documents without re-serialization.