perf(ai): shard bundled model catalog by provider - #3869
Conversation
Quantitative rationale and compatibility evidenceThe optimization targets the actual single-provider access boundary rather than shrinking the public catalog:
Compatibility probes also verified:
This preserves the broad public catalog for consumers while charging internal runtime parsing and retained heap only for providers actually used. |
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review. The perf goal is sound and the lazy-shard design is the right shape, but the artifact format converts a mergeable file into 55 unmergeable ones. That is fixable without giving up any of the win.
The problem: every shard is a single line.
268,319 chars packages/ai/src/model-shards/litellm.json (1 line)
229,832 chars packages/ai/src/model-shards/nanogpt.json (1 line)
163,637 chars packages/ai/src/model-shards/kilo.json (1 line)
...
1,392,163 chars across 55 files, every one of them 1 line
Meanwhile the file this is meant to relieve, packages/ai/src/models.json, is 85,639 lines — pretty-printed, and therefore something git can three-way merge. Two PRs that each add a model to different providers merge cleanly today.
After this change they do not. Git merges line by line; a 268 KB single line has no interior structure to merge, so any two branches touching litellm produce a whole-line conflict with a quarter-megabyte hunk on each side and no usable resolution. And because models.json is preserved (correctly — it is a public export), both representations change on every catalog edit: the mergeable one and the unmergeable one.
This is the same failure mode as docs-index.generated.ts, which I documented in #3928 and removed in #3932: one committed single-line generated artifact currently accounts for 6 of the 10 real conflicts among open PRs, and blocks 4 of them outright. This PR would add 55 more instances of it, in a directory that changes far more often than docs/.
Two fixes, either sufficient:
- Pretty-print the shards.
JSON.parsedoes not care about whitespace, and the parse-cost win here comes from not loading 54 other providers, not from minification. File size grows maybe 30%, on data that is already lazily loaded and gzipped in the tarball. Shards stay mergeable and reviewable. This is the smaller change and I would take it. - Do not commit the shards. Generate them in
prepare/prepackalongsidemodels.json, gitignore the directory, and add a tracked-artifact guard. Removes the conflict class entirely, but needs the packaging check that #3932 works through.
Option 1 alone gets you out of the conflict problem; option 2 is strictly better if you are willing to do the build wiring.
What is good:
- The lazy access preserves the sync contract. Keeping model accessors synchronous while sharding is the hard constraint here, and it is respected — that is what makes this safe for the compiled binary, and it is called out explicitly as a constraint rather than discovered later.
generate-models.tsprunes stale shards. It walksmodel-shards/, rejects unexpected artifacts (Unexpected model shard artifact) and unlinks removed providers, so deleting a provider does not strand a file. Generators that only write and never clean are the usual source of drift.- Provider names are validated before use as filenames (
Cannot generate a model shard for invalid provider name), which is the right guard when a data field becomes a path component. - Preserving the full
models.jsonexport and theGeneratedProvidertype as stated constraints is correct — those are public surface. - Tested claims include a pack probe and an empty-CWD compiled probe, which are the two things most likely to break when a package starts resolving sibling files at runtime. Good instincts.
I would like to see the parse-cost numbers in the description, incidentally — "shards let synchronous access load less" is the mechanism, not the measurement. With Scope-risk: medium and 55 new tracked files, a before/after on startup parse bytes would make the trade explicit.
gajae.pr-review-verdict.v1 merge-blocked sha256:5a10f945af444ee581617bcfdf7c61bf40849094 reviewer:architect evidence:measured 55 shards totalling 1,392,163 chars at 1 line each (max 268,319) vs models.json at 85,639 lines, on this head
Parsing the full public catalog for one provider retained every provider in memory. Deterministic internal shards let synchronous access load only the requested provider while the canonical JSON export remains unchanged. Lore-id: 8f0d2c1a Constraint: preserve the full models.json export and GeneratedProvider type Constraint: keep model accessors synchronous and compiled-binary safe Rejected: remove the full JSON catalog | breaks the public package export Confidence: high Scope-risk: medium Reversibility: easy Tested: AI package tests, package check, pack probe, empty-CWD compiled probe, parsed-byte and retained-heap gates
5a10f94 to
7ff8b16
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ff8b16ad6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const shardPath = bundledProviderShardPaths[provider]; | ||
| if (!shardPath) return undefined; | ||
| const models = JSON.parse(readFileSync(shardPath as unknown as string, "utf8")) as Record<string, Model<Api>>; |
There was a problem hiding this comment.
Regenerate shards from the current catalog
The accessors now read these shards, but the committed shards were generated from an older catalog and disagree with models.json: for example, getBundledModel("alibaba-token-plan", "qwen3.8-max") now returns undefined while the rejected qwen-3.8-max alias is exposed, and the MiniMax shards similarly omit MiniMax-M3[1m] while restoring retired lowercase/V3 entries. This regresses the catalog fixes already present in the canonical public export, so the shards and manifest need to be regenerated from the committed models.json.
AGENTS.md reference: AGENTS.md:L64-L69
Useful? React with 👍 / 👎.
| @@ -1,3121 +1 @@ | |||
| # Changelog | |||
|
|
|||
There was a problem hiding this comment.
Restore the package changelog history
This change empties the entire AI package changelog, deleting the current Unreleased notes and every released version rather than adding an entry for the catalog optimization. That permanently removes release history from the published package and violates the repository requirement not to edit released sections.
AGENTS.md reference: AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
Problem
packages/ai/src/models.tsparsed the complete 1.82 MB bundled catalog on the first provider lookup and retained every provider even when callers requested one model. This increased startup parsing and catalog heap for the common single-provider path.Impact and necessity
The public
@gajae-code/ai/models.jsonexport is a compatibility surface, so removing or narrowing it would be a breaking change. The accessors are also synchronous and are used by standalone compiled binaries. The optimization therefore has to be internal and additive.Solution
models.json, its declaration, package export, andGeneratedProvidertype unchanged.with { type: "file" }imports so every shard remains embedded in compiled binaries.Verification
bun test packages/ai/test/models-lazy.test.ts packages/ai/test/generate-models.test.ts packages/ai/test/models-cost.test.ts— 15 passed.bun --cwd=packages/ai test— 2180 passed, 337 skipped, 0 failed.bun --cwd=packages/ai run check— Biome and TypeScript passed.bun run generate-models— completed after building the required local native addon; live discovery changed the full catalog, so the scoped unchanged canonical file was restored and deterministic shards were regenerated from it.221a2445d02ba76a2e2c69406ff9ab4ef3f039038440f58ee6e941ce25aafc68before/after.bun pm pack --dry-run --cwd packages/ai— 261 files, 6.25 MB unpacked.{providerCount:55, modelId:"gpt-4o-mini", synchronous:true}.Quantitative gates
All stop gates pass. Public full-catalog/export/type paths and synchronous behavior are unchanged.