prevent cross-architectural leakeage in matrix_job_re - #76
Open
cgoea wants to merge 2 commits into
Open
Conversation
…by_arch # Conflicts: # scripts/receive_therock/tests/therock_update_status_json_test.py # scripts/receive_therock/therock_update_status_json.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
status.json's matrix-cell key for PyTorch/JAX fan-out builds is parsed purely from job names as(py, ref)— it carries no GPU architecture. When a single build cell (e.g.py 3.12 | torch release/2.10) nests test jobs for multiple architectures (e.g.gfx942andgfx1101), all of those jobs collapse into one shared, worst-of variant.That conflated variant then gets broadcast onto every architecture's test leaf by
_refresh_same_run_fanout_tests, so one architecture's failure silently drags down another architecture's otherwise-passing result (and vice versa) instatus.json, even though the two GPUs' actual test outcomes are independent.closes #75
Technical Details
_TEST_ARCH_JOB_RE/_job_matches_archto detect which architecture (if any) a job names in its own"Test | <arch>"segment, so a job can be classified as either arch-agnostic (the cell's shared build step) or scoped to one specific architecture._variants_from_jobs,_derive_variants, and_create_leafnow accept an optionalarchparameter that filters the job list down to that architecture's own jobs plus any arch-agnostic jobs before deriving matrix-cell variants._refresh_same_run_fanout_testsnow re-derives each existing test leaf's variants scoped to its own architecture, instead of reusing the build leaf's arch-blind variant list wholesale. It also rolls the leaf'sstatusup from those scoped variants (previously it copied the raw run conclusion directly, which doesn't reflect the arch-specific outcome either)._merge_run_into_documentnow re-derives a per-architecture leaf when a single event legitimately reports more than one target architecture, closing the same conflation risk (and a related object-aliasing hazard) on the primary write path, not just the same-run refresh path.Test Plan
test_completed_fanout_build_does_not_leak_status_across_architectures, which sets up two architectures (gfx942passing,gfx1101failing) nested under the same(py, torch)build cell and asserts each architecture's test leaf reflects only its own outcome.gfx942's leaf incorrectly flips tofailure) and passes with the fix applied.scripts/receive_therocktest suite and checked for regressions againstdevelop.Test Result
pytest scripts/receive_therock/tests/therock_update_status_json_test.py: 99 passedpytest scripts/receive_therock/tests/: 401 passedSubmission Checklist