Skip to content

CI: Don't fail fork and Dependabot runs on unavailable secrets - #1989

Merged
yoavkatz merged 1 commit into
mainfrom
ci/guard-fork-secrets
Oct 4, 2026
Merged

yoavkatz merged 1 commit into
mainfrom
ci/guard-fork-secrets

Conversation

@yoavkatz

@yoavkatz yoavkatz commented Oct 4, 2026

Copy link
Copy Markdown
Member

Problem

GitHub withholds repository secrets from runs triggered by forks and by Dependabot. Two steps treated those secrets as mandatory and failed hard when they resolved to an empty string.

On #1986 (a Dependabot PR) this produced 11 red jobs that never ran a single test:

Job Cause
unittests, eager, preparation (0–9) empty LLMEVALKIT_SSH_KEY
performance empty UNITXT_READ_HUGGINGFACE_HUB_FOR_TESTS → HTTP 429

1. Internal pip install

.github/actions/install-internal-pip wrote the SSH key to ~/.ssh/id_ed25519 and installed from git+ssh://github.ibm.com. With an empty key:

Load key "/home/runner/.ssh/id_ed25519": error in libcrypto
git@github.ibm.com: Permission denied (publickey).
ERROR: Failed to build 'git+ssh://****@github.ibm.com/MLT/LLMEvalKit.git'

The host is IBM-internal, so it is unreachable from an external fork regardless of the secret.

2. Hugging Face login

An empty token was interpolated into huggingface-cli login --token, retried 5×, and left the run unauthenticated — which then hit the shared-runner rate limit:

urllib.error.HTTPError: HTTP Error 429: Too Many Requests

Change

Skip each step when its secret is absent, with a ::notice:: explaining why. Both guards are no-ops on trusted runs.

  • Guard lives inside the step rather than continue-on-error: at the call sites, so a genuine install failure still fails the job when the secret is available.
  • Secrets now pass through env: instead of being interpolated into the script body.
  • The key is written with printf instead of echo, which mangles values containing backslashes or a leading dash.

Why skipping the install is safe

The llmevalkit imports in src/unitxt/metrics.py are already lazy (inside methods) and declared via _requirements_list. If something actually instantiates those metrics, unitxt raises its normal Install with "pip install ..." error. Everything else is unaffected.

Not fixed here

Unauthenticated runs are still exposed to Hub rate limits — this change reports the cause instead of masking it behind five failed login attempts. Making performance label-triggered or pull_request_target-gated would address that properly, and is left as a follow-up.

Testing

🤖 Generated with Claude Code

GitHub withholds repository secrets from workflow runs triggered by forks
and by Dependabot. Two steps treated those secrets as mandatory and failed
hard when they resolved to an empty string, which is why PR #1986 showed 11
red jobs that never executed a single test.

The internal-dependency step wrote secrets.LLMEVALKIT_SSH_KEY to
~/.ssh/id_ed25519 and pip-installed from git+ssh://github.ibm.com. With an
empty key this aborts the whole job:

    Load key "/home/runner/.ssh/id_ed25519": error in libcrypto
    git@github.ibm.com: Permission denied (publickey).

That killed unittests, eager, and all ten preparation shards before the test
step. The host is IBM-internal, so it is unreachable from an external fork
regardless of the secret. Skip the install when no key is present. The
llmevalkit imports in metrics.py are already lazy and guarded by
_requirements_list, so the affected metrics raise a clear "install with..."
message if something actually instantiates them; everything else is
unaffected.

The Hugging Face login step interpolated an empty token into
`huggingface-cli login --token`, looped five times, and left the run
unauthenticated, which then hit the shared-runner rate limit:

    urllib.error.HTTPError: HTTP Error 429: Too Many Requests

Skip the login when no token is present and say so in the log, instead of
retrying a call that cannot succeed. Unauthenticated runs remain exposed to
Hub rate limits; this reports the cause rather than fixing it.

Both guards are no-ops on trusted runs. Prefer a guard inside the step over
continue-on-error at the call sites, so a real install failure still fails
the job when the secret IS available.

Also pass both secrets through env: rather than interpolating them into the
script body, and write the key with printf instead of echo, which mangles
values containing backslashes or a leading dash.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yoav Katz <katz@il.ibm.com>
@yoavkatz
yoavkatz merged commit ce5e3ac into main Oct 4, 2026
20 of 25 checks passed
@yoavkatz
yoavkatz deleted the ci/guard-fork-secrets branch October 4, 2026 14:43
wak327 added a commit to wak327/unitxt that referenced this pull request Oct 5, 2026
… is missing

ReflectionToolCallingMetric and ReflectionToolCallingMetricSyntactic
require llmevalkit, an internal package that CI installs only when the
repository secrets are available. Since IBM#1989, pull requests from forks
and Dependabot skip that install, so the 15 tests constructing these
metrics errored with MissingRequirementsError and failed the unittests
and eager jobs. Skip them when llmevalkit is not installed; they still
run wherever it is.

Signed-off-by: Waleed Khalid <wak327@gmail.com>
yoavkatz added a commit that referenced this pull request Oct 6, 2026
…template (#1983)

* fix: Raise a clear error when HFSystemFormat's tokenizer has no chat template

HFSystemFormat failed on every instance with the generic transformers
ValueError when the model's tokenizer does not define a chat template.
Check this once in prepare() and raise a UnitxtError naming the model and
how to proceed: use a model with a chat template, pass one explicitly via
chat_kwargs_dict={'chat_template': ...}, or use SystemFormat.

Signed-off-by: Waleed Khalid <wak327@gmail.com>

* test: Skip llmevalkit-based tool calling metric tests when llmevalkit is missing

ReflectionToolCallingMetric and ReflectionToolCallingMetricSyntactic
require llmevalkit, an internal package that CI installs only when the
repository secrets are available. Since #1989, pull requests from forks
and Dependabot skip that install, so the 15 tests constructing these
metrics errored with MissingRequirementsError and failed the unittests
and eager jobs. Skip them when llmevalkit is not installed; they still
run wherever it is.

Signed-off-by: Waleed Khalid <wak327@gmail.com>

---------

Signed-off-by: Waleed Khalid <wak327@gmail.com>
Co-authored-by: Yoav Katz <68273864+yoavkatz@users.noreply.github.com>
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.

1 participant