Skip to content

feat: hf.revision property for HuggingFace Hub locations - #3890

Open
kszucs wants to merge 1 commit into
apache:mainfrom
kszucs:feat/hf-revision-property
Open

feat: hf.revision property for HuggingFace Hub locations#3890
kszucs wants to merge 1 commit into
apache:mainfrom
kszucs:feat/hf-revision-property

Conversation

@kszucs

@kszucs kszucs commented Sep 1, 2026

Copy link
Copy Markdown
Member

Supersedes #3609, which the stale bot closed and GitHub won't let me reopen. Rebased on current main, with the review feedback from that round folded in (see below).

Rationale for this change

HuggingFace Hub (hf://) storage already works via fsspec's HfFileSystem, but there's no way to select a revision (branch/tag/commit) the way iceberg-rust's hf.revision does. HfFileSystem resolves revision per call rather than as a filesystem-wide default (its constructor only takes endpoint/token/block_size/expand_info), so hf.revision is threaded through as a keyword argument on the fsspec input file, output file and delete calls.

Semantics match iceberg-rust's HF_REVISION ("default git revision for all paths that don't specify one"): the property applies to reads, writes and deletes, and a revision embedded in the location — hf://datasets/user/repo@revision/path — takes precedence. That precedence isn't cosmetic: huggingface_hub raises ValueError: Revision specified in path (...) and in 'revision' argument (...) are not the same when both are present and differ, so passing the kwarg unconditionally would break any table whose locations pin a revision themselves.

Two smaller things fall out of this:

  • FsspecInputFile.exists() went through lexists(), which doesn't forward **kwargs (fsspec's AbstractFileSystem.lexists calls self.exists(path), dropping them). It now calls exists() directly when there are kwargs to forward — that path forwards to info() with the same broad exception handling lexists() would have provided, and LocalFileSystem's symlink-aware lexists() override is still used when there is nothing to forward.
  • FsspecOutputFile.to_input_file() carries the kwargs over, so a read-back after a write stays on the same revision.

Are these changes tested?

Yes: unit tests covering revision forwarding on reads (len(), .exists(), .open()), on writes and deletes, path-embedded revisions taking precedence, the no-revision-set default, and non-hf schemes being unaffected. Plus an HF_TOKEN-gated integration test against a real temporary dataset repo that demonstrates the concrete problem: without hf.revision, reads follow the moving default branch, so a file read at one commit silently returns different content once someone pushes to the same path.

Are there any user-facing changes?

Yes: a new hf.revision catalog/table property, documented in configuration.md.

huggingface_hub.HfFileSystem resolves the revision per call rather than
as a filesystem-wide default, so hf.revision is threaded through as a
keyword argument on the fsspec input/output file and delete calls.

Matching iceberg-rust, the property is the default revision for
locations that don't pin one themselves: a revision embedded in the
location (hf://datasets/user/repo@revision/path) wins, since
huggingface_hub raises when both are given and differ.

FsspecInputFile.exists() went through lexists(), which doesn't forward
**kwargs; call exists() directly when there are kwargs to forward, as it
forwards them to info() with the same broad exception handling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 1, 2026 09:16

Copilot AI 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.

Pull request overview

Adds a new hf.revision table/catalog property to make hf:// (HuggingFace Hub) file locations reproducible by forwarding a default revision argument to fsspec HfFileSystem operations when the location itself does not already pin a revision.

Changes:

  • Introduce HF_REVISION = "hf.revision" and thread it through FsspecFileIO into FsspecInputFile/FsspecOutputFile calls via per-operation fs_kwargs.
  • Ensure revision precedence rules: an @revision embedded in the hf://... location suppresses forwarding the revision= kwarg.
  • Add unit + HF_TOKEN-gated integration tests and document the new configuration property.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tests/io/test_fsspec.py Adds unit/integration coverage for hf.revision forwarding, precedence, and scheme isolation.
pyiceberg/io/fsspec.py Implements hf.revision forwarding via fs_kwargs, including existence checks that preserve kwargs.
pyiceberg/io/init.py Defines the HF_REVISION property key constant.
mkdocs/docs/configuration.md Documents the new hf.revision configuration option.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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