Skip to content

Optional S3 artifacts features #3663

Closed
jayhawk-commits wants to merge 6 commits into
mainfrom
users/jayhawk-commits/s3-artifacts-support
Closed

Optional S3 artifacts features #3663
jayhawk-commits wants to merge 6 commits into
mainfrom
users/jayhawk-commits/s3-artifacts-support

Conversation

@jayhawk-commits
Copy link
Copy Markdown
Contributor

@jayhawk-commits jayhawk-commits commented Feb 27, 2026

Summary

Adds optional upload behavior to post_build_upload.py to support private (GitHub EMU) and other non-standard workflows. All new flags are optional; existing default behavior is unchanged.

These features integrate with the WorkflowOutputRoot and StorageLocation abstractions introduced in PR #3596 rather than hardcoding bucket-specific paths.

Changes

New CLI flags in post_build_upload.py

  • --artifacts-bucket: Override the default artifacts bucket (e.g. for private workflows using a different bucket). When set, bypasses the automatic bucket selection in WorkflowOutputRoot.from_workflow_run().
  • --skip-manifest: Skip therock_manifest.json upload and omit the manifest link from the job summary. Useful for builds that do not produce a manifest.
  • --path-prefix: Storage path prefix prepended to all output paths for this run (e.g. v3/artifacts). When set, all outputs are placed under {prefix}/{run_id}-{platform}/ instead of {run_id}-{platform}/.
  • --summary-base-url / --summary-path: Use a custom base URL (e.g. CloudFront CDN) for job summary links instead of the default public HTTPS URL. --summary-path sets an optional path prefix on the CDN URL. Avoids exposing raw bucket URLs in job summaries.

StorageLocation.cdn_url() (new method in storage_location.py)

Backend-agnostic URL construction for CDN or custom base URLs. Strips a storage path prefix (to map from storage-relative paths to CDN-relative paths) and prepends an optional CDN path prefix.

WorkflowOutputRoot updates (workflow_outputs.py)

  • New path_prefix field: when set, all output paths for the run include the prefix.
  • from_workflow_run() gains a bucket_override parameter to support --artifacts-bucket.

Tests

  • build_tools/tests/workflow_outputs_test.py: 3 new test classes covering cdn_url(), path_prefix field behavior, and bucket_override / path_prefix in from_workflow_run().
  • build_tools/github_actions/tests/post_build_upload_test.py: 2 new test classes covering --skip-manifest behavior and CDN URL substitution in write_gha_build_summary().

…loudFront summary URLs

- Optional artifacts bucket (S3_BUCKET_ARTIFACTS) for private workflows that should not hardcode bucket names in the script.

- Optional --skip-manifest to skip the manifest upload step (default: do not skip).

- Optional --s3-subdir-artifacts, --s3-summary-base-url, --s3-summary-path so job summary links can use CloudFront URLs instead of raw S3, with a path layout that matches prerelease CloudFront for tarballs and wheels. Keeps summary URLs behind secured CloudFront and avoids exposing raw S3 URLs.

Made-with: Cursor
@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

I'll add pytest unit tests for these new features tomorrow. Wanted to get the work saved on the server that is ready for review.

Copy link
Copy Markdown
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

This might interfere with @ScottTodd‘s current work in #3596.

@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

This might interfere with @ScottTodd‘s current work in #3596.

Thanks for the heads up. I can wait for that to land first. Either way, the workflows on EMU will need changes as the patches it presently have would no longer apply cleanly.

Copy link
Copy Markdown
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Yeah I think we can more cleanly support this once #3596 goes in.

Comment on lines +13 to +23
S3 path/URL arguments (--s3-subdir-artifacts, --s3-summary-base-url, --s3-summary-path):
Must not end with a forward slash. The script joins segments with "/".
--s3-subdir-artifacts is the path prefix in the S3 bucket; --s3-summary-path is
the corresponding path as it appears in the CloudFront (summary) URL. This
optional path aligns with the prerelease CloudFront layout used for tarballs
and wheels.

