Repository navigation
fix(push): repoint the model card's quickstart to the pushed repo (#109) - #115
Conversation
`render_model_card` bakes the manifest's repo into the card's `tt-model pull` /`tt-model serve` quickstart lines at package time, before any `push --repo` override is known. Pushing a package to a different repo then published a card whose quickstart pointed a reader at a repo that isn't there. `push_container` now repoints those two command lines from the packaged repo (`container.built.repo`) to the actual push target when they differ. Those lines are the only place the card names the repo, so the swap is complete; a no-op when the repos match or the card is missing. Closes #109 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
anirudTT
left a comment
There was a problem hiding this comment.
Thanks for picking this up — I filed #109, and the diagnosis here is right. But I think the fix swaps my bug for a slightly different one.
_repoint_card_quickstart edits README.md in the staged dir in place, but matches on built.repo, which never moves. So the anchor is fixed and the file isn't. After the first push --repo, the card no longer contains built.repo anywhere, and every push after that quietly does nothing.
I checked out the branch and ran it. Two cases:
package for tenstorrent/devstral, then
push --repo anirud/devstral card says anirud/devstral ok
push uploads to tenstorrent/devstral,
card still says anirud/devstral
push --repo a/devstral card says a/devstral ok
push --repo b/devstral bytes go to b, card still says a,
and no note is printed
The second is #109 again with different names. The first is arguably worse — a plain push right after a redirected one publishes someone else's repo name onto the canonical repo, silently.
The staged dir isn't temporary either. It's ~/tt-model-builds/<name>, and people push from it more than once, so this sticks.
Fix is small, I think: anchor on the command prefix instead of the old repo name, so the swap doesn't care what's currently in the file.
_QUICKSTART = re.compile(r"(?m)^tt-model (pull |serve )\S+")
...
swapped = _QUICKSTART.sub(lambda mo: f"tt-model {mo.group(1)}{new_repo}", text)I ran that against a real render_model_card output — authored → a/x → b/x lands correctly, no trace of either earlier repo, reapplying is a no-op. The existing swapped == text check then covers the nothing-to-do case on its own, so the old_repo == new_repo early return becomes optional rather than the thing holding it up.
One thing to watch with the regex: the author's own card.quickstart gets appended after the generated block, so a whole-file sub would rewrite command lines there too. Probably what you want, but if you'd rather keep the blast radius tight, scope the sub to the first fenced block.
Smaller stuff:
- The docstring says those two command lines are the only place the card names the repo. True for the generated part, but
card.descriptionandcard.quickstartare author prose and can name it too. Worth softening. - The write happens before the upload, so if
push_large_folderthrows you're left with a card naming a repo it never got pushed to. With the anchoring fix that heals itself on the next push; right now it doesn't. built.repostaying put is correct — its only consumer is the default target atcli.py:1717. Just flagging that the publishedtt-model.yamlandREADME.mdwill disagree about the repo after a redirected push. Fine by me, noting it for the record.- Tests: 896 pass, confirmed locally.
test_repoint_card_quickstart_swaps_only_the_command_linesmentions "no write of an identical file" but only asserts on the return value, which short-circuits before any write anyway. The render → repoint round-trip test is the valuable one, definitely keep that. Worth adding the two sequential-push cases above as regressions.
… repo (#109) Addresses @anirudTT's review of #115. The first version of this fix matched on the authored repo (`built.repo`), which never moves in the manifest — so it only worked for the first `push --repo`: once the card no longer literally contained the old repo, every later push matched nothing and silently no-op'd. Worse, a plain push after a redirected one left the canonical repo's card advertising someone else's repo. Anchor on the command prefix instead, so the swap replaces whatever repo is currently on the line regardless of history — self-healing and idempotent. Also handles all four generated command lines, including the `tt model pull` / `tt serve` (tt-cli) pair that #133 added since this branch was opened; the old two-line swap would have missed them. Requiring a `namespace/name` after the prefix keeps a stray prose line from matching, and `card.description` / an author's appended `card.quickstart` prose are left alone. - Reworked `_repoint_card_quickstart` (drops the `old_repo` arg; the no-op case is the `swapped == text` check now). - Regression tests: repeated/redirected pushes stay correct (canonical -> a -> canonical -> b), the tt-cli lines are repointed, prose is untouched, and the end-to-end render -> repoint test now covers all four commands to guard format drift. - Merged current main (picks up #133's new card format). Full offline suite: 991 passed. Closes #109 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @anirudTT — you were exactly right, and the sequential-push case (redirect then plain push publishing someone else's repo name onto the canonical repo) is the worse one. Fixed in Done:
Left Merged current main too, so it's up to date with the new card format. Full offline suite: 991 passed. Ready for another look when you have a minute. |
jashansinghTT
left a comment
There was a problem hiding this comment.
Tested end to end from a clean venv on a QB2 box with four real pushes to private HF repos (override → plain push back to canonical → second override → idempotent re-push): the published card's quickstart named the right repo on the Hub every time while current main still shipped the authored repo, and the full offline suite is green — LGTM; one small non-blocking follow-up is that the generated "open a discussion at https://huggingface.co//discussions" line still names the authored repo after a redirected push.
anirudTT
left a comment
There was a problem hiding this comment.
deferring to @jashansinghTT approval
What
tt-model push --repo <other>published a model card whose Quickstart pointed at the authored repo, not the one the package was actually pushed to (issue #109). A reader following the card'stt-model pull/tt-model servelines would hit a repo that isn't there.Why
render_model_cardbakes the manifest'srepointo the two quickstart command lines at package time — before anypush --repooverride exists. Nothing downstream realigned them, so a redirected push shipped a stale card.Fix
push_containernow repoints those two command lines from the packaged repo (container.built.repo) to the real push target (repo_id) whenever they differ:tt-model pull <repo> --with-weightsandtt-model serve <repo>lines — the only two places the card names the repo, so the swap is complete;pushis unaffected);repointed the card's quickstart to <repo>note when it fires.Tests
Three tests in
tests/test_container_cli.py:test_repoint_card_quickstart_swaps_only_the_command_lines— helper swaps both lines and nothing else;test_push_container_repoints_the_card_when_repo_is_overridden— end-to-end push with an overridden repo rewrites the staged card;test_a_rendered_card_repoints_cleanly_end_to_end— render → repoint round-trip, guarding against quickstart format drift inbuild.render_model_card.Verified the tests fail without the source fix (the regression genuinely reproduces #109), and the full suite is green: 896 passed.
Closes #109
🤖 Generated with Claude Code