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.

Sign up for GitHub

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

Jump to bottom

Problems with autotester SHA1s passed into get-changed-trilinos-packages.sh in Trilinos auto-tester #3265

Closed
bartlettroscoe opened this issue Aug 9, 2018 · 3 comments
Closed

Problems with autotester SHA1s passed into get-changed-trilinos-packages.sh in Trilinos auto-tester #3265

bartlettroscoe opened this issue Aug 9, 2018 · 3 comments
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: bug The primary issue is a bug in Trilinos code or tests
Milestone
Improve productiv...

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Aug 9, 2018 •
edited
Loading

@trilinos/framework, @william76

Next Action Status

PR #3258 merged on 8/14/2018 which should fix this and this seems to be fixed as noted in closed #3133.

Description

There seems to be a problem with the Trilinos autotester in passing in the right range of commits to the get-changed-trilinos-packages.sh script (see #3133 and #3218). The evidence for this is the PR testing iteration for PR #3260 which only changes files under the cmake/std/atdm/ directory yet it triggered the enable of several Trilinos packages as shown in this PR iteration this morning which shows:

loading initial cache file /scratch/trilinos/workspace/trilinos-folder/Trilinos_pullrequest_gcc_4.8.4/packageEnables.cmake
-- Setting Trilinos_ENABLE_ALL_PACKAGES = ON
-- Setting Trilinos_ENABLE_Belos = ON
-- Setting Trilinos_ENABLE_Ifpack2 = ON
-- Setting Trilinos_ENABLE_Piro = ON
-- Setting Trilinos_ENABLE_ROL = ON
-- Setting Trilinos_ENABLE_TpetraCore = ON

My guess is that the Python script is passing in the git SHA1 for the tip of the 'develop' branch instead of the base commit that the topic branch is created off of.
@bartlettroscoe bartlettroscoe added type: bug The primary issue is a bug in Trilinos code or tests Framework tasks Framework tasks (used internally by Framework team) labels Aug 9, 2018
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Aug 9, 2018 •
edited
Loading

@trilinos/framework,

Looking at:

showing the command:

changed_packages_app=Trilinos/commonTools/framework/get-changed-trilinos-packages.sh
${changed_packages_app} ${TRILINOS_SOURCE_SHA} ${TRILINOS_TARGET_SHA} packageEnables.cmake

is ${TRILINOS_SOURCE_SHA} the tip of the 'develop' branch? If it is, this is going to give you the wrong set of difference files.

Looking at the SHA1s for this PR iteration of #3260 reported here, they are:

If you run the script get-changed-trilinos-packages.sh using these SHA1s you get:

$ ./Trilinos/commonTools/framework/get-changed-trilinos-packages.sh \
  45adc90 c96c475 enablePackages.cmake \
  &> get-changed-trilinos-packages.out

$ cmake -P enablePackages.cmake
-- Setting Trilinos_ENABLE_ALL_PACKAGES = ON
-- Setting Trilinos_ENABLE_Belos = ON
-- Setting Trilinos_ENABLE_Ifpack2 = ON
-- Setting Trilinos_ENABLE_Piro = ON
-- Setting Trilinos_ENABLE_ROL = ON
-- Setting Trilinos_ENABLE_TpetraCore = ON

That is not good. So the SHA1 TRILINOS_SOURCE_SHA = c96c475 is the tip of the branch for my PR #3260 where the top of the git graph is:

Graph for TRILINOS_SOURCE_SHA c96c475

* c96c475 "Change ctest drivers from cuda-xxx to cuda-9.2-xxx (TRIL-215)" <[email protected]> [Wed Aug 8 15:11:57 2018 -0600] (15 hours ago)
* 5ecd0f5 "Adjust for all-at-once build being the default (TRIL-198)" <[email protected]> [Wed Aug 8 13:50:12 2018 -0600] (17 hours ago)
* a3363fa "CUDA 9.2 build on white/ride and remove GPU KOKKOS_ARCH for GNU build (TRIL-215)" <[email protected]> [Wed Aug 8 08:35:49 2018 -0600] (20 hours ago)
* e16895a "Disable test Piro_MatrixFreeDecorator_UnitTests_MPI_4 in Intel builds (#2474) (#3252)" <[email protected]> [Tue Aug 7 22:17:52 2018 -0400] (34 hours ago)
...

So what is the other SHA1 TRILINOS_TARGET_SHA = 45adc90?

Graph for TRILINOS_TARGET_SHA = 45adc90:

*   45adc90 "Merge pull request #3249 from trilinos/Fix-3235" <[email protected]> [Wed Aug 8 12:34:59 2018 -0600] (18 hours ago)
|\  
| * fb8dc4a "Tpetra,Belos: Fix #3235, & add unit test" <[email protected]> [Tue Aug 7 12:54:26 2018 -0600] (2 days ago)
* | f93a0cb "Consolidate disables for all builds on 'mutrino' (#3183) (#3251)" <[email protected]> [Wed Aug 8 13:56:51 2018 -0400] (19 hours ago)
* |   74d195d "Merge pull request #3243 from jmgate/ifpack2-remove-unused-parameter-warnings" <[email protected]> [Wed Aug 8 09:40:37 2018 -0600] (21 hours ago)
|\ \  
| * | b63d73b "Ifpack2:  Remove Unused Parameter Warnings" <[email protected]> [Mon Aug 6 13:32:31 2018 -0600] (2 days ago)
| |/  
* | f9de01a "Piro: use template keyword when calling memeber function (fix issue #3157)" <[email protected]> [Tue Aug 7 09:33:20 2018 -0700] (22 hours ago)
* | 09e26b1 "ROL: fix typo in ThyraProductME_Objective constructor, '==' --> '='." <[email protected]> [Tue Aug 7 09:31:50 2018 -0700] (22 hours ago)
* | e16895a "Disable test Piro_MatrixFreeDecorator_UnitTests_MPI_4 in Intel builds (#2474) (#3252)" <[email protected]> [Tue Aug 7 22:17:52 2018 -0400] (34 hours ago)
...

So here we see the problem. These two graphs are divergent. TRILINOS_TARGET_SHA= 45adc90 must be the tip of the 'develop' branch that will be merged intoTRILINOS_SOURCE_SHA` = c96c475 in order to do testing. These two branches share the common commit e16895a:

e16895a "Disable test Piro_MatrixFreeDecorator_UnitTests_MPI_4 in Intel builds (#2474) (#3252)" <[email protected]> [Tue Aug 7 22:17:52 2018 -0400] (34 hours ago)

The way to fix this is to find the commit on the topic branch that is already on 'develop' The way you do that is with:

TRILINOS_MERGE_BASE_SHA1=`git merge-base ${TRILINOS_SOURCE_SHA} ${TRILINOS_TARGET_SHA}`

For this case, this returns:

$ git merge-base 45adc90 c96c475
e16895a3f7a7d7213e47a1fc6ce7d5dd71043b25

That is the correct SHA1. Using that SHA1, we get the right set of file changes:

$ ./Trilinos/commonTools/framework/get-changed-trilinos-packages.sh \
  e16895 c96c475 enablePackages.cmake \
  &> get-changed-trilinos-packages.out

$ cat changed-files.txt 
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-debug-panzer.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-debug-pt.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-debug.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-opt-Pascal60.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-opt-panzer-Pascal60.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-opt-panzer.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-9.2-opt.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-debug-all-at-once.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-debug-panzer.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-debug-pt-all-at-once.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-debug.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-opt-Pascal60.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-opt-panzer-Pascal60.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-opt-panzer.sh
cmake/ctest/drivers/atdm/ride/drivers/Trilinos-atdm-white-ride-cuda-opt.sh
cmake/std/atdm/ride/all_supported_builds.sh
cmake/std/atdm/ride/environment.sh
cmake/std/atdm/ride/tweaks/CUDA-9.2-DEBUG-CUDA-POWER8-KEPLER37.cmake
cmake/std/atdm/ride/tweaks/CUDA-9.2-RELEASE-CUDA-POWER8-KEPLER37.cmake
cmake/std/atdm/ride/tweaks/CUDA-DEBUG-CUDA-POWER8-KEPLER37.cmake
cmake/std/atdm/ride/tweaks/CUDA-RELEASE-CUDA-POWER8-KEPLER37.cmake
cmake/std/atdm/ride/tweaks/GNU-DEBUG-OPENMP-POWER8-KEPLER37.cmake
cmake/std/atdm/ride/tweaks/GNU-DEBUG-OPENMP-POWER8.cmake
cmake/std/atdm/ride/tweaks/GNU-RELEASE-OPENMP-POWER8-KEPLER37.cmake
cmake/std/atdm/ride/tweaks/GNU-RELEASE-OPENMP-POWER8.cmake
cmake/std/atdm/utils/set_build_options.sh

That is the correct set of changed files.

I will update the branch in PR #3258 to fix this.

bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Aug 9, 2018
@bartlettroscoe
Fix SHA1s passed to get-changed-trilinos-packages.sh (trilinos#3133, t…
b2cccbf 
…rilinos#3265)

You need to find the common base commit between the soruce and the target
branch and then use that diff (see trilinos#3265).

Without this change, the auto PR tester will test more package that is it
supposed to.
@bartlettroscoe bartlettroscoe mentioned this issue Aug 9, 2018
Merged
3 tasks
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Aug 10, 2018
@bartlettroscoe
Fix calling of get-changed-trilinos-packages.sh and other improvements (
6aea0bc 
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.
@bartlettroscoe bartlettroscoe mentioned this issue Aug 13, 2018
Closed
@bartlettroscoe
Copy link
Member Author

With PR #3258 merged (thank @jwillenbring), this issue should now be resolved. Now we just need to watch the next few PRs over the coming days and make sure that the right packages are being enabled and tested.

@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

All seems good as noted in closed #3133.

Closing as complete.

tjfulle pushed a commit to tjfulle/Trilinos that referenced this issue Dec 6, 2018
@bartlettroscoe @tjfulle
Fix calling of get-changed-trilinos-packages.sh and other improvements (
5cc45a6 
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.
@trilinos-autotester trilinos-autotester mentioned this issue Apr 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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: bug The primary issue is a bug in Trilinos code or tests
Projects
None yet
Milestone
Improve productivity, stability, and ...
Development

No branches or pull requests
1 participant
@bartlettroscoe