From 968f482ca2e891627d8fe1af80625d5aa9795e2d Mon Sep 17 00:00:00 2001 From: Baggio Wong Date: Wed, 7 Oct 2026 18:20:28 +0000 Subject: [PATCH] feat: Create new minor templates on main before the first build Since 31c0e14, a new minor's template has been copied from the previous minor during that minor's first build and committed only to its release branch. Main never had it, so nobody editing templates on main could see that the new minor needed their change, and a later port that created the folder on main gave the two branches unrelated copies that conflicted. That is what broke 4.6.0 (#1343, #1361, #1363). Make the copy on main instead: - create-minor-template.yml: when X.Y.0 is published (or on manual dispatch), open a pull request to main that copies vX.Y to vX.(Y+1). The propagation check verifies it is an exact copy. - main.py: _ensure_minor_template_exists no longer copies; it fails and points to that pull request. X.0 still needs a manual template. - build-image.yml: commit only build_artifacts/ to release branches. - CONTRIBUTING.md: describe the new flow. - template_tools.py: add `create` and require_minor_template. --- .github/workflows/build-image.yml | 6 +- .github/workflows/create-minor-template.yml | 102 ++++++++++++++++++++ CONTRIBUTING.md | 51 +++------- src/main.py | 37 ++----- src/template_tools.py | 54 ++++++++++- test/test_template_tools.py | 23 +++++ 6 files changed, 200 insertions(+), 73 deletions(-) create mode 100644 .github/workflows/create-minor-template.yml diff --git a/.github/workflows/build-image.yml b/.github/workflows/build-image.yml index 644eadfd3..d8cd4655c 100644 --- a/.github/workflows/build-image.yml +++ b/.github/workflows/build-image.yml @@ -80,10 +80,8 @@ jobs: run: | git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" - # For minor/major releases, a new per-minor template dir (e.g. template/v4/v4.5/) - # is created during artifact generation. Stage ./template so it is persisted to the - # release branch; otherwise it is discarded when the runner is torn down. - git add ./build_artifacts ./template + # Only build_artifacts/ is generated. Templates come from main and are never created here. + git add ./build_artifacts git diff-index --quiet HEAD || git commit -m 'chore: Generate build artifacts for ${{ steps.calc_target.outputs.target_version }} release' git push --set-upstream origin release-${{ steps.calc_target.outputs.target_version }} - name: Create or fetch release PR diff --git a/.github/workflows/create-minor-template.yml b/.github/workflows/create-minor-template.yml new file mode 100644 index 000000000..d072feb32 --- /dev/null +++ b/.github/workflows/create-minor-template.yml @@ -0,0 +1,102 @@ +name: Create Next Minor Template +# When X.y.0 is released, the nightly starts building X.(y+1).0 (see add-nightly-builds.yml). That build +# reads template/vX/vX.(y+1)/ from main and will not create it, so this opens the pull request that does: +# a plain copy of template/vX/vX.y/. Merging it lets the X.(y+1).0 build start. +on: + release: + types: [published] + workflow_dispatch: + inputs: + version: + description: 'Minor version to create the template for (x.y), e.g. 4.7' + required: true + type: string +jobs: + open-template-pr: + name: Open template pull request + runs-on: ubuntu-latest + # Release tags look like 4.6.0-cpu and 4.6.0-gpu. Act once per X.y.0 release, on the cpu tag. + if: | + github.repository == 'aws/sagemaker-distribution' && + (github.event_name == 'workflow_dispatch' || endsWith(github.event.release.tag_name, '.0-cpu')) + permissions: + contents: write + pull-requests: write + steps: + - uses: actions/checkout@v6 + with: + ref: main + sparse-checkout: | + src + template + - name: Work out the version + id: target + env: + EVENT_NAME: ${{ github.event_name }} + RELEASE_TAG: ${{ github.event.release.tag_name }} + INPUT_VERSION: ${{ inputs.version }} + run: | + set -euo pipefail + if [ "$EVENT_NAME" = "release" ]; then + released="${RELEASE_TAG%-cpu}" # e.g. 4.6.0 + major="${released%%.*}" + rest="${released#*.}" + version="$major.$(( ${rest%%.*} + 1 ))" # e.g. 4.7 + else + version="$INPUT_VERSION" + fi + if ! [[ "$version" =~ ^[0-9]+\.[0-9]+$ ]]; then + echo "::error::Expected ., got '$version'." + exit 1 + fi + echo "version=$version" >> "$GITHUB_OUTPUT" + echo "major=${version%%.*}" >> "$GITHUB_OUTPUT" + echo "previous=${version%%.*}.$(( ${version#*.} - 1 ))" >> "$GITHUB_OUTPUT" + - name: Copy the previous minor's template + id: copy + env: + VERSION: ${{ steps.target.outputs.version }} + MAJOR: ${{ steps.target.outputs.major }} + run: | + set -euo pipefail + branch="template-v$VERSION" + if [ -d "template/v$MAJOR/v$VERSION" ]; then + echo "template/v$MAJOR/v$VERSION/ is already on main. Nothing to do." + elif git ls-remote --exit-code --heads origin "$branch" > /dev/null; then + echo "Branch $branch already exists. Nothing to do." + else + python3 src/template_tools.py create --version "$VERSION" + echo "branch=$branch" >> "$GITHUB_OUTPUT" + fi + - name: Open the pull request + if: steps.copy.outputs.branch != '' + env: + # Pull requests opened with the default GITHUB_TOKEN do not trigger pull_request workflows, so the + # required "Check template propagation" check would never run on this one. Use a token that does. + GH_TOKEN: ${{ secrets.GH_TOKEN }} + BRANCH: ${{ steps.copy.outputs.branch }} + VERSION: ${{ steps.target.outputs.version }} + MAJOR: ${{ steps.target.outputs.major }} + PREVIOUS: ${{ steps.target.outputs.previous }} + run: | + set -euo pipefail + git config --local user.email "github-actions[bot]@users.noreply.github.com" + git config --local user.name "github-actions[bot]" + git checkout -b "$BRANCH" + git add "template/v$MAJOR/v$VERSION" + git commit -m "template: Create v$VERSION template from v$PREVIOUS" + git push origin "$BRANCH" + cat > "$RUNNER_TEMP/body.md" < + + "Check template propagation" compares this copy with the latest \`template/v$MAJOR/v$PREVIOUS/\` when the pull request merges. + EOF + gh pr create --base main --head "$BRANCH" \ + --title "template: Create v$VERSION template from v$PREVIOUS" \ + --body-file "$RUNNER_TEMP/body.md" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4536588da..cb33dd831 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,63 +138,42 @@ template/ ### Deciding where to make your change -Where you put a template change determines how far forward it reaches. The key fact: a given minor's template feeds **all future patches of that same minor**, but a *new* minor's template is created by copying the previous minor's template only once (see "How new minor version templates are auto-created at build time" below). So **to reach future minor/major versions, your change must live in the newest minor template** — if the current newest minor is already released, that means creating the next minor's template and putting the change there. +A minor's template feeds every future patch of that minor. A new minor's template is a one-time copy of the previous minor's template, made on `main` before the new minor's first build (see "How new minor version templates are created" below). Nothing carries template changes forward after that, so where you make a change decides how far it reaches. -First, find the latest minor version that has a template directory (the highest `v4.N`): +The newest template for a major version (the highest `template/v4/v4.N/`) is always the next minor to be released. It is created on `main` as soon as the minor before it ships. -```shell -ls -d template/v4/v4.*/ -``` - -Then check whether a build artifact already exists for that minor version: - -```shell -# Example: checking if v4.6 has any released patch version -ls build_artifacts/v4/v4.6/ -``` - -There are two possible states: +- **To reach future minor and major versions,** change the newest minor template. That covers the next minor, whether or not its release branch exists yet, and every minor copied from it later. +- **To also reach future patches of minors that are already released,** change their templates in the same pull request. +- **For a change meant only for older minors,** such as a targeted backport, add the line `template-propagation: scoped` to the pull request description. -- **Template exists, but NO build artifact exists yet** (e.g., `template/v4/v4.6/` exists but `build_artifacts/v4/v4.6/` is empty or does not exist): this minor version has not been released. **Update `template/v4/v4.6/` directly** — it is both the next patch and the newest minor, so the change reaches 4.6.0 and every minor created after it. +Open template pull requests against `main`. Don't edit templates on release branches. The build merges `main` into an existing release branch, so the branch picks up the change from `main`. -- **Template exists AND a build artifact exists** (e.g., both `template/v4/v4.6/` and `build_artifacts/v4/v4.6/v4.6.0/` exist): this minor version has already been released. What you do depends on how far forward you want the change to reach: - - To reach only **future patches of this same minor** (4.6.1, 4.6.2, …), edit `template/v4/v4.6/` directly. The change will *not* reach any later minor/major version. - - To reach **future minor/major versions** (4.7.0, 5.0.0, …), you must create the next minor version's template by copying from it, then make your change there: +> **Why this rule exists:** templates are copied forward only **once**. A change merged to an older minor's template after the copy never reaches the newer minor unless it is also made there. That is how 4.6.0 missed the 4.5 skills change in #1343. - ```shell - cp -r template/v4/v4.6 template/v4/v4.7 - # Now edit template/v4/v4.7/ with your change - ``` +The required "Check template propagation" check enforces this on every pull request to `main`: - (If you want it in both the current minor's future patches *and* future minors, apply the change to `template/v4/v4.6/` and the new `template/v4/v4.7/`.) - -Include every template directory you created or edited in your PR, and open the PR against `main`. Because a new minor's template is created by copying the previous minor's template, landing your change in the newest minor template is what carries it into every minor created afterward. - -> **Why this rule exists:** templates are copied forward only **once** — at the moment a new minor version is first created (see "How new minor version templates are auto-created at build time" below). After that, each minor's template is an independent, frozen snapshot. So editing an *already-released* minor's template does not reach any newer minor. A common past mistake was editing v4.5 while v4.6 was already in flight: the change landed only in the v4.5 line and never reached v4.6+. Following the rule above avoids this — you always land the change on the newest minor template, creating it first if the current newest is already released. - -The "Check template propagation" check (`src/template_tools.py check`) backs this up on every pull request to `main`: - -- Changing a file in an older minor's template fails unless the same file also changes in the newest minor's template. For a change that is intentionally scoped to older minor lines only (e.g. a targeted backport), add the line `template-propagation: scoped` to your PR description to opt out. -- Adding a new minor template fails unless it is an exact copy of the previous minor's template, apart from paths listed in the description as `template-divergence: ` lines. +- Changing a file in an older minor's template fails unless the same file also changes in the newest minor's template, or the description contains `template-propagation: scoped`. +- Adding a new minor template fails unless it is an exact copy of the previous minor's template, apart from paths listed in the description as `template-divergence: ` lines. The merge queue runs this against the latest `main`, so a change to the previous minor that lands while the copy is in review can't be lost. - Editing the old per-major paths, such as `template/v4/dirs/`, fails, because the build only reads `template/v4/v4./`. #### Applying a security fix or infrastructure change (applies to all supported minor versions) -A security or infrastructure fix must reach **every supported minor line**, not just future ones. Apply the same fix to the templates of all supported minor versions, including the newest minor template (and, if the newest is already released and the fix must also reach future minors, a newly created next-minor template): +A security or infrastructure fix must reach **every supported minor line**, not just future ones. Apply the same fix to the templates of all supported minor versions, through the newest minor template: ``` template/v4/v4.0/Dockerfile template/v4/v4.1/Dockerfile ... template/v4/v4.6/Dockerfile # through the newest minor template -template/v4/v4.7/Dockerfile # including any next-minor template you just created ``` Applying the fix to each template explicitly (rather than relying on copy-forward) is intentional — it makes the scope of the fix auditable and prevents accidental feature leakage. -### How new minor version templates are auto-created at build time +### How new minor version templates are created + +When `X.Y.0` is released, the "Create Next Minor Template" workflow opens a pull request that copies `template/vX/vX.Y/` to `template/vX/vX.(Y+1)/` on `main`. Merge it before the first `X.(Y+1).0` build. `create-minor-version-artifacts` no longer creates a missing template: it fails and points to that pull request. -If no one creates the template directory before `create-minor-version-artifacts` runs, the tooling will automatically copy it from the previous minor's template (e.g., v4.6 → v4.7). This copy happens exactly **once per minor**, when that minor's template is first created, and only ever copies from the immediately-preceding minor. There is no ongoing sync: once a minor's template exists, later edits to an older minor's template are never propagated into it. That is exactly why the rule above has you land your change on the newest minor template. +To create one yourself, run `python src/template_tools.py create --version X.(Y+1)` and open the pull request against `main`. The first minor of a new major version (`X.0`) is still created by hand. ## Finding contributions to work on diff --git a/src/main.py b/src/main.py index 4a790351e..b18d668d2 100644 --- a/src/main.py +++ b/src/main.py @@ -30,6 +30,7 @@ generate_package_staleness_report, ) from release_notes_generator import generate_release_notes +from template_tools import require_minor_template from utils import ( get_dir_for_version, get_match_specs, @@ -75,36 +76,12 @@ def _delete_all_files_except_additional_packages_input_files(base_version_dir, v def _ensure_minor_template_exists(version: Version): - """Ensure the per-minor template directory exists for a new minor/major version. + """Fail unless the per-minor template directory for a new minor/major version exists. - If the directory already exists (e.g. a contributor pre-created it with - feature changes), it is left as-is. Otherwise, copies from the previous - minor's template (e.g. template/v4/v4.2/ -> template/v4/v4.3/). - - For the first minor of a new major version (minor == 0), the directory must - be created manually since there is no previous minor to copy from. + The template must already be on main. Copying it here put the copy only on the release + branch, so later template changes on main skipped the new minor. See template_tools.py. """ - major = version.major - minor = version.minor - minor_template_dir = f"template/v{major}/v{major}.{minor}" - - if os.path.exists(minor_template_dir): - return - - if minor == 0: - raise Exception( - f"Cannot auto-create template for v{major}.0 — no previous minor exists. " - f"Please create {minor_template_dir}/ manually with a Dockerfile and dirs/." - ) - - prev_minor_template_dir = f"template/v{major}/v{major}.{minor - 1}" - if not os.path.exists(prev_minor_template_dir): - raise Exception( - f"Previous minor template {prev_minor_template_dir}/ does not exist. " - f"Cannot create template for v{major}.{minor}." - ) - - shutil.copytree(prev_minor_template_dir, minor_template_dir) + require_minor_template(version.major, version.minor) def _create_new_version_artifacts(args): @@ -166,8 +143,8 @@ def _copy_static_files(base_version_dir, new_version_dir, new_version_major, new # - Patch bumps never accidentally gain/lose features from template edits # meant for newer minors. # - Security fixes can be applied to specific minor templates explicitly. - # - New minor/major versions get their template created at version-creation time - # (copied from the top-level template/v{major}/ directory). + # - A new minor's template is copied from the previous minor's on main before the + # minor's first build (see template_tools.py); the build never creates one. base_path = f"template/v{new_version_major}/v{new_version_major}.{new_version_minor}" for f in glob.glob(os.path.relpath(f"{base_path}/Dockerfile")): diff --git a/src/template_tools.py b/src/template_tools.py index e9a9e3944..9843610bb 100644 --- a/src/template_tools.py +++ b/src/template_tools.py @@ -5,17 +5,23 @@ Commands: + python src/template_tools.py create --version 4.7 + Create template/v4/v4.7/ by copying template/v4/v4.6/. + python src/template_tools.py check --base --head [--pr-body-file ] Check a pull request's template changes. See find_problems() for the rules. -Why the check exists: a new minor's template is a one-time copy of the previous minor's template. -Nothing carries later template changes forward, so a change merged to an older minor's template -after the copy skips the newer minor unless it is made there too (4.6.0 missed #1343 this way). +Why templates are created this way: a new minor's template is a one-time copy of the previous +minor's template. It used to be made during the minor's first build, which saved it only on the +release branch. Main never saw it, so template changes merged to main afterwards skipped the new +minor (4.6.0 missed #1343 this way). The copy now lands on main, through a pull request, before the +first build, and the build refuses to make one itself. """ import argparse import os import re +import shutil import subprocess import sys @@ -29,6 +35,36 @@ def minor_template_dir(major, minor): return f"template/v{major}/v{major}.{minor}" +def require_minor_template(major, minor, root="."): + """Raise unless template/vX/vX.Y/ exists. The build calls this for new minor and major versions.""" + path = minor_template_dir(major, minor) + if os.path.isdir(os.path.join(root, path)): + return + if minor == 0: + hint = f"Create {path}/ (a Dockerfile and dirs/) by hand in a pull request to main." + else: + hint = ( + "Create it on main first: merge the pull request the 'Create Next Minor Template' workflow opened " + f"when {major}.{minor - 1}.0 was released, or run `python src/template_tools.py create " + f"--version {major}.{minor}` and open that pull request yourself." + ) + raise Exception(f"{path}/ does not exist. {hint}") + + +def create_minor_template(major, minor, root="."): + """Create template/vX/vX.Y/ by copying the immediately preceding minor's template.""" + path = minor_template_dir(major, minor) + if minor == 0: + raise Exception(f"{path}/ is the first minor of a new major version; create it by hand.") + if os.path.exists(os.path.join(root, path)): + raise Exception(f"{path}/ already exists.") + previous = minor_template_dir(major, minor - 1) + if not os.path.isdir(os.path.join(root, previous)): + raise Exception(f"{previous}/ does not exist, so there is nothing to copy into {path}/.") + shutil.copytree(os.path.join(root, previous), os.path.join(root, path)) + return path + + def parse_minor_template_path(path): """Return (major, minor, path inside the template) for a per-minor template file, or None.""" match = _MINOR_TEMPLATE_PATH.match(path) @@ -166,15 +202,27 @@ def run_check(base, head, pr_body): return 1 +def _parse_version(text): + match = re.fullmatch(r"(\d+)\.(\d+)", text.strip()) + if not match: + raise argparse.ArgumentTypeError(f"expected ., got {text!r}") + return int(match.group(1)), int(match.group(2)) + + def main(argv=None): parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) commands = parser.add_subparsers(dest="command", required=True) + create = commands.add_parser("create", help="Create a minor template by copying the previous minor's.") + create.add_argument("--version", required=True, type=_parse_version, help="., e.g. 4.7") check = commands.add_parser("check", help="Check a pull request's template changes.") check.add_argument("--base", required=True, help="Commit the pull request merges into.") check.add_argument("--head", required=True, help="The pull request merged with --base.") check.add_argument("--pr-body-file", help="File containing the pull request description.") args = parser.parse_args(argv) + if args.command == "create": + print(f"Created {create_minor_template(*args.version)}/") + return 0 pr_body = "" if args.pr_body_file: with open(args.pr_body_file) as f: diff --git a/test/test_template_tools.py b/test/test_template_tools.py index 8e0e3e529..4d5ebf341 100644 --- a/test/test_template_tools.py +++ b/test/test_template_tools.py @@ -5,11 +5,13 @@ pytestmark = pytest.mark.unit from template_tools import ( + create_minor_template, find_copy_problems, find_legacy_path_problems, find_problems, find_propagation_problems, parse_minor_template_path, + require_minor_template, run_check, ) @@ -104,6 +106,27 @@ def test_pr_description_opt_out(): assert find_problems(changed, set(head), head, "Backport only.\n\ntemplate-propagation: scoped\n") == [] +def test_create_minor_template_copies_the_previous_minor(tmp_path): + (tmp_path / "template/v4/v4.6/dirs").mkdir(parents=True) + (tmp_path / "template/v4/v4.6/Dockerfile").write_text("FROM x\n") + assert create_minor_template(4, 7, root=str(tmp_path)) == "template/v4/v4.7" + assert (tmp_path / "template/v4/v4.7/Dockerfile").read_text() == "FROM x\n" + assert (tmp_path / "template/v4/v4.7/dirs").is_dir() + with pytest.raises(Exception, match="already exists"): + create_minor_template(4, 7, root=str(tmp_path)) + with pytest.raises(Exception, match="nothing to copy"): + create_minor_template(4, 9, root=str(tmp_path)) + with pytest.raises(Exception, match="by hand"): + create_minor_template(5, 0, root=str(tmp_path)) + + +def test_require_minor_template(tmp_path): + with pytest.raises(Exception, match="template/v4/v4.7/ does not exist"): + require_minor_template(4, 7, root=str(tmp_path)) + (tmp_path / "template/v4/v4.7").mkdir(parents=True) + require_minor_template(4, 7, root=str(tmp_path)) + + def test_run_check_reads_the_git_trees(tmp_path, monkeypatch, capsys): def git(*args): result = subprocess.run(