Example (CloudFront base URL for job summary links):
--s3-subdir-artifacts v3/artifacts
--s3-summary-base-url https://rocm.foobar.amd.com
--s3-summary-path artifacts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should be mentioning CloudFront at all in most of our code. It's a CDN, an implementation detail. What matters is the URL, which could be backed by any CDN or other webserver and this code would not change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed mentions of CloudFront to support any CDN.

Comment on lines +541 to +543
Environment (optional override):
S3_BUCKET_ARTIFACTS: When set, use this bucket for the artifacts bucket
instead of the default chosen from repository/fork/release_type.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An override seems useful, but we need to be careful here. Environment variables are global, affecting all processes. Some of our workflows need the ability to read artifacts from one S3 bucket and write to another S3 bucket. We can do things like only set the environment variable for certain workflow steps, but at that point I think directly passing some setting through to the top level scripts and then into this function (or where I'm moving it to in #3596) would be safer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced the usage of an env with an explicit --artifacts-bucket arg that gets passed directly into WorkflowOutputRoot.from_workflow_run() as a bucket_override parameter.

Comment on lines +406 to +411
parser.add_argument(
"--s3-summary-path",
type=str,
default="",
help="Path prefix for summary URL when --s3-summary-base-url is set (e.g. artifacts). Must not end with /.",
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Either strip trailing / characters or assert with an argument parsing error. Comments are not error handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Trailing / characters are now rstrip'd.

@amd-shiraz
Copy link
Copy Markdown
Contributor

amd-shiraz commented Feb 27, 2026

@jayhawk-commits started looking at this. some very initial comments off the bat:

  1. --skip-manifest should also suppress the job summary manifest link
  2. in the log message, consider including external repo/fork run info if applicable
    Overall, Concept/design is good and useful for private workflows. on the backend side, not sure what bucket we are using for this usecase but make sure the s3 bucket is private with versioning and encryption on, cloudfront is the only reader of the bucket, internal users access via vpn, external users via signed urls and so on.

jayhawk-commits pushed a commit that referenced this pull request Mar 11, 2026
## Motivation

Bump rocm-systems from 93bc019 to 093b66c (includes fix for hip-tests
issue and revert for mathlib hiprtc issues and revert for rccl-test,
added revert for miopen failures due to PR 653):

Commits:
093b66c (HEAD, origin/develop, origin/HEAD) Revert "SWDEV-546177 -
hipModuleGetLoadingMode API impl (#653)" (#3858)
d8a0adb [AMD-SMI] Hide libamd_smi.so internal symbols (#3777)
d4da458 [rocprofiler-sdk] [Documentation ] Updating changelog (#3827)
19fadeb (origin/users/abchoudh/fix_dispatch_count) [RCCL][Tuner
Plugin] Enable tuning of RCCL tuning constants (#3757)
b4f5f8a rocr: Fix IPC dmabuf hang with large allocations (#3211)
64efea0 RCCL: allow users to override max and per job memory & fix
defaults. (#3797)
9b3dd10 Removing ready_for_review (#3849)
7e43880 [rocprofiler-systems] Update ROCm version to 7.2.0 in CI
workflows for Debian, RedHat, and Ubuntu (#3431)
1fdb6b9 [rocshmem] add gda/topology unit tests (#3715)
be1ea24 Move hipMipmappedArrayGetMemoryRequirements test to common
tests
e4513f0 Update amdgpu-windows-interop with latest changes, pal
58aa0bab2ced0cc9ebe8d2d0932db6774feb4e49 2026-03-04(#3773)
b1f964d [rocprofiler-compute] Ensure long kernel name fully shows in
compute analyze (#3665)
4dcf1e3 SWDEV-567112 - Replace test names (#3787)
33f5f30 ROCM-2428 - fixes hipStreamBatchMemOp invalid operation
checks (#3099)
139f4bf [SWDEV-556456] Align HIP_UUID with rocminfo (#3614)
8e89285 Reduce buffers alignment to 4 bytes (#3821)
51be29a AIRUNTIME-125: Consolidate Windows optimization and debug
flags (#3825)
1407392 [AMD-SMI] CI: Fix root workflow to use ASIC-specific test
filters (#3807)
63f78a9 (origin/users/mcao/fix_rocpdsummary) [ROCM-SMI] Fix DRM
include dirs leaking absolute build paths to consumers (#3808)
caf2f7e [ROCM-186] amd-smi: Add support for a VRAM and GTT tuning
interface (#3636)
a0712d4 [TheRock CI] Update projects_to_test lists (#3749)
02090c4 rocrtst: install gfx .hsaco files to share/rocrtst (#3744)
4a0a1cb Merge other simd table (#3696)
0d07657 Add missing kwargs from
rocprofiler_add_integration_validate_test in .cmake-format.yaml (#2336)
3a3df30 Optimize device counting service GPU interactions (#1583)
95d9da0 Add SPM Enable flag in build infrastructure (#3677)
12bb943 [rocprofiler-sdk] On-demand GPU profile queue
creation/destruction (#3586)
941057c  Navi4 tuning table iter 1 (#3052)
dbf2b73 [AMD-SMI] Display N/A for cu_occupancy when file is
unavailable (#3589)
b0efc7c [RCCL] [UT] Add ROCTX test (#3625)
ba7a20e Reducing the p2pnChannels for half-subscription A2A on
multi-node MI350 (#3381)
75238c9 [clr] Fix memory leak in getOrCreateHostcallBuffer (#3699)
af2ee0e [hip-tests] ASAN Check for image support before we create
context (#3834)
ad44966 Update windows ci subtree in include amdgpu-windows-interop
(#3814)
c8ad252 [rocprofiler-register] Fix compilation with system fmt/glog
(#1243)
7818815 Update README to include dbgapi and debug agent components
(#3731)
88e4a78 ROCProfiler and ROCTracer: Modifying deprecation note (#3831)
b5918a5 [ROCM-3124-3125-3126] CUID file generation hangs on MI350
systems/CUID test failures/Segmentation fault in CUID example code
(#3548)
97a5dd9 Update copyright to use SPDX IDs (#3805)
511730a [rocshmem]: add flood-amo tester (#3653)
2d650a0 [clr] Fix heap use after free error in device allocations
(#3789)
b6b179a Disable hipHostRegister_Negative test for ASAN (#3832)
39ec318 [RCCL] Add GDA alltoallv via rocshmem integration (#3613)
fb0f4d5 [RCCL] [CUMEM] Fix cuMem multi-process runs (#3811)
c3de7d4 SWDEV-526201 - Fix and enable disabled HIP tests from warp
group (#3089)
8d9a8ca roofline: code cleanup and refactor vector types (#3813)
8957e49 Don't wait on command completion if worker thread is
destroyed (#3790)
9e7586a [rocshmem] Add barrier APIs and expose `ROCSHMEM_TEAM_WORLD`
on device (#3651)
91b0923 Revert "fix local gpu release static build failure (#3667)"
(#3799)
0fda754 libhsakmt: Add secondary KFD context creation support
ee43db9 Revert "Update TheRock reference to 20260303 commit (#3709)"
(#3826)
86e28b9 Added fix to update GL2C counters instance count for GFX11.5
(#3100)
93f69f7 Adjust includes to match use (#3742)
e9fbc3f (develop) Update TheRock reference to 20260303 commit (#3709)
be0675a (HEAD) Revert "Support fp8 types in hiprtc (#2605)" (#3792)
3e3a94a [rocprofiler-systems] Add trace_cache support for
std::optional<T> serialization (#3490)
0b42a7f clr: Eliminate unnecessary kernel name string copies (#3774)
b6b0d77 rocr: Add hsa_amd_memory_async_batch_copy API for batched
memory copies (#3259)
486e6d1 Resolve staircase RS regression with 48 max channels (#3684)
eb59c85 [gfx942][gfx950] Leverage new cache bypass builtins for
simple protocol where available (#2847)
4d74d27 (origin/users/raramakr/rocm-smi-target) Revert "Auto Labeler:
Add ci:regression-detection label to rccl PRs (#3543)" (#3769)
8f07955 [AMD-SMI] CI: Use ASIC-specific test blacklists in workflows
(#3775)
7cef5b6 Fix MFMA total FLOPS calculation (#3371)
aea3751 Remove duplicated tests (#3235)
b6c656f Remove duplicated tests in memory module (#3087)
ca3137d [rocprofiler-sdk] Install integration tests without building
for therock & Misc. fixes (#3047)
0ab5c41 [rdc] Enable on-demand queue mode in rocprofiler-sdk to
prevent inference degradation (#3629)
a1eb2a1 rocr/wsl: a library should not output to std::out by default
(#3718)
b7da296 Reenable flood_put/get testers on mlx5 since they should work
after pr2732 (#3748)
000e24d [rocprofiler-sdk] Add automatic late-start support to
rocprofiler_force_configure (#2168)
64ea87f [hip-tests] Fix memory leaks in hipMemPoolTrimTo tests
(#3643)
543a7d7 rocr: Include code object allocs in lightweight coredump
a58da37 [rocdecode] - update rocdecode ctest (#3768)
f88e4ee [rocprofiler-systems] Make CDash submit non-fatal and add
GitHub Actions logging (#3525)
cb14deb [rocprofiler-systems] Update nlohmann-json submodule (#3391)
4492530 SWDEV-567112 - Introduce new mechanism for tagging and
disabling tests - Part 2 (#3707)
8ca9913 disabling rccl from full build (linux), covered in RCCL CI
(#3770)
c4fdb20 [ATT] Re-enable tests. Add option to specify perf to target
CU only (#2819)
615aab9 ROCM-3816 Out of Memory fix (#3588)
8ffad41 Fix rocm_smi64 exporting invalid absolute paths to consumers
(#3717)
042d76a rocr: Remove dependency on KFD in Runtime::VMemoryHandleMap
(#2515)
555db59 [AMD-SMI] CPU: Added support for family 1A Models 50h-57h
(#3206)
3affa2c [SWDEV-555935] Fix shared mutex and self-heal (#3729)
ba0bf0f Replace hipMemGetInfo with ihipMemGetInfo and use it for
internal calls. (#2845)
c5cef9b Fix HIP_RETURN on all HIP API calls. (#2838)
241ce7b Revert "memory: fix "contiguous_bytes" calculation in generic
conversion (#3285)" (#3755)
8a690f4 [kpack/clr] Windows PE/COFF support for kpack artifact
splitting and runtime loading (#3728)
863bdf8 MFMA pre-processor guards for ipc.hip (#3724)
90bb9b1 Release queue outside of vgpusAccess lock (#3705)
de45239 clr: Add build support of ROCR and PAL backends together
(#3722)
dfb7abc [rocprofiler-sdk] RCCL API changes for
RCCL_API_TRACE_VERSION_PATCH = 3 (#3477)
d69d4f2 [AICOMRCCL-633] - Fixed warnings in tests (#3402)
067d86d rocr/wsl: Disable AQL Queue usage with flag ROCR_USE_PM4
(#3663)
594eb60 [TheRock CI] rocm-systems build full ROCm stack (#3182)
27d17e8 [ROCProfiler-SDK] Fix SWDEV-556922: Handle comments before
checking for pmc: (#1723)
c80d904 memory: fix "contiguous_bytes" calculation in generic
conversion (#3285)
669987c [hip-tests] ASAN - add missing release handles (#3735)
a24bbd7 fix local gpu release static build failure (#3667)
259b2ff Speed up DeviceId (#2803)
65d9264 Simplify MPI trace merge logic and remove legacy guards
(#3562)
1076c08 use system to look for zcat path instead (#3720)
22f1d19 [AICOMRCCL-355] Enable threshold-based p2p-batching (#3000)
a2e4c79 Partially flatten template tests cases (#2597)
e242abe Pass space separated gfx target list to RCCL build command
(#3701)
4f78aea SWDEV-570074 - Refactor Memset memory object handling.
(#2228)
b3ad12d Support Nvidia build on theRock for HIP-tests (#3335)
a1cf15e Support fp8 types in hiprtc (#2605)
8ef84b0 [rocprofiler-systems] Add HPC examples to automated testing
(#3437)
db3a70d Free memory which was allocated in tests (#3710)
27e6809 [rocprofiler-systems]: Fix rhel CI failure on for MPI and UCX
tests (#3700)
0d9aaf5 rccl/topo_expl: fix build issue. (#3719)
be04d75 Fix zcat path used for checking kernel configs (#3423)
cab60a7 rocr/thunk/win: Add CU mask support (#3518)
5b3d826 [CUMEM] Initial support for cuMem APIs (#2763)
0606ff4 [HIP] [PLAT-194496] Improve Stress_hipMalloc_HighSizeAlloc
reliability (#3550)
05750a7 fix hip-test name in config (#3716)
33f777f hsakmt: Remove --high functionality from run_kfdtest.sh
(#2486)
e4c46e3 Hide the retain under direct dispatch check (#3698)
bfe0ca0 Add rocprof trace decoder to CI tests (#3690)
a769b6f [rocSHMEM] Edgar/abstract allocator ipc part1 (#3411)
659fb52 [AMD-SMI] Fix bugs, improve error handling, and clean up
NIC/switch code (#3654)
0eb26ea hsakmt: Fix Import/Export of dmabuf_fd for WSL/Windows
(#3348)
a122936 [SWDEV-567812] Add UBB power and power_limit fields to
npm_info (#3262)
c3bec09 [rocprofiler-sdk][rocprofv3][rocpd] Updates for KFD data
(#340)
7c44d47 SWDEV-547659 - Remove HIP_VERSION_GITHASH in logs (#448)
74b6487 SWDEV-547008 - Documentation fix for function return values
(#463)
af21cd4 SWDEV-545553 - Improve clarity and robustness of CALLBACK
unit tests (#546)
180d639 SWDEV-544900 - Change hip-test test case name (#547)
feeca99 Doc improvements (#3688)
c1822b6 ROCprofiler-SDK: deprecation of legacy tools (#3609)
5d7aff8 Fix rocprof-compute-viewer link (#3459)
0b0b484 AIRUNTIME-129 - Fix Ocl test failures of 2D image with
pitches. (#3584)
ac569b8 Fix memory tests config (#3687)
603fe7a [hip-tests] Enable hipMipmappedArrayGetMemoryRequirements
test via cmake
4fad445 [hip] Docs: Updates to some memory management pages
8cc5955 AICOMRCCL-656 fix memory leak in ncclCommInitRankFunc (#3628)
94a4595 Fix missing amd_comgr linkage in pc-sampling integration test
(#3453)
2a68565 rocrtst: CMAke file: strip xnack/feature suffixes from gfxNum
in build_kernel (#3652)
c3542bf [rocprofv3] Deprecating input text files for counter
collection (#1562)
ff122e7 SWDEV-573073 - Cleanup hipHostAlloc/Malloc/Register tests
(#3017)
5b1deaf SWDEV-567112 - Introduce new mechanism for tagging and
disabling tests - Part 1 - Core (#2351)
6e0cc30 rocrtst: MaxSingleAllocationTest: skip CPU NUMA nodes >0
(#3208)
d65f601 [AICOMRCCL-667] rccl: Change GDR selection logic. (#3607)
f1c44ab Patch Back to Old Repo: fixes from manual runs (#3621)
fe53bcd [AMD-SMI] Allow amdsmi init to succeed when no NIC hardware
is present (#3403)
b25600e [ROCM SMI] Fix fw pldm version not displayed in default
amd-smi (#3594)
169d2ef root to module wiring, remove legacy source collection
(#3482)
7469781 [LRT][clr] SWDEV-512963-Fix CTS test failures for 1D buffer
copy (#3520)
c8f55d9 Adding rocprof trace decoder (#3576)
425e983 Trace decoder codeowners (#3600)
a176efd [hip-tests] Add return statements to HIP_SKIP_TEST (#3647)
32687cf rocrtst: CPUAccessToGPUMemoryTest: Cap host allocation to 512
MB under ASAN (#3407)
97c0206 Update codeowners for thunk DXG (#3334)
be44b28 [rocdecode][rocjpeg] - ctest CMakeLists cleanup (#3632)
80ff0b8 Various memory leak fixes in hip-tests (#3605)
0988f67 fix typo in help text (#3314)
9f823c5 Fix CUID file lookup by loading files before searching
entries (#3436)
064c892 SWDEV-546177 - hipModuleGetLoadingMode API impl (#653)
006213e ROCM-2696: Ignare size and base if null ptr (#3336)
6060b99 Improve atomic min max test perf (#2580)
3fbcc13 Change printf capture impl (#1127)
93bc019 (tag: hip-version_7.12.60610,
origin/users/mradosav-amd/rocprofsys-selective-region) [ROCM-CORE]
Update rdhc script to support rocm install prefix
(ROCm/rocm-systems#3596)

[AICOMRCCL-355]:
https://amd-hub.atlassian.net/browse/AICOMRCCL-355?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Resolves conflicts in post_build_upload.py and github_actions_api.py
against the new WorkflowOutputRoot/StorageBackend abstractions from #3596.
Implements features against the new architecture:
- --artifacts-bucket: explicit bucket override (replaces S3_BUCKET_ARTIFACTS env var)
- --path-prefix / --summary-base-url / --summary-path: CDN-agnostic URL support
- --skip-manifest: skip upload and suppress summary link
- Adds bucket_override and path_prefix to WorkflowOutputRoot.from_workflow_run()
- Adds StorageLocation.cdn_url() for CDN link generation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

Updated to work with #3596 and apply some of the feedback provided. I wanted to get that part out of the way, but I will continue after I am back from travel.

The merge conflict resolution incorrectly introduced two blank lines
before str2bool(). This file should be unchanged from main.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jayhawk-commits jayhawk-commits added ci:skip Skip all CI builds/tests for this PR ci:skip labels Mar 19, 2026
@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

Adding skip-ci labels temporarily to not consume CI sources until I return from travel

Trailing slashes on --path-prefix, --summary-base-url, and --summary-path
are silently stripped, so callers don't need to worry about them.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ScottTodd ScottTodd removed the ci:skip label Mar 20, 2026
jayhawk-commits and others added 2 commits April 6, 2026 12:06
- StorageLocation.cdn_url(): remaps storage path prefix to CDN prefix
- WorkflowOutputRoot.path_prefix field: prepended to all output paths
- WorkflowOutputRoot.from_workflow_run(): add bucket_override/path_prefix args
- Tests: TestStorageLocationCdnUrl, TestWorkflowOutputRootPathPrefix,
  TestWorkflowOutputRootOverrides, TestWriteGhaBuildSummarySkipManifest,
  TestWriteGhaBuildSummaryCdnUrl

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jayhawk-commits jayhawk-commits removed the ci:skip Skip all CI builds/tests for this PR label Apr 6, 2026
@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

jayhawk-commits commented Apr 7, 2026

@jayhawk-commits started looking at this. some very initial comments off the bat:

1. --skip-manifest should also suppress the job summary manifest link

2. in the log message, consider including external repo/fork run info if applicable
   Overall, Concept/design is good and useful for private workflows. on the backend side, not sure  what bucket we are using for this usecase but make sure the s3 bucket is private with versioning and encryption on, cloudfront is the only reader of the bucket, internal users access via vpn, external users via signed urls and so on.
  1. write_gha_build_summary() now takes a skip_manifest parameter and skips both the upload and the manifest link in the job summary when set.
  2. The existing _retrieve_bucket_info() already logs external_repo and is_pr_from_fork as part of its output, which appears before our bucket_override log line. So the fork/repo context is always visible in the log when the override is active

Security concerns are accounted for when using the bucket overrides. One of the use cases involve buckets that we only want accessible through restricted CloudFront URLs.

@jayhawk-commits jayhawk-commits marked this pull request as ready for review April 7, 2026 02:21
Copy link
Copy Markdown
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

This PR has been open for a while I'm not sure about the approach here given how workflows have been changing as a result of switching to multi-arch CI. This also overlaps with other ongoing work.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This script is only used by "CI", not "Multi-arch CI", and thus is scheduled for deletion (#3340). New feature work should be in https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/post_stage_upload.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unless NPI pipelines are changed to work with multi-arch CI, such scripts that the older CI used cannot be deleted. A discussion on how multi-arch CI can intersect NPI pipelines is overdue.

Comment on lines +63 to +72
def cdn_url(
self,
base_url: str,
strip_prefix: str = "",
cdn_prefix: str = "",
) -> str:
"""CDN URL, optionally remapping the storage path prefix to a CDN prefix.

Strips ``strip_prefix`` from the start of ``relative_path`` (if present),
then prepends ``cdn_prefix``, and finally prepends ``base_url``.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ScottTodd added a commit that referenced this pull request Apr 7, 2026
## Motivation

Back in November of 2025, #2046
switched S3 buckets from:
* `therock-artifacts` -> `therock-ci-artifacts`
* `therock-artifacts-external` -> `therock-ci-artifacts-external`

We then added backwards compatible bucket selection logic in
#2357 so tools could continue to
load from these older buckets.

We're in the process of implementing retention policies for our S3
buckets and all files in the older buckets will soon be deleted, so I
think we can remove this code now. Simplifying the code will also help
with new feature work like multi-arch releases:
#3334.

## Technical Details

If we really want to support fetching from older buckets using tooling
(and not just manual downloading via AWS tooling, wget, the index pages,
etc.), we can add a way to explicit specify a bucket or directory like:
* #3663
* #4088
* #4167
* #4199


## Test Plan

* (Removed) Unit tests
* Artifact fetching now fails as expected for a run before the cutover
(5 months ago): https://github.com/ROCm/TheRock/actions/runs/19264731491
    ```
(.venv) λ python build_tools/fetch_artifacts.py --run-id 19264731491
--artifact-group gfx1151 --dry-run
    Retrieving bucket info...
      (implicit) github_repository: ROCm/TheRock
      workflow_run_id             : 19264731491
      head_github_repository      : ROCm/TheRock
      is_pr_from_fork             : False
    Retrieved bucket info:
      external_repo:
      bucket       : therock-ci-artifacts
Retrieving artifacts from
's3://therock-ci-artifacts/19264731491-windows'
    Matching artifact target families: ['generic', 'gfx1151']
Found no artifacts matching ['generic', 'gfx1151'] at
's3://therock-ci-artifacts/19264731491-windows'
    No matching artifacts for 19264731491 exist. Exiting...
    ```
* Artifact fetching still works for more recent runs

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
rahulc-gh pushed a commit that referenced this pull request Apr 9, 2026
## Motivation

Back in November of 2025, #2046
switched S3 buckets from:
* `therock-artifacts` -> `therock-ci-artifacts`
* `therock-artifacts-external` -> `therock-ci-artifacts-external`

We then added backwards compatible bucket selection logic in
#2357 so tools could continue to
load from these older buckets.

We're in the process of implementing retention policies for our S3
buckets and all files in the older buckets will soon be deleted, so I
think we can remove this code now. Simplifying the code will also help
with new feature work like multi-arch releases:
#3334.

## Technical Details

If we really want to support fetching from older buckets using tooling
(and not just manual downloading via AWS tooling, wget, the index pages,
etc.), we can add a way to explicit specify a bucket or directory like:
* #3663
* #4088
* #4167
* #4199


## Test Plan

* (Removed) Unit tests
* Artifact fetching now fails as expected for a run before the cutover
(5 months ago): https://github.com/ROCm/TheRock/actions/runs/19264731491
    ```
(.venv) λ python build_tools/fetch_artifacts.py --run-id 19264731491
--artifact-group gfx1151 --dry-run
    Retrieving bucket info...
      (implicit) github_repository: ROCm/TheRock
      workflow_run_id             : 19264731491
      head_github_repository      : ROCm/TheRock
      is_pr_from_fork             : False
    Retrieved bucket info:
      external_repo:
      bucket       : therock-ci-artifacts
Retrieving artifacts from
's3://therock-ci-artifacts/19264731491-windows'
    Matching artifact target families: ['generic', 'gfx1151']
Found no artifacts matching ['generic', 'gfx1151'] at
's3://therock-ci-artifacts/19264731491-windows'
    No matching artifacts for 19264731491 exist. Exiting...
    ```
* Artifact fetching still works for more recent runs

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

Outdated and no longer compatible with current upload scripts.

@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage May 11, 2026
@jayhawk-commits jayhawk-commits deleted the users/jayhawk-commits/s3-artifacts-support branch May 11, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants