PR-Based Repro CI

[CodeGat]

This PR adds PR-based repro CI for access-om3-configs.

In this PR:

• Add config.yml to validate config/ci.json
• Add ci.yml for PR-related events
  • Added access-om3-configs-specific logic for committing changes to docs/
• Added CONTRIBUTING.md to detail PR process
• Update README.md to include info on config branches, config tags and PR CI

Repo Settings Modified:

• Gadi GitHub Environment
• secrets.GH_FORCE_PUSH_TOKEN, secrets.GH_COMMIT_CHECK_TOKEN

Closes #120

[anton-seaice]

Thanks @CodeGat

I will review properly next week. I guess we need to wait until the binary path is changed before merging.

Just confirming, dev- branches don't have any versioning right ? How does the checksum in dev- branches get update (i.e. its not !bump ) ?

[CodeGat]

Just confirming, dev- branches don't have any versioning right ? How does the checksum in dev- branches get update (i.e. its not !bump ) ?

Correct. The checksum in dev-* is updated either via the generate-initial-checksum workflow, or by opening a PR into a release-* branch, where we check repro, then if the checksums differ you can do !bump major.

[anton-seaice]

Oh I see. We would like to compare and update checksums on PR into dev- branches also. Theres no versioning policy for dev- branches, so could it be !bump checksum (or similar)

[CodeGat]

We don't have that functionality just yet, but we will as part of ACCESS-NRI/model-config-tests#38 once that's done

[aidanheerdegen]

A question/suggestion or two.

[aidanheerdegen]

LGTM. Just a question.

[anton-seaice]

I'd prefer not to merge this until we have the location of the build resolved (the tests this change introduces would fail currently I think)

[aidanheerdegen]

@anton-seaice under what circumstances would you not want to commit update docs?

[anton-seaice]

@anton-seaice under what circumstances would you not want to commit update docs?

The most likely is that the docs have shown that something unrelated to the intended change has changed, that should be resolved in a way that is different to committing the docs.

One example is if a model version changes the default settings, which would show up in the these docs

[aidanheerdegen]

One example is if a model version changes the default settings, which would show up in the these docs

Why wouldn't you want that reflected in the docs?

Sure I can see if you didn't intend the change you'd want to a subsequent change to the config, but then when the docs are regenerated you'd be sweet. No?

I can't see why you wouldn't always want the docs to exactly reflect the state of the config and the model (default config values).

[anton-seaice]

Maybe I misunderstood the question ... if the docs don't match the test fails non ?

[aidanheerdegen]

Maybe I misunderstood the question ... if the docs don't match the test fails non ?

The docs are quantitatively different to a repro hash. I think you just want to make sure they're always up to date with the current config and build.

In which case no test is necessary. Just always pull them back and commit them. git will take care of knowing if they've changed or not, in which case no commit is necessary.

[anton-seaice]

Does the workflow cause a failure at this line?

access-om3-configs/.github/workflows/ci.yml

Lines 76 to 79 in 4f0559f

	if git diff --cached --exit-code ./docs; then
	echo "::warning::Modification to 'docs/', need to commit with '!update docs'"
	exit 1
	fi

Then the developer can decide how to fix it ... update the docs or change the config

[aidanheerdegen]

Does the workflow cause a failure at this line?

access-om3-configs/.github/workflows/ci.yml

Lines 76 to 79 in 4f0559f

	if git diff --cached --exit-code ./docs; then
	echo "::warning::Modification to 'docs/', need to commit with '!update docs'"
	exit 1
	fi

Then the developer can decide how to fix it ... update the docs or change the config

Yes it does, because that was the requested behaviour (I assume).

But my point is that it doesn't matter if an unwanted change in the docs is committed to the repo. Change the config and push and the PR will update, and update the docs.

It makes the CI significantly simpler if we just assume all changes to the docs are committed.

[anton-seaice]

