Skip to content

[CI] Add weekly overview snapshot sweep / [CI] 新增每周 overview snapshot 定时 sweep - #2588

Open
edwingao28 wants to merge 1 commit into
mainfrom
wenyaogao/weekly-overview-snapshot
Open

[CI] Add weekly overview snapshot sweep / [CI] 新增每周 overview snapshot 定时 sweep#2588
edwingao28 wants to merge 1 commit into
mainfrom
wenyaogao/weekly-overview-snapshot

Conversation

@edwingao28

Copy link
Copy Markdown
Collaborator

Closes #2586. Parent context: #2304.

Adds weekly-overview-snapshot.yml: Saturday 06:00 UTC cron (+ workflow_dispatch with config-keys override and skip-ingest smoke mode) that runs the pilot best-config list via e2e-tests.yml, then dispatches ingest-results to InferenceX-app with its own run id.

  • Pilot scope: dsv4-fp4-b200-sglang, dsv4-fp4-b300-sglang-mtp, qwen3.5-fp4-b200-sglang-mtp, qwen3.5-fp4-b300-sglang-mtp → 37 jobs, ~18 node-hours/wk
  • App side zero changes: ingest accepts any run id; missing changelog-metadata falls back to the run name as changelog description (ingest-ci-run.ts)
  • Priority: schedule events get no event bump in ci-priority.yaml → PR/main sweeps preempt
  • Partial failures still ingest (app skips failed rows)

Validation: generator command validated locally (37 entries). Cron only activates once merged to main — post-merge validation is one manual workflow_dispatch run, then confirm rows land in the DB and /overview cells show the new date.


Closes #2586。父 issue 背景:#2304

新增 weekly-overview-snapshot.yml:周六 06:00 UTC cron(另有 workflow_dispatch,支持 config-keys 覆盖与 skip-ingest 冒烟模式),通过 e2e-tests.yml 跑 pilot best-config 列表,随后用自身 run id 向 InferenceX-app 派发 ingest-results

  • Pilot 范围:dsv4-fp4-b200-sglangdsv4-fp4-b300-sglang-mtpqwen3.5-fp4-b200-sglang-mtpqwen3.5-fp4-b300-sglang-mtp → 37 个 job,约 18 node-hours/周
  • App 侧零改动:ingest 接受任意 run id;缺 changelog-metadata 时回退用 run name 作 changelog 描述(ingest-ci-run.ts
  • 优先级:schedule 事件在 ci-priority.yaml 无 event 加分 → PR/main sweep 天然抢占
  • 部分失败仍然 ingest(app 跳过失败行)

验证: generator 命令已本地验证(37 entries)。cron 需 merge 到 main 才生效——merge 后手动 workflow_dispatch 跑一次,确认数据入库且 /overview 格子显示新日期。

https://claude.ai/code/session_01Ltmsy4CWWcA1XetNR7JBev

Comment on lines +32 to +48
uses: ./.github/workflows/e2e-tests.yml
secrets: inherit
with:
test-name: weekly-overview-snapshot
generate-cli-command: >-
test-config
--config-keys ${{ inputs.config-keys ||
'dsv4-fp4-b200-sglang
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 +49 to +69
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 }}"
}
}'

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ead4a4d3a8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +8 to +9
# ingest path. `schedule` events get no event bump in ci-priority.yaml, so
# PR and main-push sweeps always preempt these jobs.

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 +15 to +18
on:
schedule:
- cron: "0 6 * * 6" # Saturday 06:00 UTC — weekend off-peak, avoids weekday PR-sweep contention
workflow_dispatch:

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 👍 / 👎.

@claude claude Bot left a comment

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.

I reviewed this PR and the inline nits above cover what I found. Beyond those, I also checked whether including configs/amd-master.yaml in --config-files (line 45) was a bug given the pilot's config-keys are all NVIDIA-only (dsv4/qwen3.5 on b200/b300) — it isn't: load_config_files just merges all provided files into one lookup dict, and unreferenced AMD keys are simply never selected, so the extra file is inert, not incorrect.

Extended reasoning...

This PR adds a single new GitHub Actions workflow file with no application code changes. Beyond the three inline nits (dead test-name input on a uses:-invoked reusable workflow, duplicated ingest-dispatch curl logic already present in run-sweep.yml/recover-reused-ingest.yml, and an inaccurate priority-preemption rationale in the header comment), I traced the config-file/config-keys resolution path in utils/matrix_logic/generate_sweep_configs.py to confirm that passing both nvidia-master.yaml and amd-master.yaml to --config-files while only requesting NVIDIA pilot keys is harmless — load_config_files merges all files into a single dict keyed by config name, and expand_config_keys only pulls the keys actually requested, so the AMD file contributes nothing and doesn't affect job count or behavior.

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).

Comment on lines +47 to +69
# rows and a partially fresh snapshot beats an empty week.
trigger-ingest:
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 }}"
}
}'

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.

Comment on lines +4 to +9
# 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[CI] Weekly overview snapshot sweep for /overview freshness / [CI] 每周 overview snapshot 定时 sweep

2 participants