Skip to content

[CI] Parity: carry commit SHA labels through log + running-time parsing#3362

Open
ethanwee1 wants to merge 2 commits into
developfrom
ethanwee/parity-commit-label-support
Open

[CI] Parity: carry commit SHA labels through log + running-time parsing#3362
ethanwee1 wants to merge 2 commits into
developfrom
ethanwee/parity-commit-label-support

Conversation

@ethanwee1

@ethanwee1 ethanwee1 commented Jun 23, 2026

Copy link
Copy Markdown

Split out of #3210 (1 of 3) for easier review.

Consumer-side support for commit-vs-commit parity (baseline_sha mode):

  • detect_log_failures.py: preserve the short commit-SHA prefix on log filenames (e.g. 09e0c59b_rocm3.txt) as the platform label, matching the SHA-prefixed filenames download_testlogs produces in baseline mode.
  • summarize_xml_testreports.py: name the per-test-file running-time CSV columns from the resolved set names (set1/set2) instead of hardcoded rocm_/cuda_, and add per-config test-shard counts.

The download_testlogs run-selection side of the original commit already landed in develop via #3278, so this PR carries only the remaining consumer-side parsing changes.

Made with Cursor


Rebased onto current develop + validated

This PR was rebased onto current develop (it was previously opened against an old lineage and showed unrelated files). Validation parity run on the clean SHA ba9910c6 (mi300): https://github.com/ethanwee1/pytorch/actions/runs/28884939416

Commit-vs-commit validation (SHA labels)

Parity run ba9910c6 vs 8b3ff3b4 (mi300): https://github.com/ethanwee1/pytorch/actions/runs/28888069456 — confirms labels follow the two SHAs (e.g. BA9910C6ONLY, SKIPPED (on ba9910c6, but not on 8b3ff3b4)).

@rocm-repo-management-api

rocm-repo-management-api Bot commented Jun 23, 2026

Copy link
Copy Markdown

Jenkins build for 1f07ad538148fd18634c2375c520d91165424255 commit finished as FAILURE
Links: Pipeline Overview / Build artifacts / Test Results

@pablo-garay pablo-garay self-requested a review June 23, 2026 19:38
set2_test_shards_col = f"{set2_name}_test_shards"
set1_passed_col = f"{set1_name}_passed"
set1_skipped_col = f"{set1_name}_skipped"
set1_missed_col = f"{set1_name}_missed"

@jithunnair-amd jithunnair-amd Jun 24, 2026

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 don't know how far you want to take this "secularizing" of set1/set2 naming instead of ROCm/CUDA, but there are still many occurrences in the file that refer to ROCm/CUDA. Just something to keep in mind if you really want to make everything consistent. Not everything in this PR probably.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed — I've kept this PR scoped to the user-facing output: the running-time CSV columns and the printed summary now both follow set1/set2. The remaining ROCm/CUDA occurrences are internal variable names (test_file_level_ROCm, *_DEFAULT counters, etc.); I left those untouched to keep the diff focused, and we can do a broader internal rename as a separate cleanup if you'd prefer.

print( "SKIPPED_DISTRIBUTED, MISSED_DISTRIBUTED, ROCMONLY_DISTRIBUTED, CUDA_DISTRIBUTED, ROCM_DISTRIBUTED" )
print( str(SKIPPED_DISTRIBUTED) + ", " + str(MISSED_DISTRIBUTED) + ", " + str(ROCMONLY_DISTRIBUTED) + ", " + str(CUDA_DISTRIBUTED) + ", " + str(ROCM_DISTRIBUTED) )
print( " " )
print( "=====Inductor GPU Number=====" )

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.

