Skip to content

fix(hub): classify Hub failures by exception type, not class name - #131

Merged
ezietlowTT merged 2 commits into
mainfrom
stisi/fix/hub-error-classification
Sep 22, 2026
Merged

ezietlowTT merged 2 commits into
mainfrom
stisi/fix/hub-error-classification

Conversation

@stisiTT

@stisiTT stisiTT commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

classify_hub_error compared type(exc).__name__ against "EntryNotFoundError".
huggingface_hub 1.x raises subclasses for the cases it can be specific about — a
missing file is RemoteEntryNotFoundError — so that branch silently stopped
matching and the missing-file case fell through to the repo-level 404: "no such
bundle, or no access" for a repo that plainly exists, with advice to go find a
different id or log in.

isinstance covers the subclasses. LocalEntryNotFoundError has to stay ahead
of the missing-file branch because it is also an EntryNotFoundError.

The test helper now builds the real exception class whenever the installed hub
ships one. With isinstance branching a same-named fake gets routed by the
message heuristics instead, so all of these tests would have kept passing while
the type evidence went unexercised — which is how the bug survived.

  • Two new cases carry neither the manifest filename nor a network keyword, so the
    class is the only evidence (the first fails before, passes after)
  • Full offline suite: 918 passed

Stacked on #130.

🤖 Generated with Claude Code

stisiTT and others added 2 commits September 21, 2026 10:51
AGENTS.md requires every line of terminal output to go through console.py;
these two commands were the last "Publish models" entries still calling
typer.secho directly, so their messages ignored --no-color and the collapsed
summary and did not match the container push branch beside them.

The new test pins the difference rather than the wording: it asserts the
`!`/`✓` marker in the two-space gutter and that every wrapped continuation
line keeps the body padding, none of which typer.secho can produce. The
existing assertions matched both spellings, so the change was invisible to
CI and a revert would have shipped green.

Scope is the two success paths. Both commands still reach typer.secho on
their error path through `_err`, which is 68 call sites and its own concern.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
classify_hub_error compared type(exc).__name__ against "EntryNotFoundError"
and "LocalEntryNotFoundError". huggingface_hub 1.x raises SUBCLASSES for the
cases it can be specific about -- a missing file is RemoteEntryNotFoundError
-- so those branches silently stopped matching and the missing-file case fell
through to the repo-level 404: "no such bundle, or no access" for a repo that
plainly exists, with advice to go find a different id or log in.

isinstance covers the subclasses, and LocalEntryNotFoundError has to stay
ahead of the missing-file branch because it is also an EntryNotFoundError.
The base classes come from huggingface_hub.utils, which exists at our >=0.23
floor; huggingface_hub.errors does not.

The test helper now builds the REAL exception class whenever the installed
hub ships one. With isinstance branching, a same-named fake is routed by the
message heuristics instead, so every one of these tests would have kept
passing while the type evidence went unexercised -- which is how the bug
survived in the first place. The two new cases carry neither the manifest
filename nor a network keyword, leaving the class as the only evidence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@stisiTT
stisiTT marked this pull request as ready for review September 21, 2026 19:41
@stisiTT
stisiTT requested review from a team and ezietlowTT as code owners September 21, 2026 19:41
Base automatically changed from stisi/fix/publish-console-output to main September 22, 2026 15:08

@ezietlowTT ezietlowTT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. Reviewed and tested locally (full offline suite: 918 passed; targeted hub/publish tests: 41 passed).

The core fix is right and matters: classify_hub_error now branches with isinstance instead of matching type(exc).__name__. huggingface_hub 1.x raises RemoteEntryNotFoundError / LocalEntryNotFoundError (subclasses of EntryNotFoundError), so the old name comparison would stop matching the moment the library got more specific — silently, falling through to the repo-level 404 and telling a user their repo doesn't exist when only one file inside it is missing.

