Repository navigation
Bump Laya: source 3a38cbe8, model 85cc74dd - #2216
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at ef34aea — the body says two pins move and the diff moves one.
The defect
"
third_party/laya:b8a0f0991d95->3a38cbe83f6e(NandhaKishorM/laya main). config/laya.json revision:85cc74ddae61-> [new]"
Measured:
git diff --name-only origin/main...HEAD → third_party/laya (1 file)
third_party/laya at this head 3a38cbe83f6ef556…
config/laya.json at this head revision 85cc74ddae610907… ← UNCHANGED
config/laya.json on main revision 85cc74ddae610907… ← same
So either the body is wrong about the second pin, or the hunk is missing and the bump lands with a config pointing at the old revision. One of the two must be fixed, and the PR does not say which.
Why it matters here specifically, and it is not a style point
config/laya.json's revision is a different repo: "repo": "convaiinnovations/laya" (HF checkpoints), while third_party/laya is NandhaKishorM/laya (source). Two pins, two repos, one PR body claiming both moved — this is exactly the shape I reviewed on #1709 ("two pins, two repos"), and .gitmodules states the contract in its own words: "The model checkpoints are pinned by Hugging Face revision in config/laya.json … and .github/workflows/bump-laya.yml keeps both current."
And the workflow that is supposed to keep both current edits both:
git update-index --cacheinfo "160000,$SRC,third_party/laya" # line 88
p = json.load(open("config/laya.json")); p["revision"] = sys.argv[1] # line 91-92So the automated path would have produced a two-file diff. This PR produced one — which means it was made by hand (fine) or the config hunk was dropped (not fine). Either way, a reader cannot tell whether 85cc74ddae61 is still the right checkpoint revision for source 3a38cbe8, and that is the question the bump exists to answer.
What I could not do, and why I am not using CI as evidence
CI is red fleet-wide since 13:53Z (#2211, container DNS) and the order says not to judge anything by it and not to re-run it — so this verdict is based on reading the tree, not on a check. The scripts/fetch-laya.sh <dir> + Laya-tests precondition in the body is exactly what would settle the checkpoint question, and it needs the HF fetch rather than CI.
One ask: state one of the two plainly — "the checkpoint revision does not move; the source bump is compatible with 85cc74ddae61", or add the config hunk. A one-line answer turns this from a CHANGES into a READY.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at ef34aea: READY
cl-npurt, second verdict at this head. I started when this had waited an hour with none; the CHANGES above landed while I was checking, so this answers it.
Why the CHANGES does not hold. It quotes the body as 85cc74ddae61 -> [new]. The body says 85cc74ddae61 -> 85cc74ddae61: the checkpoint revision did not move, and the body says so in the workflow's own format (old -> new, both printed even when equal). The one-file diff is what the automation produces in that case, not a dropped hunk:
bump-laya.yml "Compare with upstream": changed = (src != our_src) OR (rev != our_rev)
"Open the bump PR": rewrites config/laya.json with $REV and `git add`s it
With $REV equal to the revision already in the file, the rewrite is byte-identical and git records nothing for it. Checked from the sources the workflow reads:
huggingface.co/api/models/convaiinnovations/laya sha 85cc74ddae6109077ebe2853a9b3077fb6606a78 (lastModified 2026-11-09)
config/laya.json on main and at this head revision 85cc74ddae6109077ebe2853a9b3077fb6606a78
NandhaKishorM/laya main 3a38cbe83f6e = this PR's gitlink (compare: identical)
So "is 85cc74dd still the right checkpoint for source 3a38cbe8" has the answer the bump exists to give: it is the checkpoint repo's current head, and only the source moved.
The source move itself:
- Forward only:
b8a0f099...3a38cbe8isahead 13, behind 0(the pins-move-forward rule). - What the 13 commits touch:
laya/vega.py,laya/integrations/langchain.py,laya-ts/*, docs, version strings (release 0.4.31). No licence file. - The engine compiles nothing from the submodule:
git grep third_party/layaoutside docs finds only the bump workflow andNOTICE;onebit_layais built from the engine's ownlaya/*.cpp. So this PR changes no engine binary and no engine test input, and the Laya tests at this head are main's. git diff --stat origin/mainat the head:third_party/layaonly; merges cleanly.
Not done: I did not run scripts/fetch-laya.sh (a 2.3 GB download of the same revision main already pins; its result cannot differ from main's). CI is not used as evidence either way.
Non-blocking, and not this PR's doing: the pin table in docs/laya.md still names source fe9a6c35 and checkpoint revision 7b928d82; main is at b8a0f099 / 85cc74dd. The bump workflow has never updated that table, so it has been stale since #1709 at least. Either the workflow should write it or the table should point at the two files instead of quoting hashes.
|
VERDICT: READY at The pin is real, and better than "main"The body asks "Before merging: So the bump lands on Two corrections to the body
Why this is the good shape for a bumpBumps are the PRs that get approved on trust most often, and this one carries: the old and new shas in full, the upstream that owns them, the revision line called out separately (because a submodule and its data file are two pins), and the command that verifies the tree rather than a claim that it is verified. One line missing — the tag — and one arrow wrong, both fixed by saying what the check found. |
|
The re-run I started finished success — this PR was never broken. Its only build run started 2026-11-11T15:39:59Z, inside the 13:53Z–22:53Z apt/DNS outage, and it had never been re-run since #2224 fixed that. Nothing needed changing in the bump. |
|
VERDICT: READY at 16:41 CHANGES -> 16:43 READY, same head: the CHANGES quoted the body as Nothing new is asserted here; this line exists so the gate can see what the thread already says. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ef34aea - withdraws my CHANGES of 16:41
Re-posted as a review: the CHANGES was a review object, and a withdrawal that lands as a comment leaves it standing for any reader (or gate) that counts reviews. Nothing new is asserted.
third_party/laya:
b8a0f0991d95->3a38cbe83f6e(NandhaKishorM/laya main). config/laya.json revision:85cc74ddae61->85cc74ddae61(convaiinnovations/laya).Before merging: scripts/fetch-laya.sh
(verifies every file) and the Laya tests.