Even if we restrict this PR's secularization of set1/set2 to just user-facing stuff, then this seems to also be a candidate for being updated, since this is still printing as ROCm/CUDA

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done in 96845a7 — the printed summary headers now derive from set1_name/set2_name too, so they stay consistent with the CSV columns. Normal-mode output is unchanged (ROCMONLY_DEFAULT, CUDA_DEFAULT, ROCM_DEFAULT / ROCM_RUNNING_TIME, CUDA_RUNNING_TIME); in commit-vs-commit mode they now show the SHA labels instead of mislabeling them ROCm/CUDA.

@jithunnair-amd jithunnair-amd 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.

Please change any user-facing parts of the script to remove the ROCm/CUDA hardcoding (variable names can wait for another PR, if desired).

Also, IIUC, this PR is trying to service the sha-vs-baseline-sha use-case. If so, please mention a validation run so that reviewers can verify the output.

@rocm-repo-management-api

rocm-repo-management-api Bot commented Jun 24, 2026

Copy link
Copy Markdown

Jenkins build for 96845a7bbc10d9d44402cc9b05af3f42bc87d7e1 commit finished as FAILURE
Links: Pipeline Overview / Build artifacts / Test Results

ethanwee1 added 2 commits July 7, 2026 16:39
Consumer-side support for commit-vs-commit parity (baseline_sha mode):

- detect_log_failures.py: preserve the short commit-SHA prefix on log
  filenames (e.g. 09e0c59b_rocm3.txt) as the platform label, matching the
  SHA-prefixed filenames download_testlogs produces in baseline mode.
- summarize_xml_testreports.py: name the per-test-file running-time CSV
  columns from the resolved set names (set1/set2) instead of hardcoded
  rocm_/cuda_, and add per-config test-shard counts.

The download_testlogs run-selection side of this work already landed in
develop via #3278, so this PR carries only the remaining consumer-side
parsing changes.
The running-time CSV columns use set1_name/set2_name (rocm/cuda by default, commit SHAs in commit-vs-commit mode), but the printed summary still hardcoded ROCM/CUDA. Drive the user-facing summary headers from the same set names so both stay consistent; normal-mode output is unchanged (rocm/cuda).
@ethanwee1 ethanwee1 force-pushed the ethanwee/parity-commit-label-support branch from 96845a7 to bfaa5c6 Compare July 7, 2026 16:39
@rocm-repo-management-api

rocm-repo-management-api Bot commented Jul 7, 2026

Copy link
Copy Markdown

Jenkins build for bfaa5c6f029a7bb007467c4b4d574c05459e1cce commit finished as ABORTED
Links: Pipeline Overview / Build artifacts / Test Results

Detected error during Pytorch building:

-- HIP architectures: gfx90a;gfx908;gfx942;gfx1033
CMake Warning (dev) at /opt/rocm/lib/cmake/hip/hip-config-amd.cmake:98 (message):
   GPU_TARGETS was not set, and system GPU detection was unsuccsessful.
   
   The amdgpu-arch tool failed:
   Error: 'Failed to get device count'
   Output: ''
   
   As a result, --offload-arch will not be set for subsequent
   compilations, and the default architecture
   (gfx906 for dynamic build / gfx942 for static build) will be used

@ethanwee1

Copy link
Copy Markdown
Author

Thanks for the review. Addressed:

  • User-facing ROCm/CUDA hardcoding removed. The printed summary labels and the running-time CSV column headers now follow the set1/set2 naming (which default to rocm/cuda, or become the two commit short-SHAs in commit-vs-commit mode). The remaining ROCm/CUDA tokens are internal variable names only, which I've left for a follow-up per your note.
  • Validation run (commit-vs-commit / sha-vs-baseline_sha), mi300: https://github.com/ethanwee1/pytorch/actions/runs/28888069456ba9910c6 vs 8b3ff3b4. The output labels come out SHA-based, e.g.:
    • SKIPPED (on ba9910c6, but not on 8b3ff3b4)
    • BA9910C6ONLY (PASSED on BA9910C6, NOT PASSED on 8B3FF3B4)
    • DISAGREE [(SKIPPED+MISSED)/8B3FF3B4] %

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.

3 participants