Verified against the installed hub (1.27.0):

  • LocalEntryNotFoundError really is a subclass of EntryNotFoundError, so the offline branch must stay ahead of the missing-file branch — and it does. Good catch and correct ordering.
  • RemoteEntryNotFoundError exists here, so the new subclass test actually runs rather than skipping.
  • The imports (huggingface_hub.utils.{EntryNotFoundError, LocalEntryNotFoundError}) resolve on the >=0.23 floor.

The tests are the strong part: _exc now instantiates the real hub class when present (falling back to a fabricated one otherwise), so the isinstance path is genuinely exercised rather than passing via message heuristics; and the two new cases cover the subclass and the offline-ordering hazard with messages that carry no filename/network keyword, so the class is the only evidence.

The bundled cli.py change (publish/unpublish moving from typer.secho to console.note/console.milestone) is a reasonable consistency cleanup and has its own gutter/marker test. OfflineModeIsEnabled staying a name match is fine — it isn't reliably importable across versions.

community-labeling red is the known #80 org-secret gating, unrelated.

@ezietlowTT
ezietlowTT merged commit 13f6496 into main Sep 22, 2026
7 of 8 checks passed
@ezietlowTT
ezietlowTT deleted the stisi/fix/hub-error-classification branch September 22, 2026 15:16
ezietlowTT added a commit that referenced this pull request Sep 22, 2026
…s card gate

The out-of-order merge landed #131 (publish output via console.py) on main while
#135 (publish card gate) sat unmerged. #135 makes `publish` read the published
manifest before touching visibility, so #131's test — which predates the gate —
must stub `hub.fetch_manifest` like the gate's own tests do, or it hits the Hub.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ezietlowTT added a commit that referenced this pull request Sep 22, 2026
* fix(card): drop the capacity columns from a diffusion card's profile table

max_num_seqs and max_model_len configure a continuous-batching engine. A
tt-dit-server profile has neither -- its launcher does not even ask for them
-- so every multi-profile diffusion card rendered two columns of empty cells,
a table that states nothing. Include the columns only when some profile fills
them; LLM cards are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* feat(card): document the consumer flow and derive Using it

A generated card told a reader what the package IS and then stopped: no
command to run it, and nothing about what a client actually sends. The
Quickstart now leads with the consumer path -- `uv tool install tenstorrent`,
`tt model pull`, `tt serve` -- and a "Without tt-cli" fence right under it
does the same job with `tt-model` alone.

Both fences, not one. AGENTS.md invariant 1 says a package must be fully
usable with tt-model by itself and a card must never introduce a step that
requires tt-cli; a card printing only `tt` commands broke that in the one
artifact every consumer reads. Leading with `tt` is still right -- it is what
GA ships and it installs in one line -- so the card shows the recommended
path first and the self-contained one immediately after, and the prose that
explains the flow names both spellings rather than leaving the second fence
as an unexplained footnote.

`## Using it` is derived per kind from a new OPENAI_COMPATIBLE launcher
attribute rather than by string-matching SERVER_DESC, which coupled card
semantics to prose: rewording that string would have silently told readers a
tt-dit-server speaks the OpenAI API. A test now pins the two to agree.
Capabilities are read per profile, because they deep-merge per profile and
the old code read only the default -- claiming a reasoning parser for every
profile directly under a table inviting the reader to pick a different one,
and never mentioning a `tools` profile the default lacked. Qualified lines
mark which profile is the default, so "On `--profile latency`" cannot read as
"only if you pass this" when a bare serve is already on it.

`## Feedback` names the channel that reaches the package author (the repo's
HF Discussions), `tt report issue` for tooling problems (it files against
tenstorrent/tt-cli, so it is described that way), and support@tenstorrent.com
for product feedback. `tt report feedback` is deliberately absent: it is
still a stub that exits UNSUPPORTED.

The example's card.quickstart now carries a prompting note instead of
restating the port and the commands. tt-model-yaml/SKILL.md tells authors not
to restate generated facts, and the example is what they copy; its drift
guard asserts the absence rather than requiring the duplication.

Live cards pick all of this up only on their next `tt-model package
--container`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(card): give the card template real fields

