feat(ci): add dynamic stage determination for external repos #7130
There are no files selected for viewing
| 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,17 @@ 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 the changed file so that | ||
| stage reuse analysis can determine which TheRock stages are affected. | ||
| The external repo name maps to a submodule in TheRock. | ||
| """ | ||
| print(f"External repo detected: {external_repo_name}") | ||
| return GitContext(changed_files=[external_repo_name]) | ||
|
Contributor
There was a problem hiding this comment. Should this also populate The docstring says the external repository name maps to a TheRock submodule, but returning only That changes downstream test behavior: An external GitContext(
changed_files=[external_repo_name],
submodule_paths=[external_repo_name],
)Could we also add a test asserting that |
||
|
|
||
| @property | ||
| def has_submodule_changes(self) -> bool | None: | ||
| """Check if any submodules were modified in the changed files. | ||
|
|
@@ -571,6 +587,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 +613,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. 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. 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. 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 +917,17 @@ def decide_jobs( | |
| baseline_repository = ci_inputs.baseline_repository | ||
| baseline_run_id = ci_inputs.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 | ||
| # For external repos, use their baseline_repository (ROCm/TheRock) | ||
| if ci_inputs.baseline_repository: | ||
| baseline_repository = ci_inputs.baseline_repository | ||
|
Contributor
There was a problem hiding this comment. This is already done in line 917 |
||
|
|
||
| build_rocm = BuildRocmDecision( | ||
| action=JobAction.RUN, | ||
|
|
@@ -1393,15 +1421,23 @@ def configure(ci_inputs: CIInputs, git_context: GitContext) -> CIOutputs: | |
| def main(): | ||
| ci_inputs = CIInputs.from_environ() | ||
|
|
||
| # 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) | ||
| repo_full_name = external_repo.get("repository", "") | ||
| external_repo_name = ( | ||
| repo_full_name.split("/")[-1] if "/" in repo_full_name else "" | ||
| ) | ||
| except (json.JSONDecodeError, TypeError): | ||
| external_repo_name = "" | ||
|
Member
There was a problem hiding this comment.
|
||
|
|
||
| if external_repo_name: | ||
| git_context = GitContext.from_external_repo(external_repo_name) | ||
| else: | ||
| git_context = GitContext.empty() | ||
| 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. | ||
|
|
||
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.