search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.22k stars 568 forks source link

Framework: STK subpackages being unconditionally enabled in ats2 PR build #11205

Closed bartlettroscoe closed 2 years ago

bartlettroscoe commented 2 years ago

Bug Report

@trilinos/framework

Related issues

Description

For some reason, as shown here, the ats2 PR build on 'vortex':

is unconditionally enabling the following STK subpakages regardless what is changed in the given PR topic-branch:

loading initial cache file /vscratch1/trilinos/jaas/workspace/Trilinos_PR_cuda-10.1.243/generatedPRFragment.cmake
loading initial cache file /vscratch1/trilinos/jaas/workspace/Trilinos_PR_cuda-10.1.243/packageEnables.cmake
-- Setting Trilinos_ENABLE_TrilinosFrameworkTests = ON

...

Explicitly enabled packages on input (by user):  TrilinosFrameworkTests STKUtil STKSimd STKTopology STKMesh STKIO STKSearch STKTransfer STKUnit_tests 9

For that PR #11200, only the package TrilinosFrameworkTests should be enabled.

This causes a PR like #11200 to take upwards of 2 hours to run when it too it should take about 10 seconds.

bartlettroscoe commented 2 years ago

I know that this PR build is supposed to be removed, but it should we have been saying that for months and we suffer during the meantime.

I will put in a PR to fix this.

alanw0 commented 2 years ago

@bartlettroscoe thanks for the report. do you mean you will put in a PR to remove the build that is supposed to be removed? Or to fix this STK bug? If you will fix the STK bug, thanks, otherwise we can do it! :-)

bartlettroscoe commented 2 years ago

@bartlettroscoe thanks for the report. do you mean you will put in a PR to remove the build that is supposed to be removed? Or to fix this STK bug? If you will fix the STK bug, thanks, otherwise we can do it! :-)

@alanw0, PR #11207 makes this clear. None of these configurations should ever be unconditionally be enabling any Trilinos packages, period, stop. (However, unconditionally enabling TPLs is fine and proper.)

alanw0 commented 2 years ago

@bartlettroscoe ok great, yes that's clear and I have approved the PR. thanks!

bartlettroscoe commented 2 years ago

From viewing the recent 'ats2' PR builds on 'vortex' in recent days, it is clear that PR #11207 is working as it should. For example, for PR #11222 just changes Stratimikos and the configure output shows:

loading initial cache file /vscratch1/trilinos/jaas/workspace/Trilinos_PR_cuda-10.1.243/generatedPRFragment.cmake
loading initial cache file /vscratch1/trilinos/jaas/workspace/Trilinos_PR_cuda-10.1.243/packageEnables.cmake
-- Setting Trilinos_ENABLE_Stratimikos = ON

...

Explicitly enabled top-level packages on input (by user):  Stratimikos 1

Explicitly enabled packages on input (by user):  Stratimikos 1

Explicitly disabled top-level packages on input (by user or by default):  Pliris TriKota ShyLU PyTrilinos NewPackage 5

Explicitly disabled packages on input (by user or by default):  KokkosSimd Pliris ShyLU_NodeTacho ShyLU_NodeBasker ShyLU_NodeFastILU SEACASExotec2 TriKota ShyLU_DDCore ShyLU PanzerExprEval PyTrilinos NewPackage 12

Explicitly enabled external packages/TPLs on input (by user):  CUDA CUBLAS CUSOLVER CUSPARSE MPI BLAS LAPACK Boost HDF5 Netcdf BoostLib DLlib 12

...

Enabling all required (and optional since Trilinos_ENABLE_ALL_OPTIONAL_PACKAGES=ON) upstream packages for current set of enabled packages (Trilinos_ENABLE_SECONDARY_TESTED_CODE=OFF) ...

...
-- Setting Trilinos_ENABLE_STKIO=ON because TrilinosCouplings has an optional dependence on STKIO
-- Setting Trilinos_ENABLE_STKMesh=ON because TrilinosCouplings has an optional dependence on STKMesh
...
-- Setting Trilinos_ENABLE_STKUtil=ON because PanzerAdaptersSTK has a required dependence on STKUtil
-- Setting Trilinos_ENABLE_STKTopology=ON because PanzerAdaptersSTK has a required dependence on STKTopology
-- Setting Trilinos_ENABLE_STKSearch=ON because PanzerAdaptersSTK has an optional dependence on STKSearch
...
-- Setting Trilinos_ENABLE_STKExprEval=ON because Percept has a required dependence on STKExprEval
-- Setting Trilinos_ENABLE_STKTransfer=ON because Percept has a required dependence on STKTransfer
-- Setting Trilinos_ENABLE_STKMath=ON because STKExprEval has a required dependence on STKMath
-- Setting Trilinos_ENABLE_STKUnit_test_utils=ON because STKExprEval has a required dependence on STKUnit_test_utils
-- Setting Trilinos_ENABLE_STKNGP_TEST=ON because STKUnit_test_utils has a required dependence on STKNGP_TEST
-- Setting Trilinos_ENABLE_STKBalance=ON because STKUnit_test_utils has an optional dependence on STKBalance
-- Setting Trilinos_ENABLE_STKTools=ON because STKBalance has a required dependence on STKTools
...

with the final set of enables:

Note that before, the STK subpackages (listed under "packages" above) of STKSimd and STKUnit_tests where actually forcibly enabled before the merge of PR #11207.

Also, note that there are PRs like #11238 that does not trigger the enable of any Trilinos packages, and now that STK packages are not unconditionally enabled, those PR iterations complete in just 7 minutes total wall clock time. Before that, PRs like this (e.g. #11151) as shown in the build PR-11151-test-ats2_cuda-10.1.243-gnu-8.3.1-spmpi-rolling_release_static_Volta70_Power9_no-asan_no-complex_no-fpic_mpi_pt_no-rdc_uvm_deprecated-on_no-package-enables-1271 would take over 2 hours to run (when it should have completed in about 1 minute or less). This is a massive improvement.

Closing this as complete.

Trilinos developers: Your welcome :-)