[ci] Reorganizing artifact upload by geomin12 · Pull Request #730 · ROCm/TheRock

geomin12 · 2025-05-28T22:46:03Z

Changes:

Moving log upload, job summary upload and artifact upload to a script
If the owner and repo are not ROCm/TheRock, artifacts are uploaded to the external s3 bucket
Using the third-party indexer.py file now

…min12/s3-organization

ScottTodd · 2025-05-29T22:58:59Z

+def create_index_file(args):
+    logging.info("Creating index file")
+    index_file_path = THEROCK_DIR / "third-party" / "indexer" / "indexer.py"
+    build_dir = args.build_dir
+
+    subprocess.run(
+        [sys.executable, index_file_path, "-f", "*.tar.xz*", build_dir / "artifacts"]
+    )


This overlaps with some work that @erman-gurses has been doing over at #648, moving the log file uploading and maybe log index page generation into the teatime.py script invoked by the build system.

A few things to consider about the architecture:

We should have flexible scripts that can be reused from multiple different contexts, like CI jobs and on developer machines. The log indexer and artifact fetcher are both good examples right of that right now. As we add more composite scripts like this one, we should be careful to preserve that property.

Do we want "artifact upload" to be scoped to github actions, or can it be more generic? I think here we could move this script up a level and keep the github-specific steps behind environment variables, as is already done in teatime.py:

TheRock/build_tools/teatime.py

Lines 29 to 32 in e6ecc6b

CI systems can set `TEATIME_LABEL_GH_GROUP=1` in the environment, which will

cause labeled console output to be printed using GitHub Actions group markers

instead of line prefixes. This causes the output to show in the log viewer

with interactive group handles.

ScottTodd · 2025-05-29T23:13:29Z

+          python build_tools/github_actions/artifact_upload.py \
+          --repo ${{ github.repository }} \
+          --run-id ${{ github.run_id }} \
+          --amdgpu-family ${{ env.AMDGPU_FAMILIES }} \
+          --build-dir build/


This has come up in the review of @erman-gurses 's #648 a bit, but I do like how this collapses the .yml down to a single python script invocation instead of inlining the individual subcommands and bash conditions.

Where it interfaces with that PR is that the build logs will be uploaded to S3 during the build, either at the end of the build or continuously (streaming). The build log index can either also be generated by the same process or it can be generated here at the end in the action. Longer term I think we should consider having a dynamic index page or some server-side processing so we don't need to think as carefully during the build about generating a static index page and uploading it along with the otherwise loose log files.

At some point, I need integrate the log part into teatime.py script. I need to change the architecture of #648 anyway. If @ScottTodd thinks this will make teatime.py script integration easier, I am ok to land this.

I saw that too, I think i'll split this up into more scripts, that way it'll be easier to pinpoint where to add the teatime script

I would like that. The separation will be easier for me.

erman-gurses · 2025-06-02T20:56:33Z

The PR does not only handle the artifacts here but also the logs. I think artifact_upload.py does not reflect the full functionality here

erman-gurses

LGTM! Couple of minor things.

erman-gurses · 2025-06-02T20:59:32Z

        with:
          repository: "ROCm/TheRock"
+          # for testing purposes
+          ref: users/geomin12/s3-organization


you can remove this after the testing is done.

Run working in rocm-libraries

Were these ref: changes even needed on this PR itself? I'm a bit confused about the sequencing here. Prefer to keep test changes out of PRs so we can accurately review the changes as they will be merged.

no it is not! i just added it for testing purposes to have it in rocm-libraries. i'll remove it now and have this run through

ScottTodd

Looking pretty good

ScottTodd · 2025-06-02T21:44:29Z

+    parser.add_argument(
+        "--amdgpu-family", type=str, required=True, help="AMD GPU family to upload"
+    )


I'd really like to remove this amdgpu-family plumbing, to keep as much of our infrastructure as generic as possible.

Once we move the index generation server side there will only be one usage in this file:

s3_base_path = f"{bucket_uri}/logs/{amdgpu_family}"

For that we can treat it as a generic "logs subdirectory". In fact, we could even make that change right now, just across all the other places that also look at the string (e.g. the index page url).

geomin12 · 2025-06-03T17:05:43Z

Run working in rocm-libraries

ScottTodd

Bit of clean up then this looks good to me

