ci: Set HF_HUB_OFFLINE=1 during tests when PR is from a fork#1174
ci: Set HF_HUB_OFFLINE=1 during tests when PR is from a fork#1174terrykong merged 1 commit intoNVIDIA-NeMo:mainfrom
Conversation
Signed-off-by: Charlie Truong <chtruong@nvidia.com>
📝 WalkthroughWalkthroughAdds a new is_fork_pr input to the test-template composite action and plumbs it through the main CI workflow jobs to conditionally set HF_HUB_OFFLINE=1 during docker-based test runs for forked pull requests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant WF as cicd-main.yml
participant ACT as test-template (composite)
participant Docker as docker run
Dev->>GH: Open PR / push
GH->>WF: Trigger workflow (pull_request or push)
WF->>WF: Compute is_fork_pr (fork PR if head != base)
WF->>ACT: Invoke test-template with is_fork_pr
ACT->>ACT: Check is_fork_pr
alt Fork PR
ACT->>Docker: Run tests with env HF_HUB_OFFLINE=1
else Not fork PR
ACT->>Docker: Run tests without HF_HUB_OFFLINE
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/actions/test-template/action.yml (2)
178-179: Also set TRANSFORMERS_OFFLINE for consistency; simplify the conditional flag injection.Setting only HF_HUB_OFFLINE=1 can still leave Transformers trying online paths. Recommend setting both flags, and (optionally) avoid the “empty expansion + trailing backslash” brittle pattern.
Minimal in-place fix:
- ${{ inputs.is_fork_pr == 'true' && '--env HF_HUB_OFFLINE=1' || '' }} \ + ${{ inputs.is_fork_pr == 'true' && '--env HF_HUB_OFFLINE=1 --env TRANSFORMERS_OFFLINE=1' || '' }} \More robust (optional) approach defined just before docker run:
# before docker run HF_OFFLINE_FLAGS="" if [[ "${{ inputs.is_fork_pr }}" == "true" ]]; then HF_OFFLINE_FLAGS="--env HF_HUB_OFFLINE=1 --env TRANSFORMERS_OFFLINE=1" fiand then:
docker run ... $HF_OFFLINE_FLAGS \
170-178: Optional: Skip passing HF_TOKEN into the container on forks.Harmless as-is (env will be empty), but you can avoid confusion by gating it:
# example pattern [[ "${{ inputs.is_fork_pr }}" == "true" ]] || TOKEN_FLAG="--env HF_TOKEN" docker run ... ${TOKEN_FLAG:-} \.github/workflows/cicd-main.yml (1)
228-228: Fork detection expression looks good; add quotes and verify non‑PR events.
- The boolean expression is standard and should short‑circuit, but to avoid YAML boolean coercion, pass it as a quoted string.
- Please confirm it does not evaluate github.event.pull_request.* on merge_group/workflow_dispatch (shouldn’t due to short‑circuit).
Suggested tweak:
- is_fork_pr: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name }} + is_fork_pr: "${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name }}"If preferred, you can also compare to the base repository owner/repo:
- github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name + github.event.pull_request.head.repo.full_name != github.repositoryTo verify behavior across event types, you can dispatch a dry-run on merge_group and ensure the input resolves to "false".
Also applies to: 257-257, 281-281
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/test-template/action.yml(2 hunks).github/workflows/cicd-main.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-container / main
- GitHub Check: Lint check
- GitHub Check: Lint check
- GitHub Check: Post automodel integration comment / Comment on PR
- GitHub Check: Post submodule check comment / Comment on PR
🔇 Additional comments (1)
.github/actions/test-template/action.yml (1)
57-60: LGTM on new input.The is_fork_pr input is well-documented and defaulted to "false". No issues.
…NeMo#1174) Signed-off-by: Charlie Truong <chtruong@nvidia.com>
…NeMo#1174) Signed-off-by: Charlie Truong <chtruong@nvidia.com> Signed-off-by: yuanhangs <yuanhangs@nvidia.com>
What does this PR do ?
Set HF_HUB_OFFLINE=1 during tests when PR is from a fork
Pull requests from forks do not have access to secrets like HF_TOKEN, so their CI tests will fail when downloading HF models.
Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit
New Features
Tests
Chores
Impact