Exercise the hf_hub fake in CI, and fix the upload path it found - #934
Merged
Merged
Conversation
`hf upload <repo> <file> <path_in_repo>` appended the source basename unconditionally, so the file landed at `<path_in_repo>/<basename>`. The tree then reported `path_in_repo` as a directory and `hf download <path_in_repo>` returned Entry Not Found, so a file could be uploaded and never read back. Upstream's `_resolve_upload_paths` only falls back to the basename when the operand is absent: `path_in_repo` is the destination file for a file source and the destination folder for a directory one. Confirmed against the real `hf` 0.35.3 binary pointed at our fake, where `hf upload integ/repo-v1 ./a.txt docs` leaves a tree of exactly `['docs']`. `collect` now reports whether the source was a directory, and the caller spreads only in that case. `test_upload_prefixes_with_path_in_repo` asserted `docs/a.txt` and so encoded the bug; it is corrected, and the file, directory and absent cases are pinned in both languages.
#931 shipped the resource, the CLI and the fake, but no target, so CI never started the fake and never ran a case against it. This adds the harness wiring and 27 cases, 14 on the resource and 13 on the CLI, and both hosts run all of them. `integ/fixtures/hf-hub/v1.json` was an empty object, which is why the service looked wired: `POST /reset` accepted it and seeded nothing. It now holds three repos, their commits and their `main` refs, so a mount has something to list. The resource cases mirror `integ/resources/github/`: ls, cat, head, wc, stat, `find -name`, `find | wc -l`, `find -empty`, `du -s`, `grep -rl`, `grep -c`, ENOENT, plus a dataset mount and a binary read. The CLI cases cover the flags: `--include`, `--exclude`, `--revision`, `--local-dir`, `--force-download`, `--quiet`, `--repo-type`, `--exist-ok`, `--message`, `--yes`, `--commit-message`, `repo tag create|list|delete`, `repo-files delete` with a glob, and all three 404 kinds. Two things the cases settled: `hf_hub`'s `defaultPort` was 5090, which is discord's port in the cli and chat facets, so the two fakes collided whenever both ran. Moved to 5086, outside the 5087-5099 block, and the goldens that embed the endpoint follow it. A CLI write does not invalidate the mount's index (`indexTtl` 86400, `cachesReads` true), so a mount read straight after a CLI write reads stale. The CLI cases verify through `hf download` rather than through the mount.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e8c7fefef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The TypeScript adapter registered `hf_spaces` and then built it with `HfModelsResource`: the kind was chosen by a ternary that read `hf_datasets` and fell through to models for everything else. The three resources differ only by the `repo_type` they send, so that does not fail. It silently reads the models endpoints and reports the models implementation as the one under test. Python names all three in a map and had never had the gap. Every kind is now named, and an unrecognized one throws rather than becoming models. An absent `repo` throws too, for the same reason: a Hub mount NAMES a repository, so `repoId: ''` would reach the fake as a request for the repository called nothing. Latent until now, since no target mounted a space. So the target gains one, and a case reads it. Restoring the old ternary fails that case with `cat: /space/one_byte.txt: No such file or directory`, which is the models endpoint answering for a repository only the spaces endpoint has. 28 cases now, both hosts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #931, which shipped the hf_hub resource, the
hfCLI and the fake but no harness wiring, so CI started neither and ran no case against them. This adds the wiring plus 27 cases, and both hosts run all 27.Writing the cases found a real bug in the code #931 shipped, which is the first commit here.
hf uploadstored a file under its destination, not at ithf upload <repo> <file> <path_in_repo>appended the source basename unconditionally, sohf upload r a.txt docsstored the file atdocs/a.txt. The mount then reporteddocsas a directory andhf download docsreturned Entry Not Found, so a file could be uploaded and never read back.Upstream's
_resolve_upload_pathsonly falls back to the basename when the operand is absent:path_in_repois the destination file for a file source and the destination folder for a directory one. Settled two ways rather than guessed, since the two readings differ only on a case no unit test covered: upstream's source, and the realhf0.35.3 binary pointed at our fake, wherehf upload integ/repo-v1 ./a.txt docsleaves a tree of exactly['docs'].test_upload_prefixes_with_path_in_repoasserteddocs/a.txt, so it had encoded the bug; it is corrected, and the file, directory and absent cases are pinned in both languages.The fixture was empty
integ/fixtures/hf-hub/v1.jsonwas{}, which is what made the service look wired:POST /resetaccepted it and seeded nothing, so a mount had nothing to list. It now holds three repos, their commits and theirmainrefs.The cases
14 on the resource (
integ/resources/hf_hub/repo.json), mirroring the shape ofinteg/resources/github/: ls, cat, head, wc, stat,find -name,find | wc -l,find -empty,du -s,grep -rl,grep -c, ENOENT, plus a dataset mount and a binary read.13 on the CLI (
integ/cli/hf.json), covering the flags rather than just the verbs:--include,--exclude,--revision,--local-dir,--force-download,--quiet,--repo-type,--exist-ok,--message,--yes,--commit-message,repo tag create|list|delete,repo-files deletewith a glob, and all three 404 kinds, which are distinguishable only byX-Error-Code.Two things the cases settled
A port collision.
hf_hub'sdefaultPortwas 5090, which is discord's port in the cli and chat facets, so the two fakes collided whenever both ran. Moved to 5086, outside the 5087-5099 block, with the goldens that embed the endpoint following it.A stale-index constraint, not a bug. A CLI write does not invalidate the mount's index (
indexTtl86400,cachesReadstrue), so a mount read straight after a CLI write reads stale. The first draft of the CLI cases was flaky for exactly this reason; they now verify throughhf downloadrather than through the mount. Worth knowing before anyone writes the next CLI case against a mount.Verified
27/27 on python and 27/27 on typescript-node, re-run on this base after #932 landed. Python suite exit 0.
pre-commit run --all-filesgreen across all 20 hooks.pnpm -r typecheckclean. Case targets 2290 = baseline, layout parity 252 = baseline. CI wiring isbash -nswept and yaml-linted, but this is the first run of the newinteg-battery-setupstep and of the hf_hub server in theclifacet, so that is the part to watch here.