Skip to content
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

settings.json: Release 0.22 spack Does Not Contain Hash Due to access-nri/spack Rebase #140

Open
CodeGat opened this issue Sep 24, 2024 · 6 comments

Comments

@CodeGat
Copy link
Member

CodeGat commented Sep 24, 2024

See warning in Validate [config/settings.json] Deployment Settings here: https://github.com/ACCESS-NRI/build-cd/actions/runs/11007422156/job/30563301689#step:5:245

This is due to ACCESS-NRI/spack@21da7d7 no longer existing on the releases/v0.22 branch of access-nri/spack. This is because after syncing our fork with the upstream for its 0.22.2 updates, @harshula rebased our ACCESS-NRI-specific fixes on top of that.

This currently prevents Model Deployment Repo PRs from being merged as any warnings in validation are converted to errors upon merging.

We need to come to a decision where either:

  • We remove the check that verifies that the commit hash referenced is in the release/v$VERSION branch. This would remove a guardrail that prevents people from modifying the 0.XX install to a commit hash not within that major version. Relevant code here:
    - name: Verify Spack Commits Exist
    id: spack-hash-all-exist
    env:
    GH_TOKEN: ${{ github.token }}
    CHECKOUT_DIR: ./spack-metadata
    shell: bash
    # Verify that every commit referenced actually exists on the
    # `releases/VERSION` branch in access-nri/spack, but don't
    # bother checking out the code.
    run: |
    gh repo clone access-nri/spack ${{ env.CHECKOUT_DIR }} -- --no-checkout --bare --filter=blob:none
    # Essentially, pull out all the spack 'major: hash' sections and iterate
    version_hashes=$(jq --compact-output --raw-output \
    '.deployment.${{ inputs.target }}[]
    | keys[] as $major | .[$major].spack as $hash
    | "\($major) \($hash)"' \
    ${{ inputs.settings-path }}
    )
    # For each of the version hashes, check if $hash is in releases/v$major
    while read -ra line; do
    version=${line[0]}
    hash=${line[1]}
    echo "Checking if $hash is in $version"
    if ! git -C ${{ env.CHECKOUT_DIR }} merge-base --is-ancestor $hash releases/v$version; then
    echo "::${{ inputs.error-level }}::Commit $hash does not exist on branch releases/v$version"
    failed=true
    fi
    done <<< "$version_hashes"
    if [ -n "$failed" ]; then
    msg="Some commits referenced do not exist in access-nri/spack. Check the workflow logs."
    echo "::${{ inputs.error-level }}::$msg"
    echo "msg=$msg" >> $GITHUB_OUTPUT
    fi
    rm -rf ${{ env.CHECKOUT_DIR }}
  • We figure out a way to keep our forks spack fixes commits stable
@aidanheerdegen
Copy link
Member

We're at war with our smarter selves. I wonder who will win?

@harshula
Copy link
Contributor

Ironically, this is probably one of the arguments for using patch files instead of a branch that tracks upstream with local changes as commits.

@harshula
Copy link
Contributor

Hi @CodeGat , Could we do something like:

diff --git a/config/settings.json b/config/settings.json
index 94fe836..7c3fbf2 100644
--- a/config/settings.json
+++ b/config/settings.json
@@ -5,16 +5,19 @@checks
       "Release": {
         "0.20": {
           "spack": "6812713cf470b473a607f0de0e8e1cf53f804fb7",
+          "access-spack": "x",
           "spack-config": "2024.03.22"
         },
         "0.22": {
           "spack": "21da7d7e2b5e2680cd9d2e0a2fb4a7d13d8baa9d",
+          "access-spack": "y",
           "spack-config": "2024.07.05"
         }
       },
       "Prerelease": {
         "0.22": {
           "spack": "21da7d7e2b5e2680cd9d2e0a2fb4a7d13d8baa9d",
+          "access-spack": "z",
           "spack-config": "2024.07.05"
         }
       }

where:
spack denotes the latest upstream commit
access-spack denotes the latest ACCESS-NRI commit

Then only do your checks on the spack variable.

@CodeGat
Copy link
Member Author

CodeGat commented Sep 25, 2024

But we test against the commit hash of the deployed access-nri/spack hash - what does that have to do with the upstream one?

@harshula
Copy link
Contributor

The latest upstream commit hash will be an ancestor of the latest ACCESS-NRI commit. I think you should be able to create a better test when you take into account how the ACCESS-NRI fork of Spack operates.

@CodeGat
Copy link
Member Author

CodeGat commented Oct 4, 2024

@harshula and myself came to two possibilities to fix this:

  • The above, where we add .deployment.TARGET.TYPE.MAJOR.spack-upstream which is used to test that it, and therefore it's child commit (the spack commit hash), is on the access-nri/spack MAJOR branch. One downside to this is that editors of config/settings.json must do their own homework on what the value of upstream-spack is, all for a single check.
  • A variation of the above, where we don't add an extra field, but we must come up with a heuristic to find out the last parent of the spack field that is on the upstream spack, and check that that is on the access-nri/spack MAJOR branch. Issues with this is that it might be difficult to determine what the last true commit on spack/spack is, as opposed to one that is introduced during an interative rebase in which we swap/drop/create new commits. Potential solution is to link the upstream as a remote and check against upstream/releases/vMAJOR.

image

Both of those solutions will take time, so currently I'll opt for Option 3 for now, which is:

  • Comment out the test and link this discussion for when I have time to implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants