Skip to content

fix: remove existing test repo before upload #519

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.

Sign up for GitHub

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

Merged
drbh merged 8 commits into from
May 13, 2026
Merged

fix: remove existing test repo before upload #519

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
327a399
fix: remove existing test repo before upload
drbh May 4, 2026
39e6e13
fix: add missing content type
drbh May 4, 2026
ed45c37
fix: prefer removing repos via hub library
drbh May 12, 2026
bbba328
fix: use lib from nix shell on runner
drbh May 12, 2026
09d34b7
fix: disallow more than one instance of E2E running at once to avoid …
drbh May 12, 2026
ed0c47e
fix: prefer using ci token
drbh May 12, 2026
0ba827a
fix: update e2e to use trust_remote_code for the dummy user
drbh May 12, 2026
a915b9e
fix: prefer using latest kernels-data in test
drbh May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions .github/workflows/test_e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ env:
E2E_BRANCH: e2e-${{ github.event.pull_request.number || github.run_id }}-${{ github.run_attempt }}

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
Copy link
Copy Markdown
Member

@sayakpaul sayakpaul May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this change?

Copy link
Copy Markdown
Collaborator Author

@drbh drbh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change updates the concurrency group to be scoped to this workflow instead of the workflow + the pr.

this way the actions should only run one time per workflow, and not per pr, so in the case multiple PR's are open, it should only run this workflow one at a time to avoid race conditions.

since this change adds a step to delete the staging repo, if the workflows ran in parallel then it could hypothetically have a race condition if one action deleted the repo after the other action uploaded and before it downloaded in the get_kernel call.

so this change should guard against that race condition

group: ${{ github.workflow }}
cancel-in-progress: true

jobs:
Expand All @@ -45,6 +45,23 @@ jobs:
env:
USER: runner

# Remove the existing test repo if it exists so we test repo creation in the upload step
- name: Delete existing test repo
run: |
cat > /tmp/delete_repos.py << 'PYEOF'
Copy link
Copy Markdown
Member

@sayakpaul sayakpaul May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep it under scripts or workflows directory?

Copy link
Copy Markdown
Collaborator Author

@drbh drbh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can move it if you think thats a better location - however since the snippet is so small and only really expected to be used in this case I thought it made sense to inline.

lmk what you think!

from huggingface_hub import HfApi
Copy link
Copy Markdown
Member

@sayakpaul sayakpaul May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just use delete_repo() without having to initialize the API?

Copy link
Copy Markdown
Collaborator Author

@drbh drbh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I wasn't aware that was possible? can you share a snippet we can use to delete a repo in a more concise way? I was under the impression we'd need to create the api object pointing to the staging url.

Happy to make changes to a better approach!

Copy link
Copy Markdown
Member

@sayakpaul sayakpaul May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/huggingface/diffusers/blob/6abf75263a09a3e7a62458f544ce2fac28568fe2/tests/models/test_modeling_common.py#L2046

Docs: https://huggingface.co/docs/huggingface_hub/v1.12.1/en/package_reference/hf_api#huggingface_hub.HfApi.delete_repo

import os

api = HfApi(
endpoint=os.environ["HF_ENDPOINT"]
)
repo_id = os.environ["E2E_REPO_ID"]
for repo_type in ["kernel", "model"]:
api.delete_repo(repo_id, repo_type=repo_type, missing_ok=True)
print(f"Deleted {repo_type} repo (or it did not exist)")
PYEOF
nix-shell -p python3Packages.huggingface-hub --run "python /tmp/delete_repos.py"

- name: Init kernel project
run: |
cd /tmp
Expand Down Expand Up @@ -121,6 +138,8 @@ jobs:
CUDA_TAG=$(echo "$VARIANT" | grep -oP 'cu\d+')
echo "Installing torch matching variant $VARIANT (CUDA tag: $CUDA_TAG)"
uv sync --all-extras --dev
# E2E validates this checkout, so use the local kernels-data binding instead of the released wheel from uv.lock.
Copy link
Copy Markdown
Member

@sayakpaul sayakpaul May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Copy Markdown
Collaborator Author

@drbh drbh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added to ensure the E2E test is using the latest kernel-data in the repo, instead of the version from the lock file.

I ran into an issue with a difference in the api used to load the metadata in a previous action run https://github.com/huggingface/kernels/actions/runs/25754879907/job/75640986832#step:5:36

and by reinstalling the kernels-data library after syncing, it make sure to use the latest changes (and not the one pinned in the uv.lock)

uv pip install --reinstall ../kernels-data/bindings/python
uv pip install --upgrade torch --index-url https://download.pytorch.org/whl/$CUDA_TAG
uv run --no-sync python -c "import torch; print(f'torch={torch.__version__}, cuda={torch.version.cuda}, cxx11_abi={torch.compiled_with_cxx11_abi()}')"

Expand All @@ -131,7 +150,7 @@ jobs:
import torch
from kernels import get_kernel

kernel = get_kernel('${{ env.E2E_REPO_ID }}', revision='${{ env.E2E_BRANCH }}')
kernel = get_kernel('${{ env.E2E_REPO_ID }}', revision='${{ env.E2E_BRANCH }}', trust_remote_code=True)

x = torch.randn(1024, 1024, dtype=torch.float32, device='cuda')
result = kernel.kernels_upload_test(x)
Expand Down
Loading