Skip to content

fix: platform status stays in_progress - #86

Open
cgoea wants to merge 2 commits into
developfrom
users/cgoea/platform_rollup
Open

fix: platform status stays in_progress #86
cgoea wants to merge 2 commits into
developfrom
users/cgoea/platform_rollup

Conversation

@cgoea

@cgoea cgoea commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Motivation

Fixes ROCm/Quartz#66: a platform's summary.<platform>.status could already read failure while other pipelines on that same platform (e.g. jax, native_packages) had not reported anything yet. The rollup precedence (failure > in_progress > cancelled > success > skipped) let one pipeline's terminal failure shortcut past sibling pipelines that were still genuinely in flight and could still pass or fail on their own, so the platform's reported verdict was premature.

Technical Details

  • Added rollup_sibling_statuses() in therock_status_document.py
  • _build_platform_summary() in therock_summary.py now rolls each pipeline up to its own single status first
  • Net effect: a platform only crystallizes to failure/cancelled once every expected sibling pipeline has reported a terminal status; until then it correctly stays in_progress.

Test Plan

  • Updated therock_summary_test.py cases that previously asserted the premature-failure behavior
  • Added companion tests (*_wins_once_every_sibling_is_terminal / *_drags_platform_once_siblings_are_terminal) verifying failure still correctly wins once every sibling pipeline has reported terminally, so no coverage was lost.

Test Result

  • pytest scripts/: 475 passed

Submission Checklist

@cgoea
cgoea requested a review from a team August 25, 2026 14:50
@cgoea
cgoea changed the base branch from main to develop August 25, 2026 14:51
@HereThereBeDragons

Copy link
Copy Markdown
Collaborator

from afar i thought it makes sense to combine this with #57. what do you think?

my idea would be to extract from https://github.com/ROCm/therock/blob/main/.github/workflows/multi_arch_release.yml from the inputs the following:

    build_python_packages: true
    build_pytorch: true
    build_jax: true

and based on this and which archs are set

    linux_amdgpu_families: all
    windows_amdgpu_families: all

we can then scope the expected pipelines "live". (expected is always all pipelines and all archs. its more used to disable the expectation)

@cgoea

cgoea commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

from afar i thought it makes sense to combine this with #57. what do you think?

my idea would be to extract from https://github.com/ROCm/therock/blob/main/.github/workflows/multi_arch_release.yml from the inputs the following:

    build_python_packages: true
    build_pytorch: true
    build_jax: true

and based on this and which archs are set

    linux_amdgpu_families: all
    windows_amdgpu_families: all

we can then scope the expected pipelines "live". (expected is always all pipelines and all archs. its more used to disable the expectation)

I would keep it separate, and make a follow up PR against #57. #88 stacked on top of this one

@HereThereBeDragons HereThereBeDragons left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

overall lgtm just some changes to the doc and testing

A cancelled/failed rocm *test* gates nothing downstream, so the children stay
`in_progress` and the platform stays `in_progress` (in_progress outranks
cancelled in the worst-of).
Each pipeline (rocm, pytorch, jax, native_packages) first rolls its own

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think this can be condensed into what the user wants to have with less of historical annecdots where it comes from. more or less the thing is: if any pipeline is still in_progress the platform status collapses to in_progress.

i wonder if we even can just extend the table above - or if this is already included in the table above and we just behaved differently

assert doc.summary.linux.status is Status.in_progress


def test_multiple_pipelines_failure_wins_once_every_sibling_is_terminal() -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

seeing here some redundancy i asked claude for suggestion of test cleanup:

Recommendation (the real fix, not just deleting one test):

  1. Add one table-driven unit test on rollup_sibling_statuses directly — pure function, cheap, covers the full precedence (in_progress > failure > cancelled > success > skipped), the all-terminal-worst-wins case, and the empty→fallback case. That's where this rule belongs.
  2. With that in place, the integration trio no longer needs to re-prove the precedence — drop test_multiple_pipelines_failure_wins_once_every_sibling_is_terminal outright, and let the existing test_multiple_pipelines_aggregate_into_platform_status keep owning the masking half (the live in_progress). Tests 2 and 3 stay, justified by their distinct internal paths.
  3. Bonus: the padding in test_failure_beats_cancelled_and_success_within_one_pipeline (four success sibling leaves added just to stop the sibling rollup from masking) also becomes unnecessary noise once the precedence is unit-tested — that test can go back to being purely about within-pipeline precedence.

Net: one cheap unit test replaces the redundant integration re-proofs, and the suite gets more precise, not less covered.

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.

Platform status rollup shows already "failure" even though it is still "in_progress"

2 participants