-
Notifications
You must be signed in to change notification settings - Fork 434
Add workflow for weekly kokkos develop testing #7643
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: master
Are you sure you want to change the base?
Add workflow for weekly kokkos develop testing #7643
Conversation
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.
@bartgol I added comments for every place this differs from eamxx-sa. Is there an easy way for me to manually test a .github workflow?
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 that, unless the workflow is in master, it must run at least once to appear on the actions page. If you put back the PR trigger, it may run, and at that point, you could manually test it form the actions tab via workflow dispatch...
@bartgol Looks like it doesn't pick it up even with the PR tab. |
8845fd6
to
a98fe3e
Compare
Previously, C++20 was not being used (EAMxx overwrote user defined CMAKE_CXX_STANDARD var) and tests were passing. Now that we are using C++20, the builds are failing, so I changed to draft while I address the errors. |
c16f131
to
5da2ddc
Compare
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.
One thought I just had: why don't we make the new workflow do the following:
- create a temporary branch like
kokkos-develop-testing/<DATE>
- update kokkos submod of ekat in this branch, and push to the remote
- call the eamxx-v1 and eamxx-sa (or just one) workflow
the AI bot suggests this step for triggering another workflow (not sure if it works)
- name: Trigger target workflow
uses: actions/github-script@v6
with:
github-token: ${{ secrets.PAT_TOKEN }} # Use a PAT with 'repo' scope
script: |
await github.rest.actions.createWorkflowDispatch({
owner: context.repo.owner,
repo: context.repo.repo,
workflow_id: 'target-workflow.yml', // The file name of the target workflow
ref: 'main', // The branch on which the workflow should be triggered
inputs: {
# Optional inputs for the dispatched workflow
my_input_key: 'my_input_value',
another_input: 'another_value'
}
});
But whatever the syntax, I'm thinking that calling the existing eamxx-sa/eamxx-v1 workflows (to run on the newly created "kokkos-update" branch) is more robust, as we only have to maintain the testing framework in one workflow.
I can try, the only difference is that new cmake options must be added in, so maybe if I can hack those in this will delete a lot of duplicated code. |
8c1a746
to
3d33736
Compare
9fa0823
to
ad21ae7
Compare
#7691) In C++20 you cannot aggregate initialize a stuct which has a default constructor defined. We shouldn't need these constructors since views themselves have default constructors. Also, allow EAMxx to take a CMAKE_CXX_STANDARD option (default to C++17). Creating this PR so that #7643 won't have to run full CI while I test. [BFB]
In C++20 you cannot aggregate initialize a stuct which has a default constructor defined. We shouldn't need these constructors since views themselves have default constructors. Also, allow EAMxx to take a CMAKE_CXX_STANDARD option (default to C++17). Creating this PR so that #7643 won't have to run full CI while I test. [BFB]
ad21ae7
to
2ab2f9a
Compare
9b8dc53
to
61eeff8
Compare
61eeff8
to
66530c9
Compare
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.
@bartgol This is ready to take a look. I opted to make changes inside the test-all-eamxx
action instead of having a new update-kokkos-to-dev
action which checks out the branch and edits the .cmake
files with 3 needed options. I did this because I needed to modify test-all-eamxx
anyways to alter the name of the cdash report to distinguish from the normal nightlies.
I manually ran and submitted to CDASH. Here's one of the tests: https://my.cdash.org/build/3099376
Note, CUDA fails because we need to update version 12.1 -> 12.2+ to satisfy Kokkos' version requirements.
kokkos_develop: | ||
description: 'Whether or not this is a test using Kokkos develop branch' | ||
required: false | ||
default: 'false' | ||
valid_values: | ||
- 'true' | ||
- 'false' |
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.
Basically, eamxx-sa-testing.yml
needs to set this variable if this is a Kokkos dev branch test.
- name: Checkout Kokkos develop branch | ||
if: ${{ inputs.kokkos_develop == 'true' }} | ||
working-directory: externals/ekat/extern/kokkos | ||
run: | | ||
echo "Check out up-to-date Kokkos develop branch" | ||
if ! git remote | grep -q upstream; then | ||
git remote add upstream https://github.com/kokkos/kokkos.git | ||
fi | ||
git fetch upstream | ||
git checkout upstream/develop |
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.
Before running test-all-eamxx, checkout Kokkos develop.
# Add additional id for CDASH submit, and necessary cmake vars if this is a kokkos-develop test | ||
if [ "${{ inputs.kokkos_develop }}" = "true" ]; then | ||
cmd+=" --additional-id kokkos_develop" | ||
cmd+=" -c CMAKE_CXX_STANDARD=20" | ||
cmd+=" -c Kokkos_ENABLE_IMPL_VIEW_LEGACY=ON" | ||
cmd+=" -c Kokkos_ENABLE_DEPRECATED_CODE_4=ON" | ||
fi |
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.
Add additional ID and set necessary CMake vars.
I need to take a look at the issues with needing Kokkos_ENABLE_IMPL_VIEW_LEGACY=ON
(something with ekat::subview
) because, as the name suggests, this will be phased out at some point.
testid = "_" + self._additional_id if self._additional_id != "" else "" | ||
result += f"-DBUILD_NAME_MOD={build_name_mod}{testid} " |
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.
Let me know what you think of this, can't decide if I think it's too hacky, but don't have any other good ideas.
This is in an effort to continually update Kokkos with new releases, instead of large subversion jumps each time a component needs a new version.