Issue4258 remove teuchos sdmatrix inheritance #4259
Conversation
…rom Object)
From Mike Eldred:
This patch removes Teuchos::Object from inheritance chains in all Teuchos::
SerialDense* classes. This has two primary effects:
1) eliminates overhead from the Teuchos::Object data attributes, especially
the std::string label_.
2) removes the operator<< (std::ostream& os, const Teuchos::Object& obj) that
has caused problems for us defining our own operators for Teuchos types.
*** Important note: the latter change may induce requirements on client code,
*** if they were reliant on this ostream operator. For Dakota, this is a good
*** thing as it provides the opportunity to replace an inconsistently formatted
*** operator (that has previously been in the way) with a tailored one.
…rom Object) - fix client code in Trilinos needed to remove SerialDense matrix inheritance Teuchos::Object These were identified and fixed using the GCC 4.8.4 PR test setup in cmake/std/PullRequestLinuxGCC4.8.4TestingSettings.cmake with Teuchos explicitly enabled and the HDF5 TPL disabled.
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
@trilinos/teuchos |
| TEUCHOS_TEST_FOR_EXCEPTION(info != 0,std::runtime_error, | ||
| "New <Y1,X1> not s.p.d.: couldn't computed Cholesky: info == " << info | ||
| << "\nyTx1: \n" << yTx1); | ||
| std::ostringstream msg("New <Y1,X1> not s.p.d.: couldn't computed Cholesky: info == "); |
There was a problem hiding this comment.
Hm, I'd rather not build the std::ostringstream each time if we only use it when info is nonzero. How about this?
if (info != 0) {
// build the std::ostringstream
yTx1.print (msg);
TEUCHOS_TEST_FOR_EXCEPTION(true, std::runtime_error, msg);
}There was a problem hiding this comment.
I realize now that the above code is in a test, so I'm less worried about it.
| TEUCHOS_TEST_FOR_EXCEPTION(info != 0,std::runtime_error, | ||
| "New <Y2,X2> not s.p.d.: couldn't computed Cholesky: info == " << info | ||
| << "\nyTx2: \n" << yTx2); | ||
| msg.str("New <Y2,X2> not s.p.d.: couldn't computed Cholesky: info == "); |
There was a problem hiding this comment.
I believe the suggestion to implement the operator<< is exactly what Mike did in Dakota. If I added one for Teuchos::SerialDenseMatrix&, that would remove the need to touch the other Trilinos packages but in a way that has proven overly prescriptive for Dakota. Is there a more attractive approach, eg a virtual operator<< ?
There was a problem hiding this comment.
Also, is there a way to add Mike (Eldred) as a watcher of this PR?
There was a problem hiding this comment.
@rhoope You could just do an @-mention of this GitHub account name here. That should notify him.
There was a problem hiding this comment.
@rhoope wrote:
Is there a more attractive approach, e.g., a virtual
operator<<?
Is the issue that you would like Dakota to be able to override any existing print behavior of Teuchos::SerialDenseMatrix, but still be able to use operator<< everywhere?
It's hard to do that when using an operator. Argument-dependent lookup means that you can't use the same input types (std::ostream&, const SDM&) and still get a different operator<<. However, you could achieve this effect in Dakota with a little wrapper that changes SDM into a different type. This is analogous to "function advising" in some other languages. Here is an example:
// Implementation detail; not for users
template<class OrdinalType, class ScalarType>
struct SerialDenseMatrixOutputWrapper {
const SerialDenseMatrix<OrdinalType, ScalarType>& A;
};
// this is for Dakota developers
template<class OrdinalType, class ScalarType>
SerialDenseMatrixOutputWrapper<OrdinalType, ScalarType>
wrapSDM (const SerialDenseMatrix<OrdinalType, ScalarType>& A)
{
return {A};
}
// Dakota would define this
template<class OrdinalType, class ScalarType>
std::ostream& operator<< (std::ostream& out, const SerialDenseMatrixOutputWrapper<OrdinalType, ScalarType>& A)
{
// ... print A however Dakota likes to print it ...
return out;
}You would use the class this way:
SerialDenseMatrix<int, double> A ( /* whatever goes here */ );
// ... do stuff with A ...
std::cout << wrapSDM (A) << std::endl;There was a problem hiding this comment.
@mseldre wrote:
Simply dropping the inheritance from Object nicely accomplishes that, and we noticed your comments in the base about the need to deprecate and retire this legacy code.
I very much appreciate your contributions here -- thanks!
This clash was historical and may have been due to "using namespace Teuchos" pollution somewhere in the header dependencies -- not sure.
Argument-dependent lookup (ADL) would normally pick up Teuchos::operator<<, since it is in the same namespace as one of its arguments. ADL would not normally pick up an overloaded Dakota::operator<<, even inside code in the Dakota namespace. This is true even without using namespace Teuchos; etc.
So my recommendation would be to delete Object and allow Teuchos clients to provide their own customized insertion/extraction operators.
I'm OK with this personally, but it does break backwards compatibility for downstream applications. It's very likely that you won't be able to test all possible combinations of build options even within Trilinos, so this may break some Trilinos packages. It may also break downstream apps.
Trilinos hasn't had an official minor release (!) since 12.12, around three years ago (!!!). This means that all bets are off in terms of deprecation and removal policy. (Feel free to express your opinions about that to @maherou. I have no control over it.) Lack of policy mainly just means that you'll have to manage the angry-grams from various apps if their builds break. If you're OK with that, we can merge this and file a separate issue where you will be listed as the point of contact for explaining how to work around any build issues that arise. It's no big deal, but I just want to make sure we document why we did this, that it does break backwards compatibility, and how to fix it. Is that OK?
There was a problem hiding this comment.
Thanks for the pointer about ADL -- that explains what I was observing and, IMO, also strengthens the argument for not providing these operators.
I can't speak to internal Trilinos policy for deprecation or release management, but seems there needs to be some mechanism to move forward. From the Dakota side, we would of course like to see our local changes to Teuchos propagate upstream so that users can build with either our version or their own custom version of Trilinos, with minimal/no inconsistencies. I am fine with delaying this patch until the appropriate time when deprecations can be announced and managed -- in the meantime, we can get by with our local version. In terms of being the POC for Trilinos angry-grams, yes if that's necessary in the short term.
On a semi-related note, I was also eyeing the CompObject data that appears to go unused by SerialDense*. I added some (redundant) protection around the computation of FLOP increments since the flop accumulator that accepts them seems to always be NULL based on ctor initializers. It would be easy to manage ctor paths with and without meaningful FLOP counters, but it seems to currently be an afterthought. Am I missing context?
There was a problem hiding this comment.
@mseldre Thanks for the reply! In brief, I would say, go for it! :-D (Ditto for the CompObject changes, though please in a separate PR :-) .) If you're OK with getting a few questions about how to fix build errors, I'll be happy to approve this PR.
Long answer: @ikalash, @rrdrake, @theguruat12, @micahahoward, @rppawlo, or @trilinos/framework may wish to chime in, but I would summarize the de facto "Trilinos versioning" policy for many internal applications as follows:
- App either forks Trilinos, or picks a Trilinos "master" branch commit.
- App defines its own acceptance testing for Trilinos.
- When an app wants to update its Trilinos, it uses acceptance tests to decide when to merge / pull. Many apps facilitate this by testing the app nightly against current master Trilinos.
This implies that apps define their own "Trilinos versions." Apps actually use this model. That, along with open access to Trilinos' repo on GitHub, likely explains the lack of enthusiasm for official Trilinos releases. I've gotten about as many complains about deprecation warnings ("hey, we can't build warning-free") as I've gotten about actual interface changes, so it's likely less effort just to break the build than it is to do an official deprecation (how long? who knows, since no official releases!).
What's missing from this model is a way for an app to work correctly with multiple Trilinos versions at once. A few packages have added this feature themselves. For example, Kokkos has a CMake option and macro to help manage deprecation. Tpetra just recently added one. It may make sense for Teuchos to add this as well, but Teuchos is a big utility package without a single focus, so it may be hard to enforce the deprecation model and decide when to remove deprecated stuff.
Anyway, I hope the long explanation helps!
There was a problem hiding this comment.
Thanks for the explanation. Approving the PR sounds good to me. Happy to help with suggestions/examples of supplying ostream insertion operators for Vector,Matrix,SymMatrix.
There was a problem hiding this comment.
I just pushed some mods to make GCC 7.3.0 happy.
mhoemmen
added
the
pkg: Teuchos
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
mhoemmen
added
the
AT: WIP
mhoemmen
removed
the
AT: WIP
There was a problem hiding this comment.
For readers just coming to this discussion: This change breaks backwards compatibility, but may improve performance. There are a couple issues remaining, though I think they are harmless:
-
ROL does its development in a separate repository, so these changes will be clobbered on next ROL promotion. On the other hand, the ROL developers will need to pull in these changes if they want to be able to build on their next Trilinos promotion ;-) . It would be nicer if you could put the ROL changes in separate commits, but I don't want to make you jump through more hoops.
-
Why not have
printreturnstd::ostream&instead ofvoid? That way, you could write code like this:cout << "Matrix A: " << A.print (cout) << endl;.
|
@trilinos/rol @trilinos/anasazi @trilinos/belos |
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]! |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: rhoope |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 2317 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 2118 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 608 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0 # 227 (click to expand) |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: rhoope |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 2318 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 2119 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 609 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0 # 228 (click to expand) |
|
This breaks Anasazi’s build. Not sure why your texting didn’t pick this up — maybe your script didn’t enable Anasazi. |
…rom Object) - mods to make gcc 7.3.0 compile
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]! |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: rhoope |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]! |
|
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... |
|
Y'all have fun! 👍 |
|
Hi Ross, |
@trilinos/ (Several affected)
Description
See Issue #4258 for details
Motivation and Context
Closes Issue #4258
How Has This Been Tested?
Yes, using GCC 4.8.4 PR setup with Teuchos enabledand HDF5 disabled (includes all forward and optional packages affected).
Checklist