ScottTodd · 2025-06-03T19:47:38Z

        with:
          repository: "ROCm/TheRock"
+          # for testing purposes
+          ref: users/geomin12/s3-organization


Run working in rocm-libraries

Were these ref: changes even needed on this PR itself? I'm a bit confused about the sequencing here. Prefer to keep test changes out of PRs so we can accurately review the changes as they will be merged.

ScottTodd · 2025-06-03T19:50:33Z

-        shell: powershell
        run: |
-          $Env:PATH += ";C:\Program Files\Amazon\AWSCLIV2"
-          python3 build_tools/upload_logs_to_s3.py `
-            --build-dir=${{ env.BASE_BUILD_DIR_POWERSHELL }} `


Looks like BASE_BUILD_DIR_POWERSHELL is no longer used thanks to these scripts replacing shell: powershell usage. We can remove that variable in a follow-up.

geomin12 added 11 commits May 27, 2025 17:12

Moving S3 upload to script

267a2b5

Fixing multi line

10b3179

fixing args

5bd689a

removing space

db2cfbc

Adding correct logs

71ae144

Adding external folder opp

8e0d6b0

removing quotes

e0fc665

Adding external repo portion

a938a3f

merge conflict

9a2a5a6

Fixing URL

7303b19

Fixing the logging

2602d55

geomin12 requested review from ScottTodd and erman-gurses May 28, 2025 22:46

github-project-automation Bot added this to TheRock Triage May 28, 2025

github-project-automation Bot moved this to TODO in TheRock Triage May 28, 2025

geomin12 added 8 commits May 29, 2025 09:19

attempting temp sha

332b190

try with develop

f131a5f

Adding cmake updates

7ca5b15

changing sha

231a9a5

Merge branch 'main' of https://github.com/ROCm/TheRock into users/geo…

842d5b0

…min12/s3-organization

changing ref

9d5c24e

Adding temp sha for prim

669aa3c

Adding log statement

5376ae4

ScottTodd reviewed May 29, 2025

View reviewed changes

PR comments

b1359b3

geomin12 mentioned this pull request May 29, 2025

[monorepo] Moving monorepo_configure_ci to rocm-libraries #740

Merged

geomin12 added 2 commits May 29, 2025 16:59

more PR comments

3a6e66d

Fixing shlex type

cfec363

geomin12 marked this pull request as ready for review May 30, 2025 16:05

geomin12 mentioned this pull request May 30, 2025

[TheRock CI] Adding dynamic selecting of TheRock tests ROCm/rocm-libraries#125

Closed

geomin12 added 4 commits May 30, 2025 14:56

Pr comments

353f95f

Adding sys path

a5c71e8

Adding str for path

de726c6

Adding correct args

2fd83e8

erman-gurses reviewed Jun 2, 2025

View reviewed changes

ScottTodd reviewed Jun 2, 2025

View reviewed changes

Reorganizing

97c4ed0

geomin12 requested review from ScottTodd and erman-gurses June 3, 2025 17:05

ScottTodd reviewed Jun 3, 2025

View reviewed changes

PR comments

c0ac9cf

geomin12 requested a review from ScottTodd June 3, 2025 21:43

Removing ref

74b3bc5

ScottTodd approved these changes Jun 3, 2025

View reviewed changes

geomin12 merged commit 1b9fe8a into main Jun 4, 2025
23 checks passed

geomin12 deleted the users/geomin12/s3-organization branch June 4, 2025 00:41

github-project-automation Bot moved this from TODO to Done in TheRock Triage Jun 4, 2025

geomin12 mentioned this pull request Jun 4, 2025

Updating TheRock sha for s3 upload ROCm/rocm-libraries#136

Closed

	CI systems can set `TEATIME_LABEL_GH_GROUP=1` in the environment, which will
	cause labeled console output to be printed using GitHub Actions group markers
	instead of line prefixes. This causes the output to show in the log viewer
	with interactive group handles.

Conversation

geomin12 commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erman-gurses Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erman-gurses commented Jun 2, 2025

Uh oh!

erman-gurses left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ScottTodd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geomin12 commented Jun 3, 2025

Uh oh!

ScottTodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geomin12 commented May 28, 2025 •

edited

Loading

erman-gurses Jun 2, 2025 •

edited

Loading