Allow Rebases to Model Deployment PRs

[CodeGat]

Background

As per a conversation with @aidanheerdegen:

Model deployment repositories aren't able to rebase their pull request branches because the workflow makes use of the number of commits on a branch as the name of the spack environment (like <MODEL>-pr<PR NUMBER>-<COMMIT NUMBER>, eg. access-om3-pr12-2), see

build-cd/.github/workflows/ci.yml

Lines 182 to 193 in ab2b346

	- name: Generate Versions
	id: version
	# This step generates the release and prerelease version numbers.
	# The release is a general version number from the spack.yaml, looking the
	# same as a regular release build. Ex. 'access-om2@git.2024.01.1' -> '2024.01.1'
	# The prerelease looks like: `pr<pull request number>-<number of commits on this branch>`.
	# Ex. Pull Request #12 with 2 commits on branch -> `pr12-2`.
	run: |
	echo "release=$(yq '${{ env.SPACK_YAML_MODEL_YQ }} | split("@git.") | .[1]' spack.yaml)" >> $GITHUB_OUTPUT
	number_of_commits=$(git rev-list --count ${{ github.event.pull_request.base.sha }}..HEAD)
	echo "prerelease=pr${{ github.event.pull_request.number }}-${number_of_commits}" >> $GITHUB_OUTPUT

Since it is a requirement for now that users can't rebase, make this explicit in both the documentation, and in the workflow itself.

Update

We are now allowing PRs to rebase because we are using the deployment number + 1 instead of commit number

[harshula]

Please see if there's a way to allow users to rebase their Pull Requests.

[aidanheerdegen]

Please see if there's a way to allow users to rebase their Pull Requests.

I'm not convinced this is a good idea. Once you rebase there Is no longer a commit that is associated with previous pre-release deployments in the PR. They're effectively orphaned, and so kinda useless.

If a user needs to rebase they should just make a new PR.

[harshula]

The commit should still be in the repository, just not in the branch.

Use-case:

1. User creates a development branch and commits changes.
2. User pushes the development branch and commits to ACCESS-NRI and creates a PR to test the changes. This is what we've announced.
3. The Pre-Release build-cd mechanism builds and deploys the changes to Gadi.
4. The user does some testing on Gadi and decides to change the source code. This might happen multiple times.

I don't think we should then tell the user that they are not allowed to git rebase their own development branch. Can we please focus on the end-user experience?

[CodeGat]

If we were to go down this path, we need to figure out a different bit of information to pin to the spack environment/modules for Prereleases.

Currently, we use the commit number, for example, access-om2/pr12-13 for the 13th commit in the 12th PR. This would not work for rebased PRs, as the commit number may change.

Alternatives could be:

• The short commit hash that is being deployed. For example: access-om2/pr12-hdy38dh for a prerelease of a deployment with commit hdy38dh. This would link the commit with the environment quite well, but doesn't give a timeline of environments quite as well as as ordinal. From a implementation point of view, this would be quite easy.
• The number of spack environments that are currently deployed under that pull request. For example, access-om2/pr12-7, where we essentially run $(spack env list | grep 'access-om2/pr12-*' | wc) + 1. This keeps the timeline aspect, but is disconnected from the commits used. This would also be a bit of a rework of the current infra, since we have to interrogate Gadi to get the spack envs much earlier than we do currently.

I'd prefer number 1 if we were to change stuff.

[harshula]

Could you use:

• date '+%s'
• The number of comments in a PR
• ISO 8601 e.g. date '+%Y%m%dT%H%M'

[harshula]

Hi @CodeGat , Where an old suffix was reused, the spack.yaml didn't get updated to the latest one in the PR:

$ ls -l /g/data/vk83/prerelease/apps/spack/0.22/environments/access-om2-pr76-27
total 36
-rw-rw----+ 1 <USER> vk83   321 Nov 14 11:13 build-db-pkgs.json
-rw-rw----+ 1 <USER> vk83  2611 Nov 14 11:13 spack.location
-rw-rw----+ 1 <USER> vk83  2469 Nov 14 11:13 spack.location.json
-rw-rw----+ 1 <USER> vk83 18344 Nov 14 11:12 spack.lock
-rw-r--r--+ 1 <USER> vk83  1371 Oct 25 16:04 spack.yaml

$ spack config blame modules
--- modules:
[...]/environments/access-om2-pr76-27/spack.yaml:43    default:
[...]/environments/access-om2-pr76-27/spack.yaml:44      enable:
[...]/environments/access-om2-pr76-27/spack.yaml:45      - tcl
[...]/environments/access-om2-pr76-27/spack.yaml:46      roots:
[...]/environments/access-om2-pr76-27/spack.yaml:47        tcl: $spack/../release/modules
[...]/environments/access-om2-pr76-27/spack.yaml:48        lmod: $spack/../release/lmod
[...]/environments/access-om2-pr76-27/spack.yaml:49      tcl:
[...]/environments/access-om2-pr76-27/spack.yaml:51        include:
[...]/environments/access-om2-pr76-27/spack.yaml:52        - access-om2
[...]/environments/access-om2-pr76-27/spack.yaml:53        exclude_implicits: True
[...]/environments/access-om2-pr76-27/spack.yaml:54        all:
[...]/environments/access-om2-pr76-27/spack.yaml:55          autoload: run
[...]/environments/access-om2-pr76-27/spack.yaml:56          conflict:
[...]/environments/access-om2-pr76-27/spack.yaml:57          - '{name}'
[...]/environments/access-om2-pr76-27/spack.yaml:58          environment:
[...]/environments/access-om2-pr76-27/spack.yaml:59            set:
[...]/environments/access-om2-pr76-27/spack.yaml:60              SPACK_{name}_ROOT: '{prefix}'
[...]/environments/access-om2-pr76-27/spack.yaml:61        projections:
[...]/environments/access-om2-pr76-27/spack.yaml:62          all: '{name}/{version}'
[...]/environments/access-om2-pr76-27/spack.yaml:63          access-om2: '{name}/pr76-27'
[...]/spack/etc/spack/modules.yaml:10                      hash_length: 0
[...]/spack/etc/spack/defaults/modules.yaml:51           lmod:
[...]/spack/etc/spack/defaults/modules.yaml:52             all:
[...]/spack/etc/spack/defaults/modules.yaml:53               autoload: direct
[...]/spack/etc/spack/defaults/modules.yaml:54             hierarchy:
[...]/spack/etc/spack/defaults/modules.yaml:55             - mpi
[...]/spack/etc/spack/defaults/modules.yaml:19         prefix_inspections:
[...]/spack/etc/spack/defaults/modules.yaml:20           ./bin:
[...]/spack/etc/spack/defaults/modules.yaml:21           - PATH
[...]/spack/etc/spack/defaults/modules.yaml:22           ./man:
[...]/spack/etc/spack/defaults/modules.yaml:23           - MANPATH
[...]/spack/etc/spack/defaults/modules.yaml:24           ./share/man:
[...]/spack/etc/spack/defaults/modules.yaml:25           - MANPATH
[...]/spack/etc/spack/defaults/modules.yaml:26           ./share/aclocal:
[...]/spack/etc/spack/defaults/modules.yaml:27           - ACLOCAL_PATH
[...]/spack/etc/spack/defaults/modules.yaml:28           ./lib/pkgconfig:
[...]/spack/etc/spack/defaults/modules.yaml:29           - PKG_CONFIG_PATH
[...]/spack/etc/spack/defaults/modules.yaml:30           ./lib64/pkgconfig:
[...]/spack/etc/spack/defaults/modules.yaml:31           - PKG_CONFIG_PATH
[...]/spack/etc/spack/defaults/modules.yaml:32           ./share/pkgconfig:
[...]/spack/etc/spack/defaults/modules.yaml:33           - PKG_CONFIG_PATH
[...]/spack/etc/spack/defaults/modules.yaml:34           ./:
[...]/spack/etc/spack/defaults/modules.yaml:35           - CMAKE_PREFIX_PATH

[CodeGat]

Yeah, I suspect that would have happened...it would have failed to run spack env create in the deployment workflow and errored out

[aidanheerdegen]

@CodeGat and discussed this and I am on-board team-rebase. Or at least, I am on-board team "don't stop people rebasing".

I support ordinal numbering because I think that is the best solution for users, and maximises "user delight" when all they have to do is change a single digit in their config.yaml to update to their next pre-release environment. From an implementation point of view it only requires inspecting the last environment, finding the number of the last pre-release environment and incrementing the counter.

My caveats still stand about re-basing a deployment repo

• The pre-release environments lose connection with commits in the PR. It is an assumption that the pre-release environments would be the same with the equivalent commit in the rebased PR, and one that might be difficult to know how accurate it is
• Deployment repos aren't the same as code repos, and the need for rebasing is less compelling
• It would be simple to open a new PR from a rebased version of the branch
• If it was deemed necessary it would be quite possible to regenerate all the environments for a rebased PR, which would be a more correct approach