All: Do not auto-enable deprecated packages #12869
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now, TriBITS will not automatically enable any of the to-be-deprecated packages if Trilinos_ENABLE_SECONDARY_TESTED_CODE is OFF. Also default Trilinos_ENABLE_SECONDARY_TESTED_CODE to OFF. Most of the PR builds enable secondary tested code, so this should be fairly safe. Summary: Do NOT automatically enable deprecated packages, even if Trilinos_ENABLE_ALL_OPTIONAL_PACKAGES is ON (default). This is a change for user-facing behavior. If a user was depending on automatic package enablement for explicit behavior, they will need to specify that from now on (e.g., they were enabling Thyra and needed ThyraEpetraAdapters, they will now need to explicitly enable ThyraEpetraAdapters).
sebrowne
requested review from
bartlettroscoe,
rppawlo,
cgcgcg,
mperego and
ccober6
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
This might create mild issues to applications, but it's a good way to further signal that they are using packages that will be eventually removed. |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mperego ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
There was a problem hiding this comment.
Summary: Do NOT automatically enable deprecated packages, even if
Trilinos_ENABLE_ALL_OPTIONAL_PACKAGESisON(default).
I don't see how the changes in this PR has that effect in all cases. For example, if someone changes an ST package then the PR build that has Trilinos_ENABLE_SECONDARY_TESTED_CODE=OFF will still enable those changed ST packages. (It just will not enable the intra-package dependencies for those enabled and changed ST packages.) If you want to have a PR build that disables all deprecated packages, I think you need to hard disable them in the configuration settings for that build.
cmake -DTrilinos_ENABLE_Thyra=ON -DTrilinos_ENABLE_Epetra=ON ..
This enabled Thyra and Epetra, but NOT ThyraEpetraAdapters.This might create mild issues to applications, but it's a good way to further signal that they are using packages that will be eventually removed.
That is going to be confusing to some users and this change definingly breaks backward compatibility for everyone's builds that involve these deprecated packages.
I think it might be worth taking a step back and considering what the goals are here related to these deprecated packages and making the transition for users and developers to be smoother.
| @@ -1,6 +1,6 @@ | |||
| SET(PROJECT_NAME Trilinos) | |||
|
|
|||
| SET(${PROJECT_NAME}_ENABLE_SECONDARY_TESTED_CODE_DEFAULT ON) | |||
| SET(${PROJECT_NAME}_ENABLE_SECONDARY_TESTED_CODE_DEFAULT OFF) | |||
There was a problem hiding this comment.
This is a break in backward compatibility. This will break many users configure scripts.
There was a problem hiding this comment.
Agreed. This is the most dangerous part of this change (and is what the large text in the description is intended to convey).
That's a PR-specific behavior, since we explicitly tell TriBITS to turn on packages that we have identified as being changed (but yes, for PRs that touch ST packages, I think the behavior stays the same. Maybe even for all changes, since we do have some PR builds that turn on ST). For users, the behavior does change in that if they were explicitly enabling superpackages and expecting all subpackages to be enabled. I think it's low impact honestly, and it would have been better practice for them to explicitly enable the desired subpackage in the first place. |
Users that want the old behavior can just set But given this is a break in backward compatibility, should there not be an entry added to the RELEASE_NOTES file? |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
4 similar comments
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
trilinos-autotester
added
the
AT: STALE
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
5 similar comments
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
Status Flag 'Pull Request AutoTester' - Failure: Job Trilinos_PR_gcc-8.3.0 on Jenkins server at https://do.sandia.gov/trilinos-ci Does Not Exist
|
trilinos-autotester
removed
the
AT: RETEST
Looking into this update: Github Issue about this problem: |
achauphan
added
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
|
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
2 similar comments
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
@bartlettroscoe done |
|
After discussion within the leadership team and at the developer's meeting, this was decided to be an acceptable approach. I will craft an email as well notifying users of the behavior change. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
sebrowne
added
the
AT: AUTOMERGE
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
mperego
approved these changes
Apr 18, 2024
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mperego ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
|
Merge on Pull Request# 12869: IS A SUCCESS - Pull Request successfully merged |
trilinos-autotester
removed
the
AT: AUTOMERGE
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@trilinos/framework
Set all packages that will be deprecated next year to
ST(secondary tested).Also default
Trilinos_ENABLE_SECONDARY_TESTED_CODEtoOFF.Now, TriBITS will not automatically enable any of the to-be-deprecated packages if
Trilinos_ENABLE_SECONDARY_TESTED_CODEisOFF.Most of the PR builds enable secondary tested code, so this should be fairly safe.
Summary: Do NOT automatically enable deprecated packages, even if
Trilinos_ENABLE_ALL_OPTIONAL_PACKAGESisON(default).This is a change for user-facing behavior. If a user was depending on automatic package enablement for explicit behavior, they will need to specify that from now on (e.g., they were enabling Thyra and needed ThyraEpetraAdapters, they will now need to explicitly enable ThyraEpetraAdapters).
Tagging members of the operational leadership team, Curt (Product Manager), along with Ross (in case I missed anything about how ST works).
Motivation
Now that a warning message is printed if deprecated packages are enabled (#12746), we want to not auto-enable any of those packages (somewhat contradictory behavior).
In addition, in preparation for behavior as in #12828 , we don't want to present users with quite as much in-your-face warning behavior if they did not mean to enable deprecated packages (combination of
Trilinos_ENABLE_ALL_OPTIONAL_PACKAGESdefaulting toONandTrilinos_ENABLE_SECONDARY_TESTED_CODEdefaulting toON.Testing
I tested the following configurations with the described behavior:
cmake -DTrilinos_ENABLE_Thyra=ON ..This enabled Thyra, but NOT Epetra or ThyraEpetraAdapters.
cmake -DTrilinos_ENABLE_Thyra=ON -DTrilinos_ENABLE_SECONDARY_TESTED_CODE=ON ..This enabled Thyra, Epetra, and ThyraEpetraAdapters
cmake -DTrilinos_ENABLE_Thyra=ON -DTrilinos_ENABLE_Epetra=ON ..This enabled Thyra and Epetra, but NOT ThyraEpetraAdapters.
cmake -DTrilinos_ENABLE_Thyra=ON -DTrilinos_ENABLE_Epetra=ON -DTrilinos_ENABLE_SECONDARY_TESTED_CODE=ON ..This enabled Thyra, Epetra, and ThyraEpetraAdapters.