-
Notifications
You must be signed in to change notification settings - Fork 295
feat(ci): add dynamic stage determination for external repos #7130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
edf376c
34ccc5a
1217e49
a398618
9d474a2
071fb1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,6 +124,7 @@ on: | |
|
|
||
| permissions: | ||
| contents: read | ||
| actions: read | ||
|
|
||
| jobs: | ||
| setup: | ||
|
|
@@ -177,14 +178,17 @@ jobs: | |
| PREBUILT_STAGES: ${{ inputs.prebuilt_stages }} | ||
| BASELINE_RUN_ID: ${{ inputs.baseline_run_id }} | ||
| STAGE_REUSE_MODE: ${{ inputs.stage_reuse_mode }} | ||
| # The checkout commit is used for commit-compatibility checking. | ||
| STAGE_REUSE_CURRENT_SHA: ${{ steps.checkout.outputs.commit }} | ||
| STAGE_REUSE_MAX_AGE_HOURS: ${{ inputs.stage_reuse_max_age_hours }} | ||
| STAGE_REUSE_COMMIT_HISTORY: ${{ inputs.stage_reuse_commit_history }} | ||
| # For cross-repo artifact reuse, use repository input (defaults to GITHUB_REPOSITORY). | ||
| # External repos set repository to "ROCm/TheRock" to reuse TheRock's prebuilt artifacts. | ||
| THEROCK_REPOSITORY: ${{ inputs.repository }} | ||
| # Skip path filtering for external repos (git sha won't exist in TheRock) | ||
| SKIP_PATH_FILTERS: ${{ inputs.external_repo != '' && 'true' || '' }} | ||
| # For baseline run selection, use repository input (defaults to GITHUB_REPOSITORY). | ||
| # External repos set repository to "ROCm/TheRock" to fetch TheRock baseline runs. | ||
| THEROCK_REPOSITORY: ${{ inputs.repository || github.repository }} | ||
| # Token for GitHub API calls (branch history, baseline workflow runs) | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding GITHUB_TOKEN. Do we also need actions: read in the workflow permissions? |
||
| # External repo JSON (e.g., {"repository":"ROCm/rocm-libraries","ref":"..."}) | ||
| EXTERNAL_REPO: ${{ inputs.external_repo }} | ||
| CI_CONFIG_PATH: ci-config | ||
| # External repo family overrides allow callers to add/modify GPU family configs | ||
| # (e.g., adding test runners for architectures not enabled in TheRock's default config) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -161,6 +161,10 @@ class CIInputs: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Repository to query for baseline runs (for cross-repo artifact reuse) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_repository: str = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # External repo JSON (e.g., '{"repository":"ROCm/rocm-libraries","ref":"..."}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Non-empty when an external repo calls TheRock workflows | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| external_repo: str = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def log(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Log parsed inputs for CI diagnostics.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print("CIInputs:") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -260,6 +264,7 @@ def from_environ() -> "CIInputs": | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prebuilt_stages=os.environ.get("PREBUILT_STAGES", ""), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_run_id=os.environ.get("BASELINE_RUN_ID", ""), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_repository=os.environ.get("THEROCK_REPOSITORY", ""), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| external_repo=os.environ.get("EXTERNAL_REPO", ""), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -309,6 +314,22 @@ def empty() -> "GitContext": | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return GitContext() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def from_external_repo(external_repo_name: str) -> "GitContext": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Create context for external repo builds (e.g., rocm-libraries). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| For external repos, we treat the repo name as both a changed file and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| a submodule path so that: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. Stage reuse analysis can determine which TheRock stages are affected | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 2. has_submodule_changes returns True, enabling submodule_bump_tests_only | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| families to run their tests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+324
to
+325
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need to be part of this PR, but let's consider removing or reworking The behavior that
Here are a few proposals that I think would simplify:
We should also consider moving gfx950 from postsubmit/nightly to presubmit (at least in rocm-systems, later rocm-libraries and TheRock) given its increasing importance. If we don't have enough runner capacity for tests, we could also split out "build targets" from "test targets" to give a more directly configurable way to say "this PR shouldn't just build gfx950, it should also run gfx950 tests". I don't want to hide "run tests" behind requirements like "only if the PR touches these files". Here's the code:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah good point! seems we just need to advance our labelling system and make it known. As I was making updates for gfx90a, it was pretty messy with all the "conditionals" we are doing for postsubmit due to limited capacity. 100% agree! I'll do this in a follow up PR as this will also impact gfx90a plus as this becomes the norm, we can educate ROCm devs about this and utilize (if they know it impacts gfx950, enable it!) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"External repo detected: {external_repo_name}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return GitContext( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| changed_files=[external_repo_name], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| submodule_paths=[external_repo_name], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @property | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def has_submodule_changes(self) -> bool | None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Check if any submodules were modified in the changed files. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -571,6 +592,9 @@ def should_skip_ci( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'ci:skip' PR label | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - Only skippable files changed (docs, .md, etc.) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - No files changed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| For external repo builds, path filtering is skipped since the external repo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name is used for stage reuse analysis, not for CI skip decisions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "ci:skip" in ci_inputs.pr_labels: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(" Skipping: 'ci:skip' PR label") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -594,6 +618,12 @@ def should_skip_ci( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "ci:asan" in ci_inputs.pr_labels and ci_inputs.build_variant == "asan": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(" Running: 'ci:asan' PR label triggers ASAN CI") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # External repo builds skip path filtering - they always run CI and use | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # stage reuse to determine which stages to rebuild. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ci_inputs.external_repo: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(" External repo build: skipping path filter checks, using stage reuse") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+621
to
+627
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem flexible enough, at least on the surface. If a PR to rocm-libraries or rocm-systems only modifies non-code files like Can we reuse the skip path filters that currently exist in those repositories like https://github.com/ROCm/rocm-libraries/blob/7df08c3d5d7ca4087a21810122f19d65fc102975/.github/scripts/therock_configure_ci.py#L28-L70 ? We could start with this as you have it, but we get frequent requests for ways to skip CI where it isn't applicable, so I don't want to regress there. For example,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes! do not worry, I am aware of this. I'm trying to make incremental PRs all in parallel to handle scoped items so it isn't massive! this is at least letting CI know that rules that apply to TheRock do not apply to external repo (such as CMakeLists.txt edit, don't run everything!)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could add a TODO here or link to an issue with the plan so we don't lose track and if someone later finds the code they know what the ideal/planned state is. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If we have a list of changed files (push/pull_request events), check if | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # CI should run for that set of changed files. For example: if only .md | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # files are changed, skip CI. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -892,14 +922,14 @@ def decide_jobs( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_repository = ci_inputs.baseline_repository | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_run_id = ci_inputs.baseline_run_id | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Apply automatic stage reuse when running in the same repo as baseline. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current_repo = os.environ.get("GITHUB_REPOSITORY", "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not baseline_repository or baseline_repository == current_repo: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # reuse-stage mode returns non-empty applied_reuse_stages. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for stage in auto_stage_reuse.applied_reuse_stages: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stage_decisions.setdefault(stage, JobAction.PREBUILT) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if auto_stage_reuse.applied_reuse_stages and auto_stage_reuse.baseline_run_id: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_run_id = auto_stage_reuse.baseline_run_id | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Apply automatic stage reuse. For external repos (rocm-libraries, rocm-systems), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # we reuse stages from TheRock baselines. For same-repo runs, baseline_repository | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # is empty or matches the current repo. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # reuse-stage mode returns non-empty applied_reuse_stages. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for stage in auto_stage_reuse.applied_reuse_stages: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stage_decisions.setdefault(stage, JobAction.PREBUILT) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if auto_stage_reuse.applied_reuse_stages and auto_stage_reuse.baseline_run_id: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_run_id = auto_stage_reuse.baseline_run_id | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| build_rocm = BuildRocmDecision( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| action=JobAction.RUN, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1393,15 +1423,24 @@ def configure(ci_inputs: CIInputs, git_context: GitContext) -> CIOutputs: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def main(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ci_inputs = CIInputs.from_environ() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Skip path filtering for external repos (e.g., rocm-libraries calling TheRock workflows) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The "run everything" is initial state for superrepo multi-arch CI migration. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # We will eventually support path filtering and component selection. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # TODO: Provide custom decision logic to run specific components and paths | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| skip_path_filters = os.environ.get("SKIP_PATH_FILTERS", "").lower() == "true" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Check if this is an external repo build (e.g., rocm-libraries calling TheRock workflows) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ci_inputs.external_repo: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # External repo: use repo name for stage reuse analysis. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # external_repo is JSON like {"repository":"ROCm/rocm-libraries","ref":"..."} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| external_repo = json.loads(ci_inputs.external_repo) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except (json.JSONDecodeError, TypeError) as exc: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"EXTERNAL_REPO contains invalid JSON: {ci_inputs.external_repo!r}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) from exc | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if skip_path_filters: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # External repo: skip path filtering, run everything | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| git_context = GitContext.empty() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo_full_name = external_repo.get("repository", "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not repo_full_name: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"EXTERNAL_REPO missing 'repository' field: {ci_inputs.external_repo!r}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| external_repo_name = repo_full_name.split("/")[-1] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| git_context = GitContext.from_external_repo(external_repo_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif (ci_inputs.is_pull_request or ci_inputs.is_push) and ci_inputs.base_ref: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # 'pull_request' and 'push' events can use the list of changed files | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # compared to the "prior commit" to affect job selections/options. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -474,7 +474,11 @@ def _default_baseline_selector(*, platform: str) -> BaselineSelector: | |
| extra "passing build" check is needed here. | ||
| """ | ||
|
|
||
| github_repository = os.environ.get("GITHUB_REPOSITORY", "ROCm/TheRock") | ||
| # THEROCK_REPOSITORY is set by setup_multi_arch.yml to the repository input | ||
| # (ROCm/TheRock for external repos, or github.repository for normal runs). | ||
| github_repository = os.environ.get( | ||
| "THEROCK_REPOSITORY", os.environ.get("GITHUB_REPOSITORY", "ROCm/TheRock") | ||
| ) | ||
|
Comment on lines
+477
to
+481
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This correctly redirects external-repo baseline queries to TheRock. However, with this new external-repo path, could we fail closed if the branch-history query below fails or returns no commits while STAGE_REUSE_CURRENT_SHA is set? |
||
| branch = os.environ.get("STAGE_REUSE_BASELINE_BRANCH", "main") | ||
| workflow_name = os.environ.get("STAGE_REUSE_BASELINE_WORKFLOW", "multi_arch_ci.yml") | ||
| current_commit_sha = os.environ.get("STAGE_REUSE_CURRENT_SHA") or None | ||
|
|
@@ -489,10 +493,16 @@ def _default_baseline_selector(*, platform: str) -> BaselineSelector: | |
| # establish ancestry. select_baseline_run only accepts a candidate whose | ||
| # head_sha is `same` or `ancestor` of current_commit_sha; with an EMPTY | ||
| # window every candidate resolves to `unknown` and is rejected, so reuse | ||
| # never activates. Fetch the real history here. If the SHA is set but the | ||
| # history fetch fails (or returns empty), disable the commit rule (pass both | ||
| # as None) rather than enabling it with an empty window -- recency and | ||
| # artifact availability still gate the selection. | ||
| # never activates. Fetch the real history here. | ||
| # | ||
| # For external repos (THEROCK_REPOSITORY != GITHUB_REPOSITORY), we must be | ||
| # strict: if commit history cannot be fetched, fail closed by returning a | ||
| # selector that always returns None (no baseline). This ensures we don't | ||
| # select incompatible baselines when building against a pinned TheRock commit. | ||
| # | ||
| # For same-repo runs, we can be lenient: disable the commit rule and let | ||
| # recency/artifact availability gate the selection. | ||
| is_external_repo = github_repository != os.environ.get("GITHUB_REPOSITORY", "") | ||
| ordered_commit_shas = None | ||
| effective_commit_sha = current_commit_sha | ||
| if current_commit_sha is not None: | ||
|
|
@@ -503,6 +513,14 @@ def _default_baseline_selector(*, platform: str) -> BaselineSelector: | |
| max_count=history_count, | ||
| ) | ||
| except GitHubAPIError as exc: | ||
| if is_external_repo: | ||
| logger.warning( | ||
| "%s could not fetch branch history for external repo (%s); " | ||
| "failing closed - no baseline will be selected.", | ||
| LOG_PREFIX, | ||
| exc, | ||
| ) | ||
| return lambda required_artifacts: None | ||
| logger.warning( | ||
| "%s could not fetch branch history (%s); " | ||
| "skipping commit-compatibility rule.", | ||
|
|
@@ -511,6 +529,13 @@ def _default_baseline_selector(*, platform: str) -> BaselineSelector: | |
| ) | ||
| ordered_commit_shas = None | ||
| if not ordered_commit_shas: | ||
| if is_external_repo: | ||
| logger.warning( | ||
| "%s empty branch history for external repo; " | ||
| "failing closed - no baseline will be selected.", | ||
| LOG_PREFIX, | ||
| ) | ||
|
Comment on lines
+532
to
+537
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add LOG_PREFIX here, as in the warning above? |
||
| return lambda required_artifacts: None | ||
|
Comment on lines
516
to
+538
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non blocker: we could add unit tests for both external-repo fail-closed cases.
|
||
| effective_commit_sha = None | ||
| ordered_commit_shas = None | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,6 +225,48 @@ def test_push_created_ref_disables_path_filtering(self): | |
| ) | ||
| self.assertIsNone(inputs.base_ref) | ||
|
|
||
| def test_external_repo_reads_from_env(self): | ||
| """External repo JSON is read from EXTERNAL_REPO env var.""" | ||
| inputs = _run_from_environ( | ||
| event_name="workflow_dispatch", | ||
| event_payload={}, | ||
| extra_env={ | ||
| "EXTERNAL_REPO": '{"repository":"ROCm/rocm-libraries","ref":"abc123"}', | ||
| }, | ||
| ) | ||
| self.assertEqual( | ||
| inputs.external_repo, '{"repository":"ROCm/rocm-libraries","ref":"abc123"}' | ||
| ) | ||
|
|
||
| def test_external_repo_defaults_to_empty(self): | ||
| """External repo defaults to empty string when not set.""" | ||
| inputs = _run_from_environ( | ||
| event_name="workflow_dispatch", | ||
| event_payload={}, | ||
| ) | ||
| self.assertEqual(inputs.external_repo, "") | ||
|
|
||
|
|
||
| class TestGitContext(unittest.TestCase): | ||
| """Test GitContext methods.""" | ||
|
|
||
| def test_from_external_repo_creates_context_with_repo_name(self): | ||
| """from_external_repo creates context with repo name as changed file.""" | ||
| git = cm.GitContext.from_external_repo("rocm-libraries") | ||
| self.assertEqual(git.changed_files, ["rocm-libraries"]) | ||
| self.assertEqual(git.submodule_paths, ["rocm-libraries"]) | ||
|
|
||
| def test_from_external_repo_has_submodule_changes(self): | ||
| """from_external_repo sets has_submodule_changes to True.""" | ||
| git = cm.GitContext.from_external_repo("rocm-libraries") | ||
| self.assertTrue(git.has_submodule_changes) | ||
|
|
||
| def test_from_external_repo_empty_name(self): | ||
| """from_external_repo handles empty name.""" | ||
| git = cm.GitContext.from_external_repo("") | ||
| self.assertEqual(git.changed_files, [""]) | ||
| self.assertEqual(git.submodule_paths, [""]) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Step 2: Check Skip CI | ||
|
|
@@ -330,6 +372,17 @@ def test_release_pr_without_submodule_change_runs(self): | |
| ) | ||
| self.assertFalse(cm.should_skip_ci(inputs, git)) | ||
|
|
||
| @patch("configure_multi_arch_ci.is_ci_run_required") | ||
| def test_external_repo_skips_path_filter(self, mock_filter): | ||
| """External repo builds skip path filtering and always run CI.""" | ||
| inputs = self._inputs( | ||
| external_repo='{"repository":"ROCm/rocm-libraries","ref":"abc123"}' | ||
| ) | ||
| git = cm.GitContext(changed_files=["rocm-libraries"]) | ||
| self.assertFalse(cm.should_skip_ci(inputs, git)) | ||
| # Path filter should not be called for external repos | ||
| mock_filter.assert_not_called() | ||
|
Comment on lines
+376
to
+384
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a test covering the full external-repo stage-reuse path? The tests verify that EXTERNAL_REPO is parsed and that path filtering is skipped, but they do not check that an external repository name is passed into stage-impact analysis and that unaffected stages are applied as PREBUILT from a TheRock baseline. |
||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Step 3: Decide Jobs | ||
|
|
@@ -608,6 +661,57 @@ def test_asan_tests_only_run_on_nightly_triggers(self): | |
| ) | ||
| self.assertEqual(result.test_rocm.action, cm.JobAction.RUN) | ||
|
|
||
| @patch("configure_multi_arch_ci.compute_auto_stage_reuse") | ||
| def test_external_repo_stage_reuse_uses_repo_as_changed_file(self, mock_reuse): | ||
| """External repo builds pass repo name to stage-impact analysis. | ||
|
|
||
| When an external repo (e.g., rocm-libraries) triggers a build, the repo | ||
| name should be treated as a changed file for stage-impact analysis. | ||
| This is a plumbing test that verifies the correct arguments are passed | ||
| to compute_auto_stage_reuse. | ||
| """ | ||
| # Setup mock to return a valid AutoStageReuse result | ||
| mock_reuse.return_value = cm.AutoStageReuse( | ||
| mode=cm.StageReuseMode.DRY_RUN, | ||
| candidate_stages=(), | ||
| rebuild_stages=(), | ||
| full_rebuild_required=False, | ||
| baseline_run_id="12345", | ||
| baseline_html_url=None, | ||
| available_stages=(), | ||
| unavailable_stages=(), | ||
| applied_reuse_stages=("compiler-rt",), | ||
| reasons=(), | ||
| ) | ||
|
|
||
| # Create git context as if from external repo | ||
| git = cm.GitContext.from_external_repo("rocm-libraries") | ||
|
|
||
| # Verify GitContext is set up correctly | ||
| self.assertEqual(git.changed_files, ["rocm-libraries"]) | ||
| self.assertEqual(git.submodule_paths, ["rocm-libraries"]) | ||
| self.assertTrue(git.has_submodule_changes) | ||
|
|
||
| # Call decide_jobs with external repo context | ||
| result = cm.decide_jobs( | ||
| self._inputs( | ||
| external_repo='{"repository":"ROCm/rocm-libraries","ref":"abc123"}' | ||
| ), | ||
| git_context=git, | ||
| targets=cm.TargetSelection(), | ||
| ) | ||
|
|
||
| # Verify compute_auto_stage_reuse was called with correct arguments | ||
| mock_reuse.assert_called_once() | ||
| call_kwargs = mock_reuse.call_args.kwargs | ||
| # The changed_files should be passed through for stage-impact analysis | ||
| self.assertEqual(call_kwargs["changed_files"], ["rocm-libraries"]) | ||
|
|
||
| # Verify the result contains the mocked reuse data | ||
| self.assertIsNotNone(result.auto_stage_reuse) | ||
| self.assertEqual(result.auto_stage_reuse.applied_reuse_stages, ("compiler-rt",)) | ||
| self.assertEqual(result.auto_stage_reuse.baseline_run_id, "12345") | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Step 4: Select Targets | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions adding GITHUB_TOKEN for the baseline API calls, but I don't see it passed into this step's environment.
Should this include:
GITHUB_TOKEN: ${{ github.token }}The stage-reuse path now queries branch history and baseline workflow runs, so explicitly passing the workflow token would ensure those requests are authenticated.