-
Notifications
You must be signed in to change notification settings - Fork 16
Add Buildkite pipeline to check flagship AMIP performance #2438
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
base: main
Are you sure you want to change the base?
Conversation
|
Maybe a better approach here might be to create a single AMIP (or CMIP?) repo and have dependency repos trigger runs there (maybe use a label like In this pipeline we could be selective about what PRs we send to the integrated system repo based on changed paths, e.g.,: steps:
- label: ":mag: Check for Numerical Changes"
key: "check-changes"
command: |
# Check if files in src/numerics or src/kernels changed
if git diff --name-only HEAD~1 | grep -E '^(src/|include/|Makefile)'; then
buildkite-agent meta-data set "run_e2e" "true"
else
buildkite-agent meta-data set "run_e2e" "false"
fi
- label: ":rocket: Trigger AMIP E2E"
if: buildkite_agent_meta_data_get("run_e2e") == "true"
trigger: "amip-main-pipeline"
build:
env:
CORE_REPO_COMMIT: "${BUILDKITE_COMMIT}" |
|
I don't really know how submodules work in git, but if we used a Julia project with the coupler in it, the AMIP driver script and config files should still be accessible. Also I'm not sure if this pipeline has been setup correctly because it is not uploading. Do you have an estimate on how long the e2e test takes? |
Interesting. So I can just keep the relative paths even though ClimaCoupler.jl will live in the depot directory, not here?
It definitely seems to be stuck, but I can't tell what I did wrong. I created the pipeline on buildkite.com first, then pushed the YAML file to this branch. Let me know (when you get a chance; I know you have a presentation to give soon) if you can see any issues: https://buildkite.com/clima/climacore-end-to-end-performance/settings |
I haven't look too closely at the buildkite settings, but I think you need to add the following to the YAML steps: I did this already. |
This reverts commit ad2d968.
|
I'm not sure how much can be gained by using a sysimage or precompileCI here. My understanding is that almost anything that is compiled for ClimaAtmos, ClimaLand, and ClimaCoupler, would be invalidated because each PR/commit would be using a different version of ClimaCore. Probably still worth looking into though. I don't think this should be run on every push, but I also don't know what a good alternative would be... |
Yeah, that's a tough one. I did at least set it up to skip the simulation if |
|
@nefrathenrici do you think it would be smarter/possible to put this into the |
|
The actual timestepping part only take ~30 seconds. Apparently it's the compilation that takes most of the time, as I'm told by @imreddyTeja and @dennisYatunin. |
I think |
|
Looks like Buildkite does have some ability to skip builds. Maybe we could flip that around and have people opt-in by putting |
IMO it would be nice to know that a PR either enhances or at least doesn't hurt performance before merging. |
|
I just set up conditional filtering so builds only run if |
|
The pipeline looks good to me. My understanding of submodules is weak, but it seems convenient. An alternative would be making a different Julia Project that has ClimaCore dev'ed, and ClimaCoupler added. Then you could use |
dennisYatunin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the submodule is pretty neat. Unlike a shared repo, submodules could maybe let us pin different combinations of upstream packages for different repos. That way, incompatible code introduced upstream would have a minimal effect on CI for downstream packages.
Can the current filter on "build.message" get overruled on merge, though? Even if someone doesn't know/care about the performance check, it would still be good to run it once per PR.

Resolves #2433
Here we add the ClimaCoupler submodule to check how each PR changes performance in the most important case, since we have determined that the simpler benchmarks in this repo are not indicative enough. I suppose we could limit this to ClimaAtmos, but if the integrated land could be impacted by ClimaCore changes, why not test the whole AMIP pipeline end-to-end?
In lieu of a submodule, we could use a Julia project with the Coupler package in it, but I'm not sure if the AMIP driver script and config files would be accessible.
This pipeline will only run for commits with
[perf]in their commit message, so we don't tie upclimawith extra jobs. If you need to kick it off, you can always rungit commit --allow-empty -m "Trigger build [perf]" && git push.TODO