But my point is that it doesn't matter if an unwanted change in the docs is committed to the repo. Change the config and push and the PR will update, and update the docs.

It matters in the sense that the proponent of the change has not confirmed that they intended the change to do something which changes the docs, and therefore could slip by unnoticed.

[aidanheerdegen]

It matters in the sense that the proponent of the change has not confirmed that they intended the change to do something which changes the docs, and therefore could slip by unnoticed.

@CodeGat and I had a chat about this today and we think this can be covered by the existing !bump logic. As the docs must reflect the current state of the model and configuration then as long as they are committed when the version is bumped then it should satisfy your requirements.

It means that the docs are updated when the version is updated, but it requires no specific intervention by the developer. This is a good thing: the docs won't get out of date, they will always reflect the current state of the model and the config, but there is no requirement for the developer to do anything to make that happen.

[aekiss]

It matters in the sense that the proponent of the change has not confirmed that they intended the change to do something which changes the docs, and therefore could slip by unnoticed.

@CodeGat and I had a chat about this today and we think this can be covered by the existing !bump logic. As the docs must reflect the current state of the model and configuration then as long as they are committed when the version is bumped then it should satisfy your requirements.

It means that the docs are updated when the version is updated, but it requires no specific intervention by the developer. This is a good thing: the docs won't get out of date, they will always reflect the current state of the model and the config, but there is no requirement for the developer to do anything to make that happen.

I understand your point about having docs always synced, but the crucial thing is that the developer gets notified when the docs change, because it may alert them to an unintended configuration change. Will that happen with your proposed approach @aidanheerdegen? i.e. is !bump a manual intervention that's required before a PR can be merged?

The scenario we're trying to avoid is an exe update with a change in defaults producing an unwanted change to the parameters seen by the model. See discussion here COSIMA/access-om3#117

[CodeGat]

is !bump a manual intervention that's required before a PR can be merged?

Yes, a !bump must always be done regardless of repro success/failure - it prevents merging because we need to update the metadata.yaml version. One reason not to do it is if the result of the repro was unexpected and you wish to push further updates.

the crucial thing is that the developer gets notified when the docs change

In the current solution we add a comment noting that the docs have changed - this can be modified slightly to remove the "Comment !update_docs if you want these changes committed." See https://github.com/ACCESS-NRI/access-om3-configs/pull/122/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR81-R92

[anton-seaice]

You only need to !bump to merge into release- branches though right ? Merging into dev- branches will update the checksum without !bump right ?

[CodeGat]

I'm still working on the PR-based-repro on dev-* branches stuff at the moment (see ACCESS-NRI/model-config-tests#38) - for now you would need to use the User-dispatchable workflow

[aidanheerdegen]

I understand your point about having docs always synced, but the crucial thing is that the developer gets notified when the docs change, because it may alert them to an unintended configuration change. Will that happen with your proposed approach @aidanheerdegen? i.e. is !bump a manual intervention that's required before a PR can be merged?

An unintended configuration change should also produce a change in bitwise reproducibility right? If not, then the change is side-effect free, e.g. a formatting change in the doc output.

As @CodeGat said yes a !bump is required for release branches to be merged.

The scenario we're trying to avoid is an exe update with a change in defaults producing an unwanted change to the parameters seen by the model. See discussion here COSIMA/access-om3#117

If this is something you always want to be alerted to then git hooks might be a good approach: always warn when docs have been updated before committing them.

[CodeGat]

Hey PR-followers, I'm going to split this PR into three different ones to help get parts of it merged that are fairly set in stone. Will link the relevant PRs here, but it will be basically:

• The regular PR workflow as it exists for the other models (OM2, ESM1.5/6, etc), with no OM3-specific functionality
• An extension to the above that adds the !test repro [commit] comment command which allows the testing and committing of reproducibility results in non-release-* workflows
• Finally, a PR that integrates docs into the workflow, once that is sorted out.