fix(ci): deploy the Space by committing a digest pin, not restart_space - #9
Open
JonnyTran wants to merge 5 commits into
Open
fix(ci): deploy the Space by committing a digest pin, not restart_space#9JonnyTran wants to merge 5 commits into
JonnyTran wants to merge 5 commits into
Conversation
The restart API rejects OIDC tokens with a 401: a repo publisher grants write access to the repo, and restarting is a runtime operation rather than a repo write. Commit the Space's Dockerfile with the FROM line pinned to the digest the build job just pushed, which is what the credential is actually for and is HF's own documented GitHub Actions pattern. Pinning by digest also retires factory_reboot. The stale-base bug that shipped v0.7.0 as 0.6.1 was only possible because the FROM line was a moving tag; a changed digest cannot resolve to the image HF already built. Verified the rewrite against both live Space Dockerfiles: only the FROM line changes, it is idempotent, and a file without a FROM line fails loudly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe workflow now exports the built image digest, pins the Hugging Face Space Dockerfile to that digest, commits changes when needed, waits for the rebuild, and validates the ChangesHugging Face Space deployment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BuildJob
participant DockerRegistry
participant DeploySpace
participant HuggingFaceSpace
BuildJob->>DockerRegistry: Build and push image
DockerRegistry-->>BuildJob: Return image digest
BuildJob-->>DeploySpace: Provide image_digest
DeploySpace->>HuggingFaceSpace: Download Dockerfile
DeploySpace->>HuggingFaceSpace: Commit digest-pinned Dockerfile when needed
HuggingFaceSpace-->>DeploySpace: Rebuild asynchronously
DeploySpace->>HuggingFaceSpace: Validate runtime status
HuggingFaceSpace-->>DeploySpace: Return RUNNING
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-hf-space.yml:
- Around line 183-184: Update the workflow logic around api.wait_for_space so it
does not accept the prior RUNNING state immediately after upload_file. Require
evidence that the uploaded commit has triggered a rebuild—such as observing a
build-stage transition or matching the uploaded revision—before treating RUNNING
as successful, while preserving the existing timeout and polling behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0d5b89e-d41c-49cd-8832-55fbe5ad82ca
📒 Files selected for processing (2)
.github/workflows/build-hf-space.ymlCLAUDE.md
The digest-pin logic lived in a `python - <<'PY'` heredoc, where ruff and pre-commit
could not see it and nothing could test it. Move it to `scripts/hf_space.py`:
- `pin_dockerfile(text, image_ref)` — rewrite the first FROM line, nothing else
- `deploy_pinned_image(api, space_id, image_ref)` — download / compare / skip-or-
upload / wait / assert RUNNING
- `filter_prefixed(raw, prefix)` — `_filter_extralit` from deploy_pr_space.py,
now shared and pure
`scripts/deploy_space.py` is the env-reading entrypoint. `deploy-space` gains a
checkout to reach it, pinned by SHA rather than tag because that job holds the
production OIDC token. Blast radius is unchanged: for repository_dispatch the
workflow file and the checkout both resolve to the default branch, and for
workflow_dispatch both come from the selected ref.
`deploy-space` still only ever rewrites the FROM line. Rendering a whole Space over
it would wipe `public-demo`'s `.oauth.yaml` and README while the Space still reached
RUNNING and the job still went green — recorded in CLAUDE.md.
Adds pytest (13 tests, no mocking library) and a uv.lock so `uv run pytest` is
reproducible. The Dockerfile pip-installs from pyproject alone, so the dev group and
the lock do not reach the image.
`deploy_pr_space.py` replaced the duplicated Space's whole Dockerfile with a single `FROM` line. That dropped `COPY .oauth.yaml /home/extralit/`, and without that file `SecuritySettings` falls through to a bare `OAuth2Settings()` whose `_build_providers` returns no providers — so `hf_oauth: true` was inert, HF's injected OAUTH_CLIENT_ID had nothing to bind to, and every preview was reachable only via the pwgen'd admin password with `allowed_workspaces` unenforced. Same line pinned `:pr-N`, which is re-pushed every build. That is the stale-base failure that shipped v0.7.0 as 0.6.1 with a green job. Both go away by routing previews through `deploy_pinned_image`, which rewrites only the FROM line and pins the digest `build` already exposes. Previews now also wait for RUNNING, so a preview that fails to build fails the job instead of reporting green.
`duplicate_space` copies per-Space config, and `.oauth.yaml` exists only in the Space
repos — so anyone duplicating `extralit/public-demo` inherits extralit's
`allowed_workspaces`, workspaces they do not have, and nothing in git could restore
that file if it were deleted. `space_template/` is the committed answer: README,
Dockerfile, .oauth.yaml, and a manifest of files/variables/defaults.
`deploy_pr_space.py` renders it over the duplicated README and .oauth.yaml on create,
replacing the inline PR_README. `deploy-space` still does not render — see CLAUDE.md.
Placeholders are `__VAR__`: `{{VAR}}` would make the unrendered .oauth.yaml a YAML
flow mapping and fail check-yaml at commit time, `${VAR}` collides with Dockerfile
expansion, and `safe_substitute` passes unknowns through silently, which is the
silent-wipe failure itself. `render` raises on any placeholder it has no value for.
`scripts/check_space_config.py` renders with each live Space's variables and diffs —
parsed YAML, not bytes, ignoring the FROM line CI owns. Report-only, anonymous reads,
and deliberately in a workflow with no `id-token: write`. Both live Spaces match it
today, which is the evidence that the template is faithful rather than aspirational.
`manifest.json` carries no `required_secrets`: that contract already lives in the
Hub's `deployment_templates` table, and a second copy here would have no seeder.
`Path.read_text()` / `open()` use the locale encoding, and `manifest.json` carries a literal emoji default. On a runner whose locale resolves to ASCII the render would die with UnicodeDecodeError before touching the Space. Verified with LC_ALL=C.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_space_config.py`:
- Around line 70-72: Update _below_from in scripts/check_space_config.py: remove
and validate only the first primary FROM line, report drift when no FROM line
exists, and preserve any later-stage FROM lines. Add test cases in
tests/test_check_space_config.py covering a Dockerfile without FROM and one
containing an additional later-stage FROM.
In `@scripts/deploy_pr_space.py`:
- Around line 38-40: Update scripts/deploy_pr_space.py lines 38-40 and the
preview Space creation flow to render and upload the full Dockerfile, then wait
for the build it triggers before reporting success; do not merely add Dockerfile
to CREATE_FILES or alter the later FROM-only update behavior. Update
tests/test_deploy_pr_space.py lines 82-100 to assert that a newly created Space
receives the rendered Dockerfile with the digest-pinned FROM line and OAuth
copy.
In `@scripts/hf_space.py`:
- Around line 103-105: Update the deployment wait flow around the runtime
polling before wait_for_space so it verifies the uploaded revision or observes a
new intermediate build stage before accepting RUNNING, rather than relying on
time.sleep(30). Ensure stale RUNNING cannot pass, and add a regression test in
the existing HF Space tests covering a first post-upload stale RUNNING status.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f68d58fc-1aef-4bb0-bf32-17cee235a201
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/build-hf-space.yml.github/workflows/space-config.ymlCLAUDE.mdpyproject.tomlscripts/check_space_config.pyscripts/deploy_pr_space.pyscripts/deploy_space.pyscripts/hf_space.pyspace_template/.oauth.yamlspace_template/Dockerfilespace_template/README.mdspace_template/manifest.jsontests/conftest.pytests/test_check_space_config.pytests/test_deploy_pr_space.pytests/test_deploy_space.pytests/test_hf_space.pytests/test_space_template.py
Comment on lines
+70
to
+72
| def _below_from(text: str) -> str: | ||
| body = [line for line in text.splitlines() if not line.startswith("FROM ")] | ||
| return "\n".join(body).strip() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove and validate only the primary FROM line.
Line 71 removes every FROM line. The deploy helper changes only the first one. A live Dockerfile with no FROM, or with an added later-stage FROM, can normalize to the expected body and report no drift.
scripts/check_space_config.py#L70-L72: Remove exactly the firstFROMline and report drift when it is absent. Retain every laterFROMline for comparison.tests/test_check_space_config.py#L42-L54: Add cases for a Dockerfile withoutFROMand one with an additional later-stageFROM.
📍 Affects 2 files
scripts/check_space_config.py#L70-L72(this comment)tests/test_check_space_config.py#L42-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check_space_config.py` around lines 70 - 72, Update _below_from in
scripts/check_space_config.py: remove and validate only the first primary FROM
line, report drift when no FROM line exists, and preserve any later-stage FROM
lines. Add test cases in tests/test_check_space_config.py covering a Dockerfile
without FROM and one containing an additional later-stage FROM.
Comment on lines
+38
to
+40
| # The Dockerfile is rendered by deploy_pinned_image instead, so its upload is the one that | ||
| # triggers the rebuild this job then waits on. | ||
| CREATE_FILES = ["README.md", ".oauth.yaml"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Render Dockerfile when creating a preview Space.
Line 40 excludes Dockerfile, although render_space_files renders it. A new preview therefore inherits the source Space Dockerfile and only later changes its FROM line. This can preserve source-specific drift instead of establishing the committed template.
Do not only add Dockerfile to CREATE_FILES. If its rendered FROM already uses image_ref, deploy_pinned_image treats it as a no-op and does not wait for the rebuild triggered by the upload. Render and upload the Dockerfile on creation, then wait for that build.
scripts/deploy_pr_space.py#L38-L40: Upload the rendered Dockerfile on creation and wait for its build before success.tests/test_deploy_pr_space.py#L82-L100: Assert that a newly created Space receives the rendered Dockerfile with the digest-pinnedFROMline and OAuth copy.
As per coding guidelines, “For preview Spaces, render the full template only when creating a Space; thereafter update only the Dockerfile FROM line.”
📍 Affects 2 files
scripts/deploy_pr_space.py#L38-L40(this comment)tests/test_deploy_pr_space.py#L82-L100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/deploy_pr_space.py` around lines 38 - 40, Update
scripts/deploy_pr_space.py lines 38-40 and the preview Space creation flow to
render and upload the full Dockerfile, then wait for the build it triggers
before reporting success; do not merely add Dockerfile to CREATE_FILES or alter
the later FROM-only update behavior. Update tests/test_deploy_pr_space.py lines
82-100 to assert that a newly created Space receives the rendered Dockerfile
with the digest-pinned FROM line and OAuth copy.
Source: Coding guidelines
Comment on lines
+103
to
+105
| # wait_for_space returns at the first non-build poll, which is the stale RUNNING. | ||
| time.sleep(30) | ||
| runtime = api.wait_for_space(space_id, timeout=2700, poll_interval=10) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require evidence of the new rebuild before accepting RUNNING.
The 30-second delay does not prove that Hugging Face has processed the uploaded commit. In huggingface_hub==1.26.0, wait_for_space returns when its first poll sees any non-intermediate stage, including the prior RUNNING stage. A queued rebuild can therefore make this deployment pass while the Space still serves the old digest. (raw.githubusercontent.com)
Poll until the runtime enters an intermediate build stage, or verify the uploaded revision, before calling wait_for_space and accepting RUNNING. Add a regression test where the first post-upload status is stale RUNNING.
#!/bin/bash
set -euo pipefail
sed -n '81,111p' scripts/hf_space.py
sed -n '115,145p' tests/test_hf_space.py
rg -n -C 3 'time\.sleep|wait_for_space|get_space_runtime|stale|intermediate' scripts/hf_space.py tests/test_hf_space.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/hf_space.py` around lines 103 - 105, Update the deployment wait flow
around the runtime polling before wait_for_space so it verifies the uploaded
revision or observes a new intermediate build stage before accepting RUNNING,
rather than relying on time.sleep(30). Ensure stale RUNNING cannot pass, and add
a regression test in the existing HF Space tests covering a first post-upload
stale RUNNING status.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #8. The keyless
deploy-spacejob merged there fails in practice — caught by the staging smoke test (run 31247598490).What the smoke test proved
restart_space()returned 401 againstextralit-dev/develop.Crucially it was not an
OIDCError.HF_OIDC_RESOURCEwas set andget_token()raises rather than falling back, so the traceback landing insiderestart_spaceproves a valid OIDC token had already been minted. Two conclusions:extralit-dev/developis registered and its claims match — a bad publisher fails earlier, asinvalid_grant.The fix
Deploy the way HF's own GitHub Actions docs do — by writing to the Space repo, which is exactly what the credential grants.
deploy-spacenow rewrites the SpaceDockerfile'sFROMline to the digestbuildjust pushed and commits it; HF rebuilds on the new commit.buildgains animage_digestoutput for this.deploy-pr-spaceis untouched and still needs itsHF_TOKEN(Trusted Publishers cannot create repos).This also retires
factory_rebootThe v0.7.0 stale-base bug (green job, Space still serving 0.6.1) was only possible because the
FROMline was a moving tag, so HF reused the base image it had already built.factory_reboot=Truewas a workaround for that. A digest cannot resolve to a previously-built image, so the failure mode is gone at the root rather than suppressed.Verification
Exercised the rewrite against both live Space Dockerfiles:
COPY .oauth.yaml/ENVlines are preserved)FROMline fails loudly instead of silently no-oppingA no-change commit would trigger no rebuild, so that path skips the wait and instead asserts the Space's current stage is
RUNNING— a previously-failed build cannot be reported as a green redeploy.Still unverified until this merges and staging is dispatched: that the OIDC token can write to the Space repo. It is the documented purpose of a repo publisher, but #8 is a reminder that documented-adjacent is not observed.
mainis the smoke test — the publisher pinsbranch=main, so this cannot be tested from the PR branch.Docs updated in
CLAUDE.md; the monorepo counterpart is Extralit/extralit#238.Summary by CodeRabbit
Deployment
Monitoring
Documentation
Tests