Repository navigation
build: pin dynamo-llm version in backend-common for cargo publish - #13303
pvijayakrish wants to merge 2 commits into
Conversation
cargo publish rejects a path dependency without a version; this is why dynamo-backend-common has missed every crates.io GA since 1.2.1 (the old publish loop masked the error). workspace = true is not an option because the crate needs default-features = false. Verified with cargo publish --dry-run on release/1.4.0. Signed-off-by: pvijayakrish <pvijayakrish@nvidia.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| [dependencies] | ||
| dynamo-runtime = { workspace = true } | ||
| dynamo-llm = { path = "../llm", default-features = false } | ||
| dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false } |
There was a problem hiding this comment.
🔴 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 (version = "1.4.0" at lib/backend-common/Cargo.toml:26) in a file that the nightly version-stamping step never rewrites, so once nightly renames every crate version to a dated pre-release the requirement no longer matches and the build stops.
Impact: Nightly container/wheel builds break with a dependency version mismatch, and every future release-version bump silently leaves this pin stale.
Mismatch between the dev-version stamper and the new literal pin
.github/scripts/apply_dev_version.py is run in nightly CI (.github/workflows/shared-build-image.yml:489). It rewrites [workspace.package].version and the internal path-dep pins in the root Cargo.toml (rewrite_root_cargo, .github/scripts/apply_dev_version.py:113-148) plus an explicit allow-list of sub-crate Cargo files (SUBCRATE_CARGO_TARGETS, .github/scripts/apply_dev_version.py:35-44). lib/backend-common/Cargo.toml is not in that list — previously fine because it only used version.workspace = true and an unversioned path dep.
After stamping, lib/llm becomes e.g. 1.4.0-dev20260423, while backend-common still requires ^1.4.0. Cargo validates that a path dependency's actual version satisfies the stated requirement, and a pre-release does not satisfy ^1.4.0, so resolution fails for the whole workspace.
Note that even adding the file to SUBCRATE_CARGO_TARGETS would not help as written: VERSION_LINE_RE (.github/scripts/apply_dev_version.py:51) is line-anchored and skips inline-table dep forms like dynamo-llm = { path = "../llm", version = "1.4.0" }.
Prompt for agents
lib/backend-common/Cargo.toml now pins its dynamo-llm path dependency with a literal `version = "1.4.0"` so that `cargo publish` accepts it. However .github/scripts/apply_dev_version.py, which nightly CI runs (see .github/workflows/shared-build-image.yml), only rewrites versions in the root Cargo.toml (including internal path-dep pins) and in the explicit SUBCRATE_CARGO_TARGETS list; lib/backend-common/Cargo.toml is in neither, and its dep pin uses the inline-table form that the line-anchored VERSION_LINE_RE does not match. Consequence: after nightly stamps lib/llm to 1.4.0-devYYYYMMDD, backend-common still requires ^1.4.0, which a pre-release does not satisfy, and cargo fails to resolve the workspace. Consider either teaching apply_dev_version.py to rewrite internal path-dep version pins in arbitrary sub-crate Cargo.tomls (including inline tables) and registering lib/backend-common/Cargo.toml, or avoiding a hardcoded literal here (e.g. inherit from the workspace dependency entry and turn off default features some other way, such as a workspace-level dynamo-llm entry with default-features = false, or a feature-gated variant) so there is a single source of truth for the version. Also add a check that any internal path-dep version pin equals [workspace.package].version so future release bumps can't leave it stale.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — confirmed and fixed in 907976f.
Reproduced exactly: apply_dev_version.py .dev20260814 stamps the workspace to 1.4.0-dev20260814 while this pin stays ^1.4.0, and resolution fails:
error: failed to select a version for the requirement `dynamo-llm = "^1.4.0"`
candidate versions found which didn't match: 1.4.0-dev20260814
One correction to the suggested remedy: adding this file to SUBCRATE_CARGO_TARGETS would not fix it — VERSION_LINE_RE is line-anchored and deliberately skips inline dep tables (name = { version = "..." }), so the pin would survive stamping untouched. Instead, 907976f gives rewrite_root_cargo a WORKSPACE_PIN_CARGO_TARGETS list of member manifests that pin the workspace version inline, and applies the same exact-string rewrite it already uses for the root path-dep pins. Verified on a scratch checkout: after stamping, the pin follows the workspace version, full cargo metadata resolution passes, and re-running the stamper stays a no-op.
Why the pin exists at all: cargo publish rejects a path dep without a version (this is why dynamo-backend-common has missed every crates.io release since 1.2.1 — the old publish loop masked the error), and workspace = true cannot be combined with default-features = false (cargo: `default-features = false` cannot override workspace's `default-features`), so a versioned path dep is the only representable form.
There was a problem hiding this comment.
P1 — reopening with evidence: the mechanism in 907976f is right, but it no longer engages on this PR's own base branch.
rewrite_root_cargo builds pin_re from the root's [workspace.package].version literal and then applies that exact string to WORKSPACE_PIN_CARGO_TARGETS. That is correct provided the pin here equals the workspace version. It no longer does: main moved to 1.5.0 (Cargo.toml:48) while this line still says 1.4.0, so the new loop matches nothing here and skips the write without a word.
Ran the stamper on a pristine origin/main tree with this PR's two files applied:
$ python3 .github/scripts/apply_dev_version.py ".dev20260915" .
apply_dev_version: stamped suffix '.dev20260915'
root [workspace.package].version: version = "1.5.0-dev20260915"
root dynamo-llm ws dep: dynamo-llm = { path = "lib/llm", version = "1.5.0-dev20260915" }
backend-common dynamo-llm pin: dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false }
Exit 0. The result is the failure you reproduced, one minor version along — confirmed on a minimal workspace of the same shape:
error: failed to select a version for the requirement `llm = "^1.4.0"`
candidate versions found which didn't match: 1.5.0-dev20260915
Two controls, so this is scoped correctly:
- workspace left unstamped at
1.5.0with the pin at^1.4.0resolves fine — caret accepts1.5.0, so ordinary builds and the merge itself are unaffected. This bites the nightly path only. - pin set equal to the workspace version and then stamped resolves fine, and a re-run is a no-op. The mechanism you added is sound; only the literal is wrong.
Pre-merge CI cannot catch it either way: the stamping step is gated if: inputs.dev_version_suffix != '' (shared-build-image.yml:487) and only nightly-ci.yml supplies a suffix, so the first signal would be a red nightly after merge.
Worth noting the release line already settled this convention: v1.4.1 pins 1.4.1 and v1.4.2 pins 1.4.2 — the literal tracks [workspace.package].version at every tag, the latter via a commit (ac4d289c) whose entire content is that bump.
| dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false } | |
| dynamo-llm = { path = "../llm", version = "1.5.0", default-features = false } |
Verified: with that value, stamping a clean origin/main tree carries the pin to 1.5.0-dev20260915 alongside the root, and re-running the stamper is a no-op.
One caveat this exposes, which is why I have also left a P2 on .github/scripts/apply_dev_version.py: nothing makes the stamper tell you the pin drifted. It treats a zero-match rewrite as success. Whatever value lands here will need the same manual bump at 1.6.0 unless the script starts failing loudly.
There was a problem hiding this comment.
[P1] Still live, and the gap is now wider. lib/backend-common/Cargo.toml:26 pins 1.4.0, but main moved to 1.6.0 on 2026-09-17 (#15009). The new rewrite loop matches nothing here and skips the write without a word. My earlier 1.5.0 suggestion is stale too. Please set the pin to 1.6.0.
| dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false } | |
| dynamo-llm = { path = "../llm", version = "1.6.0", default-features = false } |
Measured at the live head merged onto the current base tip, with two controls
The head is unmoved at 907976f28c, and the timeline records no head_ref_force_pushed event. The base branch is main. The merge base is a97a2b86e0, and main has gained 694 commits since. I merged main at b522119801 into the head. Both files auto-merged with no conflict, and the new loop is byte-identical to the head copy, sha256 958a090780a6c28a on both sides.
The merged tree is the test case by itself: root version = "1.6.0", pin version = "1.4.0".
$ python3 .github/scripts/apply_dev_version.py ".dev20260918" .
apply_dev_version: stamped suffix '.dev20260918'
EXIT=0
root: version = "1.6.0-dev20260918"
root ws dep: dynamo-llm = { path = "lib/llm", version = "1.6.0-dev20260918" }
backend-common: dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false }
The member manifest is byte-identical before and after the run, sha256 f0a28996b3934822 on both sides, so no write happened.
Control on the same tree, pin set to 1.6.0 first: the rewrite engages and carries the pin to 1.6.0-dev20260918. The file checksum moves from 530811c6b21f8513 to 8c6d5d7af3750ba6, so a clean result here means something.
Cargo consequence, on a minimal workspace of the same shape, cargo 1.96.1:
| case | workspace version | pin | exit |
|---|---|---|---|
| A, after stamping | 1.6.0-dev20260918 |
^1.4.0 |
101 |
| B, control, unstamped | 1.6.0 |
^1.4.0 |
0 |
| C, control, pin fixed | 1.6.0-dev20260918 |
1.6.0-dev20260918 |
0 |
Case A:
error: failed to select a version for the requirement `llm = "^1.4.0"`
candidate versions found which didn't match: 1.6.0-dev20260918
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 if: inputs.dev_version_suffix != '' at shared-build-image.yml:469, and only nightly-ci.yml supplies a suffix.
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 llm = { workspace = true, default-features = false } with no default-features on the workspace entry. Cargo raised no error and printed no warning, and cargo tree -e features showed the member still pulling llm feature "default". Inheritance cannot turn default features off, so a versioned path dep stays the only representable form.
One caveat on the suggested value. 1.6.0 is correct today and will go stale at the next bump, exactly as 1.5.0 did in the 61 hours since I wrote it. That is the P2 thread on .github/scripts/apply_dev_version.py:167.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request updates the ChangesDependency Version Alignment
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized manifest change pins the dependency version needed for publishing while preserving existing behavior. After the repository’s normal formatting and lint checks, no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
backend-common's dynamo-llm pin lives in an inline dep table, which VERSION_LINE_RE deliberately skips; nightly stamping would leave it at 1.4.0 while the workspace moves to 1.4.0-devN and resolution fails. rewrite_root_cargo now applies its exact-string rewrite to WORKSPACE_PIN_CARGO_TARGETS as well. Verified by stamping a scratch checkout: pin follows the workspace version, cargo resolution passes, re-run is a no-op. Signed-off-by: pvijayakrish <pvijayakrish@nvidia.com>
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Reviewed at 907976f against base a97a2b8. Checked out locally; everything below was produced by running the script and cargo, not by reading them.
Intent (sourced from the diff and the repo): give lib/backend-common's dynamo-llm path dependency an explicit version so cargo publish accepts it, and teach .github/scripts/apply_dev_version.py to keep that literal in step with [workspace.package].version when nightly stamps a dev suffix.
The premise holds
cargo package really does reject the versionless form. On a minimal workspace with llm = { path = "../llm", default-features = false }, cargo 1.96.1 gives:
error: all dependencies must have a version specified when packaging.
dependency `llm` does not specify a version
And the workspace = true escape hatch is genuinely unavailable: with llm = { workspace = true, default-features = false }, cargo tree -p bc -e features still reports llm feature "default" enabled, so the member's default-features = false does not take effect. A versioned path dep is the right shape here.
Relationship to #13304
Not duplicates, and not stacked — the same change aimed at two different base branches. #13304's two commits carry (cherry picked from commit 7fa9245...) and (cherry picked from commit 907976f...), naming this PR's two commits exactly, and it targets release/1.4.0 while this one targets main. Neither depends on the other landing, so as things stand the backport would merge before its own source. This PR is still worth landing on its own: main has neither half of the change today.
Findings
- P1 — the pinned literal is
1.4.0, but this PR's base branch is now at1.5.0. Posted on the existinglib/backend-common/Cargo.toml:26thread, which I have unresolved with the reproduction. - P2 — the new stamping loop cannot tell "pin rewritten" from "pin not found", which is exactly what lets a stale literal through. Inline on
.github/scripts/apply_dev_version.py.
Context, not a finding
The description says backend-common "has missed every crates.io GA since 1.2.1". That was true when written, but dynamo-backend-common 1.4.1 (2026-08-24) and 1.4.2 (2026-08-28) have since published, carrying dynamo-llm ^1.4.1 and ^1.4.2 respectively. They came off the release line, where #13670 merged this same change on 2026-08-22: tags v1.4.1 and v1.4.2 both contain a WORKSPACE_PIN_CARGO_TARGETS block byte-identical to this PR's, with the pin literal equal to the workspace version at each tag. Whether the 1.4.0 publish is still wanted is your call — it does not make this PR redundant for main.
Deliberately not raised
Every check is green, so there is nothing to triage. Worth saying explicitly, though, that green is not evidence in this PR's favour: the stamping step is gated if: inputs.dev_version_suffix != '' (shared-build-image.yml:487) and only nightly-ci.yml passes a suffix, so pre-merge CI never executes the code path this PR changes. The unfilled "Related Issues" template section is already flagged by CodeRabbit; I am not repeating it.
| pin_re = re.compile(rf'(\bversion\s*=\s*"){re.escape(base)}(")') | ||
| text = pin_re.sub(lambda mm: f"{mm.group(1)}{new}{mm.group(2)}", text) | ||
| path.write_text(text) | ||
|
|
||
| # Bump the same literal pin where it lives in member manifests (inline | ||
| # dep tables that VERSION_LINE_RE deliberately skips). The early | ||
| # "already stamped" return above keeps this idempotent. | ||
| for rel in WORKSPACE_PIN_CARGO_TARGETS: | ||
| p = root / rel | ||
| t = p.read_text() | ||
| t2 = pin_re.sub(lambda mm: f"{mm.group(1)}{new}{mm.group(2)}", t) | ||
| if t2 != t: | ||
| p.write_text(t2) |
There was a problem hiding this comment.
P2 — this loop cannot distinguish "pin rewritten" from "pin not found".
pin_re is built from the root's [workspace.package].version literal, so when a member's pin has drifted from it, sub matches nothing, t2 == t, and the if simply skips the write. The script still prints apply_dev_version: stamped suffix ... and exits 0.
Verified on a clean origin/main tree with this PR's two files applied — the stamper exits 0 and leaves lib/backend-common/Cargo.toml at version = "1.4.0" while the root moves to 1.5.0-dev20260915:
$ python3 .github/scripts/apply_dev_version.py ".dev20260915" .
apply_dev_version: stamped suffix '.dev20260915'
root [workspace.package].version: version = "1.5.0-dev20260915"
root dynamo-llm ws dep: dynamo-llm = { path = "lib/llm", version = "1.5.0-dev20260915" }
backend-common dynamo-llm pin: dynamo-llm = { path = "../llm", version = "1.4.0", default-features = false }
The resulting cargo failure is on the lib/backend-common/Cargo.toml:26 thread.
This drift is not hypothetical: on the release line ac4d289c is a commit whose entire content is hand-bumping this same literal from 1.4.1 to 1.4.2. Nothing checks it, and pre-merge CI never runs this path (the step is gated if: inputs.dev_version_suffix != '' and only nightly-ci.yml supplies 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 at 1.5.0-dev20260915, and re-running after restoring the file prints success while leaving the pin at 1.4.0.
Validating every target before writing anything covers both:
| pin_re = re.compile(rf'(\bversion\s*=\s*"){re.escape(base)}(")') | |
| text = pin_re.sub(lambda mm: f"{mm.group(1)}{new}{mm.group(2)}", text) | |
| path.write_text(text) | |
| # Bump the same literal pin where it lives in member manifests (inline | |
| # dep tables that VERSION_LINE_RE deliberately skips). The early | |
| # "already stamped" return above keeps this idempotent. | |
| for rel in WORKSPACE_PIN_CARGO_TARGETS: | |
| p = root / rel | |
| t = p.read_text() | |
| t2 = pin_re.sub(lambda mm: f"{mm.group(1)}{new}{mm.group(2)}", t) | |
| if t2 != t: | |
| p.write_text(t2) | |
| pin_re = re.compile(rf'(\bversion\s*=\s*"){re.escape(base)}(")') | |
| text = pin_re.sub(lambda mm: f"{mm.group(1)}{new}{mm.group(2)}", text) | |
| # Bump the same literal pin where it lives in member manifests (inline dep | |
| # tables that VERSION_LINE_RE deliberately skips). Validate every target | |
| # BEFORE writing anything: a pin that has drifted from | |
| # [workspace.package].version must fail here rather than silently survive | |
| # into the nightly image, and that failure must not leave the root stamped | |
| # on its own -- the "already stamped" return above would make the re-run a | |
| # no-op instead of a recovery. | |
| pending = [] | |
| for rel in WORKSPACE_PIN_CARGO_TARGETS: | |
| p = root / rel | |
| t = p.read_text() | |
| t2, n = pin_re.subn(lambda mm: f"{mm.group(1)}{new}{mm.group(2)}", t) | |
| if n == 0: | |
| raise RuntimeError( | |
| f'{rel}: no version = "{base}" pin found -- it has drifted from ' | |
| f"[workspace.package].version and would not be stamped" | |
| ) | |
| pending.append((p, t2)) | |
| path.write_text(text) | |
| for p, t2 in pending: | |
| p.write_text(t2) |
Verified on fresh origin/main trees with this PR applied:
- stale
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); - pin corrected to
1.5.0-> exit 0, root and pin both1.5.0-dev20260915; - immediate re-run with the same suffix -> exit 0, still
1.5.0-dev20260915(idempotent).
Unverified: I could not run the repo's black/ruff hooks — 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, every WORKSPACE_PIN_CARGO_TARGETS pin equals [workspace.package].version" fails on this PR's head as it stands.
There was a problem hiding this comment.
[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, the if skips 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 907976f28c merged onto the current base tip b522119801. The two runs differ only inside the file, never in what the script says:
| pin in the member manifest | stderr | exit | member file sha256 after |
|---|---|---|---|
1.4.0, drifted |
apply_dev_version: stamped suffix '.dev20260918' |
0 | f0a28996b3934822, unchanged |
1.6.0, matching |
apply_dev_version: stamped suffix '.dev20260918' |
0 | 8c6d5d7af3750ba6, rewritten |
Test selection, run from the merged worktree:
-k "apply_dev_version or dev_version or stamp" no tests collected (900 deselected)
-k "router" 17/900 tests collected (883 deselected)
The control collects 17 from the same 900, so the selector works. Nothing anywhere in the repo tests this script. grep -rn apply_dev_version over *.py, *.yml, *.yaml, *.sh and *.toml returns only the one call site in shared-build-image.yml and the comment references in nightly-ci.yml. No pre-commit hook covers .github/scripts/apply_dev_version.py either.
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:
RuntimeError: lib/backend-common/Cargo.toml: no version = "1.6.0" pin found -- it has drifted from [workspace.package].version and would not be stamped
The root is left unwritten after that raise, sha256 d383710bac4b1fab before and after, so there is no partial write to recover from.
Pin corrected to 1.6.0, guard applied: exit 0, root and pin both 1.6.0-dev20260918. An immediate re-run with the same suffix is a no-op, with both checksums unchanged at 8c6d5d7af3750ba6 and f4af695aad8a5a58.
The control that did not move: the script as written, on the same corrected pin, produces byte-identical output, 8c6d5d7af3750ba6 and f4af695aad8a5a58. 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 was 530811c6b21f8513 against the restored f0a28996b3934822. 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 black or ruff hooks on this machine, so the guard is syntax-checked with ast.parse and tested by execution only. Let pre-commit confirm the formatting.
This branch has not been deployed
2 participants
cargo publish rejects a path dependency without a version; this is why dynamo-backend-common has missed every crates.io GA since 1.2.1 (the old publish loop masked the error). workspace = true is not an option because the crate needs default-features = false. Verified with cargo publish --dry-run on release/1.4.0.
Overview:
Details:
Where should the reviewer start?
Related Issues
🔗 This PR is linked to an issue:
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit