Skip to content
Open
Changes from all commits
Commits
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
72 changes: 72 additions & 0 deletions .github/workflows/weekly-overview-snapshot.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
name: Weekly Overview Snapshot
run-name: Weekly Overview Snapshot

# Scheduled curated-config pass (not a full sweep) so every
# /overview model×hardware cell gets a same-batch data point at least weekly
# (#2304). The config-keys list is the scope knob: pilot is dsv4+qwen3.5 on
# b200/b300; extend to mi355x then gb200/gb300 once the pilot proves the
# ingest path. `schedule` events get no event bump in ci-priority.yaml, so
# PR and main-push sweeps always preempt these jobs.
Comment on lines +8 to +9

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Lower the scheduled jobs' actual queue priority

For the scheduled run, e2e-tests.yml scores the matrix using github.event_name, but configs/ci-priority.yaml boosts only push; schedule and pull_request therefore receive the same event adjustment. Moreover, these FP4 SGLang dsv4/qwen3.5 jobs receive enough model, precision, framework, and MTP bonuses to tie or outrank PR jobs on the same runner pool, so this weekly 37-job batch can be selected ahead of the PR work it is intended to yield to. Add an explicit lower schedule priority rather than relying on the absence of an event bump.

Useful? React with 👍 / 👎.

Comment on lines +4 to +9

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 The comment's rationale is wrong: configs/ci-priority.yaml only defines adjustments.event.push: 2.0, so PR sweeps (event_name pull_request/workflow_dispatch) get the same +0 event bump as this schedule-triggered snapshot — only merged main-push sweeps get the +2.0. Since the pilot configs here accumulate large job-level bumps (fp4 + mtp + sglang + dsv4/qwen3.5 prefix can total +2.75 over base), this snapshot can actually outscore and preempt active PR CI in the runner queue. Please scope the 'PR/main sweeps always preempt' claim to main-push only, since it's the stated justification for running this unattended.

Extended reasoning...

The workflow header comment (and the PR description) states: "schedule events get no event bump in ci-priority.yaml, so PR and main-push sweeps always preempt these jobs." This is used as the load-bearing justification for letting the snapshot run unattended every Saturday without worrying about contention with active development traffic.

Checking configs/ci-priority.yaml, adjustments.event is defined as:

adjustments:
  event:
    push: 2.0

There is no pull_request or workflow_dispatch key. utils/ci_priority.py's calculate_priority() computes score += adjustments.get('event', {}).get(context.event_name, 0), so any event name other than push — including pull_request, workflow_dispatch, and schedule — contributes exactly +0.

The actual PR-sweep entrypoint, run-sweep.yml, invokes ci_priority.py with --event-name "${{ github.event_name }}". For a same-repo PR sweep that value is pull_request (+0), and trusted-external-sweep.yml dispatches fork-PR sweeps via workflow_dispatch (also +0). This weekly snapshot calls e2e-tests.yml, which likewise passes --event-name "${{ github.event_name }}" — for the cron trigger that's schedule (+0). So the snapshot and pre-merge PR sweeps sit at identical event-priority footing; the only event that gets a bump is a sweep that has already merged to main (push: 2.0).

This breaks the stated non-interference guarantee: it's not just imprecise, it's the opposite of what's claimed for the pre-merge case. Worse, the pilot configs chosen for this snapshot (dsv4-fp4-b300-sglang-mtp, qwen3.5-fp4-b300-sglang-mtp, etc.) stack large job-level adjustments — model-prefix (dsv4/qwen3.5: +0.75) + precision fp4 (+0.75) + spec-decoding mtp (+0.75) + framework sglang (+0.5) on top of base 1.0 — reaching a priority score of 3.75. An ordinary PR-sweep job without those attributes (e.g. an fp8, non-MTP job) scores only around 1.5. Since the score-ordered runner queue schedules higher scores first, this snapshot can win shared GPU runners over active, unmerged PR CI — directly contradicting the comment's claim that "PR ... sweeps always preempt these jobs."

Concrete walkthrough: a contributor opens a PR that triggers run-sweep.yml on pull_request for a vanilla fp8 config job. Its priority = base 1.0 + framework bump 0.5 (if sglang/vllm) = 1.5, no event bump since pull_request isn't in the event adjustments map. Simultaneously, this Saturday cron fires with event_name: schedule, also +0 event bump, but for a dsv4-fp4-b300-sglang-mtp job scores 1.0 + 0.75 (dsv4) + 0.75 (fp4) + 0.75 (mtp) + 0.5 (sglang) = 3.75. On a shared runner pool, the snapshot job's higher score lets it preempt/queue ahead of the PR sweep job — the exact opposite of "PR sweeps always preempt these jobs."

To fix: correct the comment (and PR description) to scope the preemption claim to main-push sweeps only, e.g. "main-push sweeps get a +2.0 event bump and will preempt this job; PR sweeps do not get an event bump and may compete with it for runners." This is a documentation/design-rationale correction, not a functional bug — the workflow still runs correctly, and the Saturday 06:00 UTC off-peak scheduling plus cancel-in-progress: false reduce real-world contention risk regardless of the comment's accuracy.


concurrency:
group: weekly-overview-snapshot
cancel-in-progress: false

on:
schedule:
- cron: "0 6 * * 6" # Saturday 06:00 UTC — weekend off-peak, avoids weekday PR-sweep contention
workflow_dispatch:
Comment on lines +15 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Document the recurring sweep and production ingest path

This introduces a new scheduled/manual sweep, configuration override, partial-failure policy, and production database dispatch, but neither docs/ci-procedures.md nor its synchronized Chinese counterpart describes how to operate, verify, or recover it. Add the workflow to the focused CI guide and update docs/ci-procedures_zh.md so operators do not have to infer this production path solely from YAML.

AGENTS.md reference: AGENTS.md:L8-L8

Useful? React with 👍 / 👎.

inputs:
config-keys:
description: "Space-separated master-config keys to run (default: pilot list)"
required: false
type: string
skip-ingest:
description: "Skip the production DB ingest (smoke-test the sweep only)"
required: false
type: boolean
default: false

jobs:
# dsv4 on b200 deliberately uses the vllm-mtp entry: the sglang/trt entries
# pin the `b200-dsv4` virtual runner pool, whose scheduler queue is not
# currently served (jobs sit queued for days while b200-dgxc idles).
sweep:
uses: ./.github/workflows/e2e-tests.yml
secrets: inherit
with:
test-name: weekly-overview-snapshot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 The test-name: weekly-overview-snapshot input on line 35 has no effect: e2e-tests.yml is invoked via uses: (a reusable workflow call), and its only consumer of inputs.test-name is its own run-name (line 2), which GitHub Actions never evaluates for workflows invoked this way. Since it also doesn't feed the app-side changelog fallback described in the PR (that fallback uses this file's own static run-name), consider removing the dead input or wiring it into real metadata.

Extended reasoning...

weekly-overview-snapshot.yml calls e2e-tests.yml as a reusable workflow via jobs.sweep.uses: ./.github/workflows/e2e-tests.yml, passing test-name: weekly-overview-snapshot (line 35). Inside e2e-tests.yml, inputs.test-name is referenced in exactly one place: line 2, run-name: e2e Test - ${{ inputs.test-name || ... }}. It is declared as an input under both workflow_dispatch (line 15) and workflow_call (line 95), but it is never read by any job, step, matrix-generation call, or ingest payload.

The problem is that run-name is only evaluated for the top-level workflow run created by the triggering event. When a workflow is invoked via jobs.<id>.uses: (a workflow_call, as is the case here), it does not create its own top-level run — its jobs execute nested inside the caller's run, and the called workflow's run-name key is never evaluated. The run that actually shows up in the Actions UI keeps weekly-overview-snapshot.yml's own static run-name: Weekly Overview Snapshot (line 2 of this file). So the test-name value passed on line 35 is inert for this invocation path.

This is worth flagging because it creates a false impression: a reader sees test-name: weekly-overview-snapshot being threaded through and reasonably assumes the sweep run is tagged/labeled with it, and might further assume that tag feeds the app-side changelog fallback mentioned in the PR description ("missing changelog-metadata falls back to run name"). It doesn't — that fallback resolves via source-run-id: ${{ github.run_id }} in the ingest step, which maps back to this file's own static run name, not to the test-name string at all.

Proof: (1) grep e2e-tests.yml for test-name — only three hits: the workflow_dispatch input declaration (line 15), the workflow_call input declaration (line 95), and the run-name expression (line 2). (2) This PR's sweep job invokes e2e-tests.yml via uses:, not workflow_dispatch — contrast with trusted-external-sweep.yml's createWorkflowDispatch caller, where run-name genuinely is evaluated because that path fires a real workflow_dispatch event. (3) GitHub's documented behavior: reusable-workflow calls run as job groups nested in the caller's run; the called workflow's run-name is not rendered anywhere in the UI for that invocation. (4) Therefore the Actions UI for this sweep will only ever show "Weekly Overview Snapshot" — never anything derived from test-name.

