[PM Fixer] Fix huggingface 1.29.0 compatibility: swallow HfUriError from commitUrl parsing - #548
[PM Fixer] Fix huggingface 1.29.0 compatibility: swallow HfUriError from commitUrl parsing#548agrasth wants to merge 2 commits into
Conversation
…rl parsing huggingface_hub >= 1.20.0 changed RepoUrl to use parse_hf_uri internally (PR huggingface/huggingface_hub#4324), which strictly validates the commitUrl field in the upload commit response. Artifactory returns a placeholder commitUrl that does not conform to the hf:// URI format expected by the new parser, causing upload_folder to raise HfUriError after the files have already been committed to Artifactory. Catch HfUriError specifically in the upload helper and treat it as a non-fatal success: the upload completed before the response was parsed. Versions of huggingface_hub < 1.20.0 are unaffected (HfUriError does not exist; ImportError is caught and the raise path is unchanged). Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
|
I have read the CLA Document and I hereby sign the CLA pm-compat-fixer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe Hugging Face upload command now handles ChangesHugging Face upload handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Hugging Face uploads now tolerate the known post-commit URI parsing error and report it on stderr, while other upload failures still fail normally. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
agrasth
added
the
pm-compat-fixer
agrasth
changed the title
| repo_type=repo_type, | ||
| **kwargs | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
Swallowing based on exception type assumes HfUriError can only occur post-commit in this call path. Can we pin this to the known-affected version range, or at least log when we swallow it, so we're not silently masking a genuine failure if that assumption stops holding in a future huggingface_hub release?
There was a problem hiding this comment.
Added in 64c51f5: now logs a clear message to stderr when the HfUriError swallow triggers (files were already uploaded, only the response URL failed to parse).
Printed to stderr specifically since the Go caller (huggingFaceUpload.go) already streams stderr through unmodified without touching the stdout JSON success/failure contract it parses — so this doesn't risk interfering with that. Didn't add version-pinning on top since the affected range is only knowable approximately and would need its own upkeep; the log line covers the "don't silently mask a future failure" concern more durably.
| # Artifactory returns a placeholder commitUrl that fails this parser, but the | ||
| # files were already committed before the response is parsed. Swallow the URI | ||
| # parse error so callers treat the upload as successful. | ||
| try: |
There was a problem hiding this comment.
Do we already have tests that mocks upload_folder to raise HfUriError and asserts we return cleanly, plus one asserting a different exception still propagates? This is the core behavior change so I think fixer should consider adding unit tests for code changes
There was a problem hiding this comment.
Added in 64c51f5: test_huggingface_upload.py with exactly the two cases you described — one mocking upload_folder to raise HfUriError and asserting upload() returns cleanly, one mocking a different exception and asserting it still propagates.
Addresses review feedback on #548: - Log a clear stderr line when HfUriError is swallowed, so a future case where this assumption breaks (e.g. HfUriError raised somewhere other than commit-response parsing) leaves a trail instead of a silent false-positive success. Printed to stderr, which the Go caller already streams through unmodified - doesn't touch the stdout JSON contract the caller actually parses. - Add unit tests: one asserting HfUriError is swallowed and upload() returns cleanly, one asserting other exceptions still propagate. huggingface_hub.errors.HfUriError doesn't exist in most currently installed huggingface_hub versions (it's a recent addition), so the test patches it in at the exact import path with create=True rather than depending on a specific installed version - verified this actually exercises the real isinstance() check in huggingface_upload.py, not a copy of the logic.
2 participants
Summary
upload_folderin huggingface_hub ≥ 1.20.0 fails against Artifactory withHfUriError(introduced in huggingface/huggingface_hub#4324). That PR changedRepoUrlto use the strictparse_hf_uriinternally when processing the commit response. Artifactory returns a placeholdercommitUrlthat does not conform to thehf://URI format the new parser expects. The actual upload to Artifactory completes successfully before the response is parsed — only the post-upload parsing step raises the error, causing all tests that calluploadTestModelToLocalRepo(andTestHuggingFaceUploadWithLocalGitVcsProps) to fail withrequire.NoError.Fix
Modified
huggingface_upload.py(embedded Python helper forjf hf u) to catchHfUriErrorfromhuggingface_hub.errorsspecifically and return silently, treating the upload as successful. All other exceptions are still propagated.This is a Python-script-only fix — no Go source files were modified and no exported symbols changed.
Backward compatibility:
HfUriErrordoes not exist; theexcept ImportErrorbranch is taken, behavior unchanged.HfUriErrorraised during post-upload response parsing is swallowed; HTTP errors, auth failures, and all other exceptions are still propagated.Compatibility impact
No exported Go symbol was added, removed, or renamed. No
go.modorgo.sumtouched. Existing supported versions (pinned at 1.19.0) are unaffected sinceHfUriErrordid not exist before 1.20.0.Test plan
go build ./...— no Go source files changed; cannot regress.gofmt -l .— no Go files touched.go vet ./...— same.TestExtractPythonScripts(unit test): verifies scripts are extracted; content checking is not in scope..github/workflows/huggingfaceTests.ymllines 69-72.Summary by CodeRabbit
Opened automatically by PM Compat Fixer investigating huggingface 1.29.0.