Skip to content

refactor(worker): use revision_exists from huggingface_hub to check branch existence #3203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ArjunJagdale
Copy link

This PR refactors the create_branch function in services/worker/src/worker/utils.py to replace the manual revision existence check using hf_api.list_repo_refs(...) with the more robust and concise revision_exists(...) utility introduced in huggingface_hub>=0.21.0.

Changes

  • Removed list_repo_refs call and related retry logic.
  • Replaced revision existence check with revision_exists(dataset, target_revision).
  • Preserved all existing behavior and error handling.

Benefits

  • Cleaner and more readable logic.
  • Leverages upstream improvements in huggingface_hub.
  • Removes the need for iterating over refs.converts.

Requirements

  • huggingface_hub version must be >= 0.21.0.

Tests

No behavioral change; existing tests covering create_branch(...) should continue to pass. If needed, additional tests can mock revision_exists() to verify branching behavior.


Closes #2562

@severo severo requested a review from lhoestq July 1, 2025 08:04
@ArjunJagdale
Copy link
Author

Noting that the end-to-end tests are currently failing with a 502 Bad Gateway error during setup:

AssertionError: assert 502 == 404

This happens in the poll("/") check from tests/conftest.py, which suggests that the test backend server is temporarily unreachable. The error occurs before any actual test logic related to this PR is exercised.

Since the failure is unrelated to the change (which only touches the revision existence check in create_branch), I believe this is a temporary infra issue.

@lhoestq @severo

@severo
Copy link
Collaborator

severo commented Jul 1, 2025

maybe due to the fact that the PR branch is in another repository. cc @AndreaFrancis @lhoestq

@ArjunJagdale
Copy link
Author

ArjunJagdale commented Jul 1, 2025

maybe due to the fact that the PR branch is in another repository. cc @AndreaFrancis @lhoestq

the original link from #2562 was pointing to a frozen commit, which I couldn't push to, and i guess where all this went wrong. So i tried to apply changes to main. What can be done now?

@severo
Copy link
Collaborator

severo commented Jul 1, 2025

I meant that possibly e2e tests are not running correctly on external PRs like yours. Nothing to do on your side for this point.

I think the two other failing checks (on services/worker) are not affected by this CI bug, and can be fixed so that we can review the PR.

@ArjunJagdale
Copy link
Author

I meant that possibly e2e tests are not running correctly on external PRs like yours. Nothing to do on your side for this point.

I think the two other failing checks (on services/worker) are not affected by this CI bug, and can be fixed so that we can review the PR.

I’ve fixed the code-quality check by removing the unused requests import in utils.py.

Regarding the unit test failure in services/worker/tests/test_executor.py -

I looked into test_executor_raises_on_bad_worker, which runs start_worker_loop_that_crashes() and expects a ProcessExitedWithError or TimeoutExpired.

From what I can see, my patch doesn’t interact with subprocess handling or the worker loop — it only affects the revision checking logic in create_branch(). So I suspect the failure may not be directly caused by this PR, but I’m happy to investigate further.
WDYT? or there can be any different reason?

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.

Use revision_exists (hfh)
2 participants