This is a pure quality/cleanup issue with no functional or correctness impact — nothing breaks, no job fails, and the ingest step and DB rows are unaffected. The fix is simply to drop the dead test-name: line (or, if per-caller labeling is actually wanted, wire it into something that's evaluated in this call path, e.g. an env var surfaced in a step summary).

generate-cli-command: >-
test-config
--config-keys ${{ inputs.config-keys ||
'dsv4-fp4-b200-vllm-mtp
dsv4-fp4-b300-sglang-mtp
qwen3.5-fp4-b200-sglang-mtp
qwen3.5-fp4-b300-sglang-mtp' }}
--no-evals
--config-files configs/nvidia-master.yaml configs/amd-master.yaml

# Fires even when some sweep jobs fail — ingest skips failed
# rows and a partially fresh snapshot beats an empty week.
trigger-ingest:
Comment on lines +35 to +51
needs: sweep
if: >-
always() &&
needs.sweep.result != 'cancelled' &&
needs.sweep.result != 'skipped' &&
inputs.skip-ingest != true
runs-on: ubuntu-latest
steps:
- name: Trigger database ingest
run: |
curl -sSf -X POST \
-H "Authorization: Bearer ${{ secrets.INFX_FRONTEND_PAT }}" \
-H "Accept: application/vnd.github+v3+json" \
https://api.github.com/repos/SemiAnalysisAI/InferenceX-app/dispatches \
-d '{
"event_type": "ingest-results",
"client_payload": {
"source-run-id": "${{ github.run_id }}",
"merge-run-id": "${{ github.run_id }}"
}
}'
Comment on lines +52 to +72
Comment on lines +50 to +72

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 This job's ingest curl block is a near-verbatim 4th copy of the repository_dispatch dispatch already implemented in run-sweep.yml (2 copies) and recover-reused-ingest.yml — same auth header, endpoint, event_type, and client_payload shape. Not blocking, but a shared composite action (e.g. .github/actions/trigger-ingest taking source-run-id/merge-run-id/event_type as inputs) would let future changes to the ingest contract be made in one place instead of four.

Extended reasoning...

The new trigger-ingest job in weekly-overview-snapshot.yml (lines 47-69) re-implements the exact same repository_dispatch API call that already exists in three other places in this repo: run-sweep.yml's trigger-ingest job (~line 1078), run-sweep.yml's trigger-agentic-ingest job (~line 1137), and recover-reused-ingest.yml (~line 21). All four use curl -sSf -X POST against https://api.github.com/repos/SemiAnalysisAI/InferenceX-app/dispatches, the same Authorization: Bearer ${{ secrets.INFX_FRONTEND_PAT }} and Accept: application/vnd.github+v3+json headers, and a client_payload shaped around source-run-id/merge-run-id.

The specific code path here is straightforward: whenever the sweep job in this new workflow completes (subject to the always() guard and skip-ingest check), the trigger-ingest step shells out to the same dispatch endpoint with event_type: ingest-results and both source-run-id and merge-run-id set to github.run_id. This is structurally identical to run-sweep.yml's trigger-ingest job, just with a simpler payload (no reuse-source-run-id branching, since this workflow always runs a full pilot sweep rather than reusing artifacts).

Nothing in the existing code prevents this duplication because there is no shared building block for it — each workflow file independently hardcodes the curl invocation inline rather than calling out to a composite action or reusable workflow. As a result, this PR's author (reasonably) copied the pattern from run-sweep.yml rather than being pointed at a shared abstraction, because none exists.

The impact is purely maintenance cost, not a functional bug: if the ingest contract ever changes (e.g. the auth secret is rotated to a different name, the endpoint moves, event_type values are renamed, or a new required payload field is added), a maintainer now has to find and update four separate inline curl blocks instead of one. It's easy to miss one of the four during such a change, silently breaking ingest for whichever workflow was missed.

Step-by-step illustration: Suppose the app team renames the ingest-results event type to ingest-results-v2\% as part of a payload schema migration. A maintainer greps for ingest-resultsand finds it inrun-sweep.yml(trigger-ingest job) and updates it, verifies CI passes on the next PR sweep, and calls it done — but missesweekly-overview-snapshot.ymlbecause it's a newer, less-frequently-touched file. The next Saturday cron run's ingest dispatch then fails silently (or is ignored by the app side if it still nominally accepts the old event type but processes it differently), and nobody notices until the/overview` snapshot data goes stale.

Suggested fix: Extract the dispatch into a composite action, e.g. .github/actions/trigger-ingest/action.yml, parameterized by event-type, source-run-id, merge-run-id, and any optional extra payload fields, and have all four call sites (run-sweep.yml x2, recover-reused-ingest.yml, and this new workflow) invoke it. This is a nice-to-have cleanup rather than something that needs to happen before merge — the current PR's copy is not itself incorrect, it just perpetuates an existing duplication pattern.