Repository navigation
build: pin dynamo-llm version in backend-common for cargo publish #13303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,7 @@ integration = ["dynamo-runtime/integration"] | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| [dependencies] | ||||||||||||||||||||||||||
| dynamo-runtime = { workspace = true } | ||||||||||||||||||||||||||
| dynamo-llm = { path = "../llm", default-features = false } | ||||||||||||||||||||||||||
| dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false } | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Nightly builds will fail because a hardcoded version number is not updated by the version-stamping step The shared backend crate now hardcodes the required version of the LLM crate ( Mismatch between the dev-version stamper and the new literal pin
After stamping, Note that even adding the file to Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — confirmed and fixed in 907976f. Reproduced exactly: One correction to the suggested remedy: adding this file to Why the pin exists at all:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1 — reopening with evidence: the mechanism in
Ran the stamper on a pristine Exit 0. The result is the failure you reproduced, one minor version along — confirmed on a minimal workspace of the same shape: Two controls, so this is scoped correctly:
Pre-merge CI cannot catch it either way: the stamping step is gated Worth noting the release line already settled this convention:
Suggested change
Verified: with that value, stamping a clean One caveat this exposes, which is why I have also left a P2 on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Still live, and the gap is now wider.
Suggested change
Measured at the live head merged onto the current base tip, with two controlsThe head is unmoved at The merged tree is the test case by itself: root The member manifest is byte-identical before and after the run, sha256 Control on the same tree, pin set to Cargo consequence, on a minimal workspace of the same shape, cargo 1.96.1:
Case A: Control B keeps the scope honest. An unstamped tree resolves fine, so ordinary builds and the merge itself are unaffected. This reaches the nightly path only, and pre-merge CI cannot see it: the step is gated Your reason for the literal pin still holds on cargo 1.96.1, so the shape of the change is right. I set a member to One caveat on the suggested value.
pvijayakrish marked this conversation as resolved.
|
||||||||||||||||||||||||||
| dynamo-protocols = { workspace = true } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| anyhow = { workspace = true } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 — this loop cannot distinguish "pin rewritten" from "pin not found".
pin_reis built from the root's[workspace.package].versionliteral, so when a member's pin has drifted from it,submatches nothing,t2 == t, and theifsimply skips the write. The script still printsapply_dev_version: stamped suffix ...and exits 0.Verified on a clean
origin/maintree with this PR's two files applied — the stamper exits 0 and leaveslib/backend-common/Cargo.tomlatversion = "1.4.0"while the root moves to1.5.0-dev20260915:The resulting cargo failure is on the
lib/backend-common/Cargo.toml:26thread.This drift is not hypothetical: on the release line
ac4d289cis a commit whose entire content is hand-bumping this same literal from1.4.1to1.4.2. Nothing checks it, and pre-merge CI never runs this path (the step is gatedif: inputs.dev_version_suffix != ''and onlynightly-ci.ymlsupplies one), so the first signal is a red nightly.A second, smaller point shapes the fix below: the root is written at line 157, before this loop runs. Any failure inside the loop therefore leaves the root stamped and the members not — and the "already stamped" early return at line 151 then makes a re-run a silent no-op rather than a recovery. Verified by removing the member manifest: the run dies with
FileNotFoundError, the root is left at1.5.0-dev20260915, and re-running after restoring the file prints success while leaving the pin at1.4.0.Validating every target before writing anything covers both:
Verified on fresh
origin/maintrees with this PR applied:1.4.0pin -> raiseslib/backend-common/Cargo.toml: no version = "1.5.0" pin found -- it has drifted from [workspace.package].version and would not be stamped, and the root is left at1.5.0(no partial write);1.5.0-> exit 0, root and pin both1.5.0-dev20260915;1.5.0-dev20260915(idempotent).Unverified: I could not run the repo's
black/ruffhooks — neither is installed on this machine — so the suggestion is syntax-checked (ast.parse) and behaviourally tested, but let pre-commit confirm the formatting.A regression test would also have caught this cheaply; there is currently no test anywhere for
apply_dev_version.py. Asserting "after stamping, everyWORKSPACE_PIN_CARGO_TARGETSpin equals[workspace.package].version" fails on this PR's head as it stands.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Still live. The script still cannot report a skipped pin.
.github/scripts/apply_dev_version.py:167. A no-match leaves the text unchanged, theifskips the write, and the run prints the same success line and exits 0 as a run that rewrote the pin. Please make a zero-match fail loudly. The suggestion on this thread still applies unchanged.The no-match run and the matching run are indistinguishable from the output, and no lane runs this script
Re-run on the live head
907976f28cmerged onto the current base tipb522119801. The two runs differ only inside the file, never in what the script says:1.4.0, driftedapply_dev_version: stamped suffix '.dev20260918'f0a28996b3934822, unchanged1.6.0, matchingapply_dev_version: stamped suffix '.dev20260918'8c6d5d7af3750ba6, rewrittenTest selection, run from the merged worktree:
The control collects 17 from the same 900, so the selector works. Nothing anywhere in the repo tests this script.
grep -rn apply_dev_versionover*.py,*.yml,*.yaml,*.shand*.tomlreturns only the one call site inshared-build-image.ymland the comment references innightly-ci.yml. No pre-commit hook covers.github/scripts/apply_dev_version.pyeither.I re-verified the suggested guard on the merged tree, in both directions, with a control that did not move.
Drifted pin
1.4.0, guard applied:The root is left unwritten after that raise, sha256
d383710bac4b1fabbefore and after, so there is no partial write to recover from.Pin corrected to
1.6.0, guard applied: exit 0, root and pin both1.6.0-dev20260918. An immediate re-run with the same suffix is a no-op, with both checksums unchanged at8c6d5d7af3750ba6andf4af695aad8a5a58.The control that did not move: the script as written, on the same corrected pin, produces byte-identical output,
8c6d5d7af3750ba6andf4af695aad8a5a58. The guard changes the failing case only. I proved the mutation landed first, because a failed write and a correct result look the same: the pre-run checksum was530811c6b21f8513against the restoredf0a28996b3934822. My first attempt at this control was a false pass, because the restore silently failed on a quoting error and the run became a no-op.Unverified: I could not run the repo
blackorruffhooks on this machine, so the guard is syntax-checked withast.parseand tested by execution only. Let pre-commit confirm the formatting.