Revision defaults to empty string #2071
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the revision field optional in the ModelRepo struct to prevent runtime errors for non-HuggingFace volume types (GCS, S3, Azure) that don't require a revision. The change maintains backward compatibility for HuggingFace repos where revision is still required and validated at the Python configuration level.
Key changes:
- Changed
revisionfromStringtoOption<String>in theModelRepostruct - Updated Python bindings to make
revisionan optional parameter with a default value ofNone - Updated all tests to use
Some("...")for HuggingFace repos andNonefor non-HF repos (GCS, Azure)
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| truss-transfer/src/types.rs | Made revision field optional with appropriate serde attributes for serialization |
| truss-transfer/src/lib.rs | Updated test to use Some("main") for revision |
| truss-transfer/src/create/hf_metadata.rs | Added unwrapping logic for HF repos that expects validation from Python layer; updated tests |
| truss-transfer/src/create/basetenpointer.rs | Updated tests to use None for non-HF repos (GCS, Azure) and Some(...) for HF repos |
| truss-transfer/src/bindings.rs | Made revision optional in Python bindings with default None value |
| truss-transfer/Cargo.toml | Bumped version from 0.0.38 to 0.0.39 |
| truss-transfer/Cargo.lock | Updated version reference to match Cargo.toml |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
michaelfeil
left a comment
michaelfeil
left a comment
There was a problem hiding this comment.
So we agreed to make main as part of truss revision, or simply drop the default requirement.
I'd say only validate a huggingface one.
also: * can be empty string or >= 2 chars * required for HF (in model cache v2)
ndeepak-baseten
force-pushed
the
ndeepak/model-cache-optional-revision
branch
from
5bfd07f to
8df3ea9
Compare
ndeepak-baseten
changed the title
| @pydantic.field_validator("revision") | ||
| @classmethod | ||
| def _validate_revision(cls, v: str) -> str: | ||
| if len(v) == 1: |
There was a problem hiding this comment.
if revision == HF: `revision must not be empty.
I have a very useful idea here: I would try recommend importing huggingface hub, get the latest revsion, and suggest it to the user.
There was a problem hiding this comment.
@michaelfeil We do validation for v2, in runtime_path() below. For v1, if it's empty it gets from HEAD. (hf_hub_download() behavior if revision=None).
| {% for repo, hf_dir in models.items() %} | ||
| {% for file in hf_dir.files %} | ||
| {{ "RUN --mount=type=secret,id=" + hf_access_token_file_name + ",dst=/etc/secrets/" + hf_access_token_file_name if use_hf_secret else "RUN" }} python3 /cache_warmer.py {{file}} {{repo}} {% if hf_dir.revision != None %}{{hf_dir.revision}}{% endif %} | ||
| {{ "RUN --mount=type=secret,id=" + hf_access_token_file_name + ",dst=/etc/secrets/" + hf_access_token_file_name if use_hf_secret else "RUN" }} python3 /cache_warmer.py {{file}} {{repo}} {% if hf_dir.revision %}{{hf_dir.revision}}{% endif %} |
🚀 What
Make revision default to empty string for model cache v2, validate it to be either empty or >= 2 chars.
Revision is still required for HF in model cache v2.
💻 How
Make changes to revision defaults. Fix tests as needed.
🔬 Testing
Tests updated. Post-commit passing.