A generated card carried what the tool could derive and nothing else, so the
sections a reader actually needs -- how fast is it, where does it fall short,
what may I do with the weights -- had nowhere to live. `card:` in the
authoring manifest now holds them, each rendered in a fixed place the
renderer owns, so the same information is in the same spot on every card.

performance and limitations render even when empty, as an italic "not
provided by the package author". Silence reads as "no limitations"; naming
the gap is both true and visible. They are also what a catalog listing will
require -- that gate is its own change, and this one adds no command
behaviour.

The Hub's own frontmatter keys (license, pipeline_tag, base_model) are
emitted under the Hub's names, so ModelInfo surfaces them without anyone
scraping markdown. pipeline_tag falls back to a new DEFAULT_PIPELINE_TAG
launcher attribute rather than a hardcoded string, and base_model defaults to
the weights repo, which is what makes the package appear on the upstream
model's own Hub page.

license.id is validated at authoring time against the Hub's identifier shape,
because the Hub validates that field SERVER-SIDE on the README commit -- i.e.
inside push, after a 2.5-4h build and a multi-GB upload, with an error
tt-model did not write. Matched with fullmatch, not a `$` anchor: Python
accepts one trailing newline there, and `id: >` (a folded scalar, which the
example uses for five neighbouring fields) yields "other\n" -- which passed
the shape check and then failed every `== "other"` comparison, silently
dropping the author's licence name and link and putting a multi-line scalar
into that very field. The authoring model now collapses whitespace on all
three verbatim frontmatter fields, and the renderer does the same on its way
out, since its input is a wire manifest another tt-model may have written.

At-a-glance cells are escaped and whitespace-collapsed: an unescaped `|` in
`architecture` ("30B MoE | 3B active") silently split the table.

The wire CardSpec stays permissive while the authoring CardSettings forbids
extras -- a newer tt-model's card section must not make an older one refuse a
bundle, but `limitiations:` in a hand-written YAML has to fail at load, where
the author can still fix it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(publish): refuse a catalog listing whose card lacks performance/limitations (#135)

The catalog's problem was never that it is empty -- it is that a listing
promises nothing, so a reader cannot tell how fast a model is or where it
falls short. Those two sections are now the price of a listing.

Gated on every route by which a command can END with the repo listed:
`publish`, `push --publish`, and a plain re-push of a bundle that is already
listed, which re-applies the listing the card overwrite just dropped. That
last one is the ordinary update workflow -- publish once, later drop a
section, push -- and it reached the same catalog state the gate exists to
prevent without anyone typing --publish. `--private` is NOT gated: it
delists, so it cannot produce a bad listing, and gating it would block the
one action that fixes one.

Fails closed. `publish` reads the PUBLISHED manifest, since the author's
tt-model.yaml never left their machine, and only one outcome is an exemption:
a repo with no manifest file at all is not a container bundle. Offline, gated,
a transport error, or a manifest that will not parse all refuse -- before the
visibility flip, so a flaky Hub can no longer skip the check and still flip
the repo public. An unparseable manifest reports itself as probably newer than
this tt-model and asks for an upgrade, rather than being routed through the
Hub-error classifier and blaming the network for something no retry fixes.

`package` only warns: a 2.5-4h build should not discover this at the end, but
a private or experimental build has every right to skip the sections. A wire
manifest with no card block is exempt on `publish` (nothing to read is not
the same as nothing to say) but a gap on `push`, where re-packaging is local
and a manifest is a JSON file anyone can hand-edit. On a re-push that refusal
offers `unpublish` as well as re-packaging, because telling someone to re-run
a four-hour build to complete a routine update is advice they cannot act on.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: stub fetch_manifest in the #131 publish-output test after #135's card gate

The out-of-order merge landed #131 (publish output via console.py) on main while
#135 (publish card gate) sat unmerged. #135 makes `publish` read the published
manifest before touching visibility, so #131's test — which predates the gate —
must stub `hub.fetch_manifest` like the gate's own tests do, or it hits the Hub.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Eric Zietlow <ezietlow@tenstorrent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants