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

Get Trilinos auto PR testing code to use TriBITS-based package enable logic #3133

Closed
bartlettroscoe opened this issue Jul 17, 2018 · 23 comments
Assignees
Labels
Framework tasks Framework tasks (used internally by Framework team) stage: in review Primary work is completed and now is just waiting for human review and/or test feedback type: enhancement Issue is an enhancement, not a bug

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Jul 17, 2018

CC: @william76, @jwillenbring

Next Action Status

With the merge of PR #3258 8/14/2018 this should be complete. As of 9/12/2018 no problems have been reported in last 4+ weeks so all is good!

Description

This story is to get the Trilinos auto PR testing code to use TriBITS-based logic for determining what to enable and test based on files that are changed in the PR branch.

The benefits of making this change are:

  • When the set of Primary Tested packages changes, the code will automatically adjust

  • Specialized logic can be applied to determine if all packages need to be tested or not. For example, if files are changed under Trilinos/cmake/ctest/drivers then that should not trigger the auto PR tester to test all of Trilinos.

@bartlettroscoe bartlettroscoe added type: enhancement Issue is an enhancement, not a bug Framework tasks Framework tasks (used internally by Framework team) labels Jul 17, 2018
@bartlettroscoe bartlettroscoe added the stage: in progress Work on the issue has started label Jul 17, 2018
bartlettroscoe added a commit to TriBITSPub/TriBITS that referenced this issue Jul 17, 2018
…rilinos/Trilinos#3133)

When returning to the shell, it is better to use commas instead of
semi-colons.  Also, the script filter-packages-list.py takes the list of
packages separated by commas instead of semi-colons.  This is needed for the
Trilinos auto PR testers upgrade.
bartlettroscoe added a commit to TriBITSPub/TriBITS that referenced this issue Jul 17, 2018
…rilinos/Trilinos#3133)

I also updated the documentation a little.  Now this should be ready to handle
the output from get-tribits-packages-from-files-list.py which can contain
ALL_PACKAGES and it should be ready to be used for the refactored Trilinos
auto PR tester (trilinos/Trilinos#3133).

Build/Test Cases Summary
Enabled Packages:
Enabled all Packages
0) MPI_DEBUG => passed: passed=268,notpassed=0 (0.72 min)
1) SERIAL_RELEASE => passed: passed=268,notpassed=0 (0.67 min)
2) MPI_DEBUG_CMAKE-3.6.2 => passed: passed=293,notpassed=0 (0.63 min)
3) SERIAL_RELEASE_CMAKE-3.6.2 => passed: passed=293,notpassed=0 (0.74 min)
4) MPI_DEBUG_CMAKE-3.11.1 => passed: passed=313,notpassed=0 (0.95 min)
5) SERIAL_RELEASE_CMAKE-3.11.1 => passed: passed=313,notpassed=0 (0.91 min)
Other local commits for this build/test group: 7a97ef5, e22a629
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Jul 17, 2018
Origin repo remote tracking branch: 'github/master'
Origin repo remote repo URL: 'github = git@github.com:TriBITSPub/TriBITS.git'

At commit:

commit 70af117494c9b68ce99d63baeaac00a452a78a7c
Author:  Roscoe A. Bartlett <rabartl@sandia.gov>
Date:    Tue Jul 17 07:18:03 2018 -0600
Summary: Update filter-packages-list.py so it can handle special ALL_PACKAGES (trilinos#3133)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Jul 17, 2018
…logic (trilinos#3133)

This will allow the Trilinos auto PR tester to use TriBITS logic for determing
what to test.
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Jul 17, 2018

@william76 and @jwillenbring,

I just posted PR #3134 where I made a few small tweaks to the TriBITS code to make it easier to use for the Trilinos use case. Once that PR is merged to 'develop', then you can use it as demonstrated in the script:

get-changed-trilinos-packages.sh (click to expend)
#!/bin/bash -e
#
# Usage:
#
#   get-changed-trilinos-packages.sh <trilinos-dir> <git-commit-from> <git-commit-to>
#
# This script takes in the base Trilinos git repository working directory
# <trilinos-dir> and a range of git commits <git-commit-from> <git-commit-to>
# and then prints to STDOUT a comma-separated list of changed Trilinos
# packages.
#
# As a bi-product of running this script it will create the following files in the current working directory:
#
# * TribitsDumpDepsXmlScript.log: Log file from running TribitsDumpDepsXmlScript.cmake
# * TrilinosPackageDependencies.xml: Generated XML package list and dependencies
# * changed-files.txt: List of files changed between git repo versions

TRILINOS_DIR=$1
GIT_COMMIT_FROM=$2
GIT_COMMIT_TO=$3

ORIG_CWD=$PWD

TRIBITS_DIR=$TRILINOS_DIR/cmake/tribits

# A) Generate the TrilinosPackageDependencies.xml file
cmake \
  -D PROJECT_SOURCE_DIR=$TRILINOS_DIR \
  -D Trilinos_TRIBITS_DIR=$TRIBITS_DIR \
  -D Trilinos_DEPS_XML_OUTPUT_FILE=TrilinosPackageDependencies.xml \
  -P $TRIBITS_DIR/ci_support/TribitsDumpDepsXmlScript.cmake \
  &> TribitsDumpDepsXmlScript.log

# B) Get the set of changed files.
cd $TRILINOS_DIR/
git diff --name-only $GIT_COMMIT_TO ^$GIT_COMMIT_FROM > $ORIG_CWD/changed-files.txt
cd $ORIG_CWD/

# C) Get the unfiltered list of Trilinos packages (including 'ALL_PACKAGES')
CHANGED_PACKAGES_FULL_LIST=\
`$TRIBITS_DIR/ci_support/get-tribits-packages-from-files-list.py --files-list-file=changed-files.txt --deps-xml-file=TrilinosPackageDependencies.xml`
#echo $CHANGED_PACKAGES_FULL_LIST

# D) Filter to get only the PT set of packages and print final list to STDOUT
$TRIBITS_DIR/ci_support/filter-packages-list.py \
  --deps-xml-file=TrilinosPackageDependencies.xml \
  --input-packages-list=$CHANGED_PACKAGES_FULL_LIST \
  --keep-types=PT
.

in order to get the filtered set of changed PT packages.

For example, I just ran this as:

./get-changed-trilinos-packages.sh ~/Trilinos.base/Trilinos ee67bdd 9f757fb

and it returned to STDOUT:

ALL_PACKAGES,Amesos2,Anasazi,AztecOO,Belos,Domi,Epetra,Galeri,Ifpack,Ifpack2,Intrepid,Intrepid2,KokkosKernels,Kokkos,KokkosAlgorithms,KokkosContainers,KokkosCore,MiniTensor,MueLu,PanzerAdaptersSTK,PanzerCore,PanzerDiscFE,PanzerDofMgr,PanzerMiniEM,Phalanx,Piro,ROL,Rythmos,Sacado,SEACAS,SEACASIoss,ShyLU_DDFROSch,ShyLU_NodeTacho,STK,STKSimd,STKUtil,Stokhos,Stratimikos,Teko,Tempus,Teuchos,TeuchosComm,TeuchosCore,TeuchosKokkosCompat,TeuchosParameterList,TpetraClassic,TpetraCore,TrilinosCouplings,Xpetra,Zoltan,Zoltan2

As described, that is a large set of 9K changed files and it completed it in under 30 seconds.

bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Jul 17, 2018
… features (trilinos#3133)

This new script generates the packageEnables.cmake file for a set of changes
tracked by git using built-in TriBITS functionality.  This is the same code
and machinary that has been used by the TriBITS checkin-test.py script and the
TribitsCTestDriverCore.cmake code for almost 10 years.  It is self maintaining
and robust.
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Jul 17, 2018

FYI: As described in #3134 (comment), the PR #3134 now contains the script get-changed-trilinos-packages.sh which completely generates the packageEnables.cmake file. Hopefully that will be easy to plug into the Trilinos auto PR testing infrastructure.

bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 18, 2018
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 18, 2018
I think this CMake -P script now has as good of input checking that a Python
script would.  Therefore, I don't see a need to create a Python script
wrapping the call of this.  This is a pure CMake solution that does not
require any Python so why not.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 18, 2018
…#3133)

This is a more accurate name for what that list contains.  Also, it allows
expanding this script for other types of filter critera, perhaps.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
…rilinos/#3133)

Just documents the scripts being used for the new Trilinos PR package enable
logic (trilinos/Trilinos#3133).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
…rilinos/#3133)

Just documents the scripts being used for the new Trilinos PR package enable
logic (trilinos/Trilinos#3133).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
…trilinos/Trilinos#3133)

I removed some old references to "trilinos" and replaced them with "project".

This is getting ready to add functionality for a TriBITS Project to specalize
logic for if a global rebuild is needed for certain files and directories.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
…trilinos/Trilinos#3133)

I removed some old references to "trilinos" and replaced them with "project".

This is getting ready to add functionality for a TriBITS Project to specalize
logic for if a global rebuild is needed for certain files and directories.  I
added an object for that logic that will be easy to pass around.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 19, 2018
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 20, 2018
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 20, 2018
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 20, 2018
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Aug 10, 2018
Now the Jenkins job output should be able to show what packages will be
enabled even before the ctest -S driver script is run.
@bartlettroscoe bartlettroscoe removed the stage: in progress Work on the issue has started label Aug 13, 2018
@bartlettroscoe
Copy link
Member Author

@william76, why did you close this? PR #3258 has not been merged yet.

@william76
Copy link
Contributor

@bartlettroscoe whoops! I was on the wrong tab and thought I was closing a merged PR, not an issue. I've reopened this one. Thanks for alerting me of the goof ;)

@bartlettroscoe
Copy link
Member Author

I was on the wrong tab and thought I was closing a merged PR, not an issue. I've reopened this one. Thanks for alerting me of the goof ;)

@william76, the merged PRs #3134 and #3218 don't quite complete this due to the problem described above and in #3265. To fix those issues, we need to merge PR #3258, as described in the "Next Action Status" field above.

trilinos-autotester added a commit that referenced this issue Aug 15, 2018
Automatically Merged using Trilinos Pull Request AutoTester
PR Title: Fix change logic in get-changed-trilinos-packages.sh and PullRequestLinuxDriver.sh (#3133)
PR Author: bartlettroscoe
@bartlettroscoe bartlettroscoe added the stage: in review Primary work is completed and now is just waiting for human review and/or test feedback label Aug 15, 2018
@bartlettroscoe
Copy link
Member Author

With the merge of PR #3258 8/14/2018 this should be complete. Next: Watch PRs over next week and ensure that right set of packages are being enabled and then close on 8/21/2018 if all looks good ...

bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Aug 15, 2018
This 'fi' statment got deleted accidentally when resolving a merge conflicit.

This should fix this script for PR testing.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Aug 15, 2018
This 'fi' statement got deleted accidentally when resolving a merge conflicit.

This should fix this script for PR testing.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Aug 15, 2018
This 'fi' statement got deleted accidentally when resolving a merge conflict.

This should fix this script for PR testing.
jwillenbring pushed a commit that referenced this issue Aug 15, 2018
This 'fi' statement got deleted accidentally when resolving a merge conflict.

This should fix this script for PR testing.
@bartlettroscoe
Copy link
Member Author

FYI: There was defect in the PR driver script merged in PR #3258 yesterday which broke the PR testing process. This was fixed in the PR #3302 which was manually merged this morning. It looks like the PR testing system is working again as shown, for example, here.

We will watch the PR builds over the next week to see that everything is going correctly.

@bartlettroscoe
Copy link
Member Author

FYI: A positive data-point ...

The PR #3309 that only changes files for the ATDM Trilinos builds did not result in any Trilinos packages getting enabled as shown in #3309 (comment) and on CDash here which showed:

loading initial cache file /scratch/trilinos/workspace/trilinos-folder/Trilinos_pullrequest_gcc_4.8.4/packageEnables.cmake
-- NOTE: No packages are being enabled!

Success!

NOTE: the first CI iteration shown in #3309 (comment) failed due to a git fetch failure (see #3276 (comment)) which has nothing to do with this story #3133 or any changes to the scripts as part of this.

@bartlettroscoe
Copy link
Member Author

This has been in production for some time and it seems to be working correctly in every case. I think we might want to tweak the set of files under Trilinos/cmake/ and other subdirs that trigger a global build but that can be handled in follow-on issues and PRs.

This has saved the ATDM Trilinos effort a huge amount of time and effort (by not testing all of Trilinos for every little ATDM Trilinos script change).

I am going to close this as complete.

tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
Origin repo remote tracking branch: 'github/master'
Origin repo remote repo URL: 'github = git@github.com:TriBITSPub/TriBITS.git'

At commit:

commit 9b9822f5f2ab7183bdc47eec2193a5fbf6ab83d1
Author:  Roscoe A. Bartlett <rabartl@sandia.gov>
Date:    Wed Aug 8 10:40:15 2018 -0600
Summary: Find ProjectCiFileChangeLogic.py for all projects (trilinos#3133)
tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
With the updated TriBITS just snapshotted, this new unit test shows that the
get-changed-trilinos-packages.sh script does correctly pick up the specialized
Trilinos/cmake/ProjectCiFileChangeLogic.py file.  The previous unit test was
not strong enough to detect that.  Instead, it was likely using the
ProjectCiFileChangeLogic.py file from TribitsExampleProject!

Now the script get-changed-trilinos-packages.sh should correctly ignore files
under cmake/std/atdm/ and yes will force a global build in other cases.
tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
)

This did not impact how the test was run, it just told CTest that these tests
were using 10 MPI processes so it would not have run other tests at the same
time.  So this just wasted a little bit of wall-clock time running the tests
(but not much since these tests run pretty fast).
tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
)

This allows for local testing of the script PullRequestLinuxDriver.sh and
other such use cases.

I also changed some $VAR to ${VAR} to be safe and I fixed a print typo.
tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
trilinos#3133, trilinos#3265)

Now calling this script will select the correct set of file changes and
correct set of enables as part the goals of trilinos#3133.  I have tested this manally
on several different PRs and verified that the correct set of enables are
being selected.

I did a few other things while editing the script:

* The script can now be run on a different scratch Trilinos repo.  This allows
  it to be manually tested without messing up the status of your main Trilinos
  git repo (see details in PR trilinos#3258)

* The Trilinos git repo where the PR branch is merged is now automatically
  updated to the correct state, no matter what its prior state is before it is
  run.  The only requirement is that the 'origin' remote is set correctly.
  None of the other local branches have to be exist or set correctly at all.
  See the new comments at the top.

* Make all formating if statements the same with the same indentation.

* Removed code that was not used or did nothing.

Without this change, the auto PR tester will test more package that is it
supposed to.
tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
Now the Jenkins job output should be able to show what packages will be
enabled even before the ctest -S driver script is run.
tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
This 'fi' statement got deleted accidentally when resolving a merge conflict.

This should fix this script for PR testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework tasks Framework tasks (used internally by Framework team) stage: in review Primary work is completed and now is just waiting for human review and/or test feedback type: enhancement Issue is an enhancement, not a bug
Projects
None yet
Development

No branches or pull requests

2 participants