Skip to content

[CI Deduplication] CI configuration for external repo integration#3188

Closed
jayhawk-commits wants to merge 17 commits into
mainfrom
users/jayhawk-commits/pr/ci-config-external
Closed

[CI Deduplication] CI configuration for external repo integration#3188
jayhawk-commits wants to merge 17 commits into
mainfrom
users/jayhawk-commits/pr/ci-config-external

Conversation

@jayhawk-commits
Copy link
Copy Markdown
Contributor

@jayhawk-commits jayhawk-commits commented Jan 31, 2026

Add CI configuration for external repository integration

Overview

This PR adds support for configuring CI workflows when external repositories (rocm-libraries, rocm-systems) call TheRock's reusable workflows.

Dependencies

This PR depends on PR #3181, which introduces the external repo detection module (detect_external_repo_config.py).

Architecture

graph TB
    subgraph layer1 [Layer 1 - Foundation]
        pathFilters[configure_ci_path_filters.py<br/>Path filtering only]
        detectConfig[detect_external_repo_config.py<br/>External repo config]
    end
    
    subgraph layer2 [Layer 2 - Orchestration]
        mainCI[configure_ci.py<br/>Main CI - internal and external]
    end
    
    mainCI -->|imports| pathFilters
    mainCI -->|imports| detectConfig
Loading
  • Layer 1: configure_ci_path_filters (path filtering; no orchestration). detect_external_repo_config (repo path, skip patterns, test list).
  • Layer 2: configure_ci.py – single entry point. Handles both TheRock CI and external-repo CI (EXTERNAL_REPO_NAME, project detection, path-based build decision, cross-product, empty matrix).

Testing

  • Existing tests in configure_ci_path_filters_test.py and test_configure_ci_external_repos.py; all updated to run against the new structure (external-repo tests import from configure_ci and patch configure_ci.get_git_modified_paths etc.).

Environment variables

  • EXTERNAL_REPO_NAME (optional): When set, config is built for that external repo (e.g. rocm-libraries).
  • PROJECTS (optional): Comma-separated project list or "all" (e.g. "projects/rocprim,projects/rocblas").

jayhawk-commits and others added 2 commits January 30, 2026 17:28
Introduce detect_external_repo_config.py module as foundational infrastructure for external repository integration. This standalone module will be used by future PRs to support rocm-libraries and rocm-systems integration.

Includes unit tests for module functionality.
@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

Need to refactor after some recent changes to configure CI by other developers.


Environment variables (for external repo runs; set by workflows that invoke this for rocm-libraries, etc.):
* EXTERNAL_REPO_NAME (optional): Repo name (e.g. "rocm-libraries"). When set, config is built for that external repo.
* PROJECTS (optional): Comma-separated project list or "all" (e.g. "projects/rocprim,projects/rocblas").
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this dependent on if it external repo or not? if yes, please add a comment

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.

I have added the requirement detail in the comment.

# --------------------------------------------------------------------------- #


def _should_skip_external_build_due_to_paths(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

both functions _should_skip_external_build_due_to_paths and should_skip_external_build_due_to_paths are not so long. i would put them in one?

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.

I have combined the two functions. Thanks for the suggestion.

return not should_build


def should_build_external_repo(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lets go away from all the "should" and just name it something like requires_external_repo_build

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.

I have updated the wording to requires.

paths = get_git_modified_paths(base_ref, cwd=cwd)
if paths is not None:
paths = list(paths)
print("modified_paths (max 200):", paths[:200] if paths else paths)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

??? the if

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.

Fixed the logic here for None paths.


def get_git_modified_paths(base_ref: str) -> Optional[Iterable[str]]:
def get_git_modified_paths(
base_ref: str, cwd: Optional[str] = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please better naming for cwd like external_repo_path or something like that

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.

I have renamed cwd.

self.assertEqual(len(result), 1)
self.assertEqual(result[0]["family"], "gfx94x")
self.assertEqual(result[0]["platform"], "linux")
self.assertEqual(result[0]["projects_to_test"], "rocprim,rocblas")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

above you expect a list in TestParseProjectsInput - but here we expect a comma separated string?

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.

I've changed the code, so that comma-separated strings are used consistently to match existing CI behaviour of accepting comma-separated strings from workflow_dispatch input.

self.assertEqual(result[0]["projects_to_test"], "rocprim")

def test_no_cmake_options_in_result(self):
"""Test that cmake_options are NOT included in result (full builds only)."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is the context here with full builds?

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.

One of the items on our backlog is having rocm-systems PRs do full ROCm build. Right now, rocm-systems PRs skips compilation of libraries. We've seen a few issues now where a rocm-systems change causes issues with a rocm-libraries project. My intention is to increase the scope of builds to full ROCm builds, and keep the test coverage the same as is.

self.assertIn("build_variant", r)
self.assertIn("test_labels", r)
self.assertIn("projects_to_test", r)
self.assertNotIn("cmake_options", r)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you are not evenl listing any cmake options here in the project_config?

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 the cmake_options assertion.

result = cross_product_projects_with_gpu_variants(project_configs, gpu_variants)

self.assertEqual(len(result), 4) # 2 projects * 2 variants
# Verify structure is preserved
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think you want to check here if the pairing was successful? you could even hardcode the expected results

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.

Hardcoded expected results.

get_all_families_for_trigger_types,
)
from detect_external_repo_config import (
get_external_repo_path,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i didnt manage to review your other pr. but i would like if some of those names could be a bit more descriptive? e.g get_skip_patterns could be better renamed get_skip_patterns_for_ci

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.

Thank you for the suggestion. I have renamed the functions from the module to be more descriptive.

@jayhawk-commits
Copy link
Copy Markdown
Contributor Author

Multi-arch CI is the path forward. Some of the logic here might be applied, but changes should be recreated as a new PR specific to multi-arch CI.

@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/pr/ci-config-external branch May 11, 2026 22:18
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.

2 participants