Skip to content

Issue 10 composer performance - #1149

Open
dereck-symmetry wants to merge 4 commits into
mainfrom
issue-10---Composer-performance
Open

Issue 10 composer performance#1149
dereck-symmetry wants to merge 4 commits into
mainfrom
issue-10---Composer-performance

Conversation

@dereck-symmetry

@dereck-symmetry dereck-symmetry commented Aug 16, 2026

Copy link
Copy Markdown
Contributor
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 calls compose_json_with_single_fragment for each one. That function does
json.loads(lif_record_json) then json.dumps(...) — i.e. a full serialization round-trip of
the 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
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality
    to not work as expected)
  • Documentation update
  • Infrastructure/deployment change
  • Performance improvement
  • Code refactoring
Project Area(s) Affected
  • bases/
  • components/
  • projects/
  • orchestrators/
  • frontends/
  • deployments/
  • cloudformation/ or sam/ templates
  • reference_data/
  • scripts/
  • test/ or e2e/
  • Database schema (migrations)
  • API endpoints
  • Documentation (docs/, READMEs, ARCHITECTURE.md, CLAUDE.md)
Checklist
  • commit message follows commit guidelines (see commitlint.config.mjs)
  • code passes linting checks (uv run ruff check)
  • code passes formatting checks (uv run ruff format)
  • code passes type checking (uv run ty check)
  • pre-commit hooks have been run successfully
Testing
  • Manual testing performed
  • Automated tests added/updated
  • Integration testing completed
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): LIFPerson leaves are Dict[str, Any] passthrough, and no consumer relies on JSON-primitive coercion — the sole production caller is query_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.

@bjagg bjagg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dereck-symmetry

Copy link
Copy Markdown
Contributor Author

Thanks — addressed in cb29702: the .claude/plans/ gitignore hunk is removed from this PR (the convention stays settled in #1143/#1148). Added two characterization tests pinning multi-fragment equivalence against the old sequential round-trip behavior. Re the model_dump() question: confirmed no consumer relies on JSON-primitive coercion (details added to the PR description). Re-requesting review.

@dereck-symmetry
dereck-symmetry requested a review from bjagg August 23, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

Improve Composer Performance

2 participants