fix(ci): dedupe two-stage consumer jobs in dispatch groups - #10231
fix(ci): dedupe two-stage consumer jobs in dispatch groups#10231andrewwhitecdw wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesWorkflow dispatch cleanup
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_finalize_workflow_dispatch_groups.py (1)
60-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: Assert exact occurrence counts for the shared job in both locations. The current checks would pass if the consumer appeared multiple times under
two_stage; asserting one standalone occurrence before finalization and exactly one consumer occurrence afterward would make the regression guard against duplicate scheduling more robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 405ebbec-c28f-4923-9cbf-57dda07829e5
📒 Files selected for processing (1)
tests/test_finalize_workflow_dispatch_groups.py
alliepiper
left a comment
alliepiper
left a comment
There was a problem hiding this comment.
I'd suggest dropping the test -- it isn't integrated and doesn't run automatically, so it'd just be dead code.
There really isn't existing test infra for these CI scripts, and adding it should be out of scope for a typo fix PR.
Otherwise the fix is correct, lgtm
## Summary
`build-workflow.py`'s `finalize_workflow_dispatch_groups` deduplicates jobs
that appear both in a dispatch group's `standalone` array and as
producer/consumer jobs of a `two_stage` entry, so each job executes exactly
once. Jobs that were duplicated as two-stage **consumers** were never removed
from the `standalone` array, so they were scheduled (and executed) twice:
once standalone and once as a consumer.
## Root cause
The consumer-removal loop passes the wrong variable to the dedup helper:
```python
for consumer in two_stage_job['consumers']:
if remove_dispatch_job_from_container(producer, unique_standalone_jobs):
print(f"Removing standalone job '{consumer['name']}' " +
f"as it appears as a consumer in '{group_name}'", ...)
```
`producer` is the leftover loop variable from the preceding
`for producer in ...` loop. Since the producer was already removed from
`unique_standalone_jobs` by that earlier loop, the second removal attempt
always returns `False`, so no consumer standalone job is ever removed and
the "appears as a consumer" notice is never printed.
## Fix
Pass `consumer` instead of `producer`:
```python
if remove_dispatch_job_from_container(consumer, unique_standalone_jobs):
```
## Testing
No automated regression test is included per reviewer feedback: the project
has no existing test infrastructure for these CI scripts, and adding one is
out of scope for a typo fix PR.
The fix was verified manually by exercising `finalize_workflow_dispatch_groups`
with a synthetic dispatch group where job `test job` appears both standalone
and as a two-stage consumer:
- With the fix: `standalone after finalize: []` / PASS (duplicated
standalone removed, consumer notice printed).
- With the unmodified code: `FAIL: standalone not emptied: ['test job']` —
reproduces the bug.
## Why existing tests missed it
This scripting layer has no unit tests; the duplication only manifests as a
silently double-scheduled CI job, which CI itself does not flag as a failure.
---
Ported from NVIDIA/cccl-gha#1 at the request of @alliepiper — the same typo
was still present in this repo.
a9f6466 to
6c1296d
Compare
|
Thanks a lot, that looks like a valid fix ^^ |
|
/ok to test 6c1296d |
3 participants
Summary
build-workflow.py'sfinalize_workflow_dispatch_groupsdeduplicates jobsthat appear both in a dispatch group's
standalonearray and asproducer/consumer jobs of a
two_stageentry, so each job executes exactlyonce. Jobs that were duplicated as two-stage consumers were never removed
from the
standalonearray, so they were scheduled (and executed) twice:once standalone and once as a consumer.
Root cause
The consumer-removal loop passes the wrong variable to the dedup helper:
produceris the leftover loop variable from the precedingfor producer in ...loop. Since the producer was already removed fromunique_standalone_jobsby that earlier loop, the second removal attemptalways returns
False, so no consumer standalone job is ever removed andthe "appears as a consumer" notice is never printed.
Fix
Pass
consumerinstead ofproducer:Testing
No automated regression test is included per reviewer feedback: the project
has no existing test infrastructure for these CI scripts, and adding one is
out of scope for a typo fix PR.
The fix was verified manually by exercising
finalize_workflow_dispatch_groupswith a synthetic dispatch group where job
test jobappears both standaloneand as a two-stage consumer:
standalone after finalize: []/ PASS (duplicatedstandalone removed, consumer notice printed).
FAIL: standalone not emptied: ['test job']—reproduces the bug.
Why existing tests missed it
This scripting layer has no unit tests; the duplication only manifests as a
silently double-scheduled CI job, which CI itself does not flag as a failure.
Ported from NVIDIA/cccl-gha#1 at the request of @alliepiper — the same typo
was still present in this repo.