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

Can we turn off GlobalOrdinal = int in all Tpetra stack packages? #74

Closed
mhoemmen opened this issue Jan 8, 2016 · 32 comments
Closed

Can we turn off GlobalOrdinal = int in all Tpetra stack packages? #74

mhoemmen opened this issue Jan 8, 2016 · 32 comments
Labels
Framework tasks Framework tasks (used internally by Framework team) pkg: Tpetra

Comments

@mhoemmen
Copy link
Contributor

mhoemmen commented Jan 8, 2016

Next action Status:

Next: Better scope this story and create plan to exclude GO=int from the explicit instantations of Trilinos when possible (i.e. when Epetra is disabled, see #313).

Blocked By: #313

Description:

It would be nice to be able to build the Tpetra enabled templated stack with just GO = long ling int and skip GO = int. This story will investigate this and then, if possible, work up a plan to make this happen.

Tasks:

???

@aprokop
Copy link
Contributor

aprokop commented Jan 8, 2016

I have a nightly testing for that (http://testing.sandia.gov/cdash/buildSummary.php?buildid=2293303). Linear stack up till MueLu builds when ETI=ON and Tpetra's GO=int is off.

@mhoemmen
Copy link
Contributor Author

Got the following report:

"I pulled a new version of Trilinos and when I build with Trilinos_ENABLE_EXPLICIT_INSTANTIATION:BOOL=OFF and Tpetra_INST_INT_INT:BOOL=OFF, I get a error ... that it cannot find Amesos2_MultiVecAdapter_decl.hpp."

Here is the error:

In file included from .../TrilinosInstall/include/Amesos2_MultiVecAdapter.hpp:47:0,
from .../TrilinosInstall/include/Amesos2_Factory.hpp:91,
from .../TrilinosInstall/include/Amesos2.hpp:45,
from .../TrilinosInstall/include/MueLu_Amesos2Smoother_def.hpp:56,
from .../TrilinosInstall/include/MueLu_Amesos2Smoother.hpp:2,
from .../TrilinosInstall/include/MueLu_DirectSolver_def.hpp:57,
from .../TrilinosInstall/include/MueLu_DirectSolver.hpp:2,
from .../TrilinosInstall/include/MueLu_FactoryManager_def.hpp:58,
from .../TrilinosInstall/include/MueLu_FactoryManager.hpp:2,
from .../TrilinosInstall/include/MueLu_Hierarchy_decl.hpp:67,
from .../TrilinosInstall/include/MueLu_Hierarchy.hpp:1,
from $APP_FILE:$LINE_NUMBER:
.../TrilinosInstall/include/Amesos2_MultiVecAdapter_decl.hpp:316:52: fatal error: Amesos2_EpetraMultiVecAdapter_decl.hpp: No such file or directory
compilation terminated.

The reporter then said: "It looks like this header file does not get installed. Looking in packages/amesos2/src/CMakeLists.txt I see:"

IF (Tpetra_INST_INT_INT)
IF (${PACKAGE_NAME}_ENABLE_Epetra)
  APPEND_SET(HEADERS
    Amesos2_EpetraRowMatrix_AbstractMatrixAdapter.hpp
    Amesos2_EpetraRowMatrix_AbstractMatrixAdapter_decl.hpp
    Amesos2_EpetraRowMatrix_AbstractMatrixAdapter_def.hpp

"Did something in Muelu get changed to require this?"

@mhoemmen
Copy link
Contributor Author

Paging @trilinos/amesos2 and @trilinos/muelu developers

@mhoemmen
Copy link
Contributor Author

Oh wait... Is it necessary to disable Amesos2's Epetra support in order to disable GlobalOrdinal = int? If that's the case, please let me know and I'll pass this along to the customer.

@mhoemmen
Copy link
Contributor Author

Btw here is a Dashboard build with GlobalOrdinal=int disabled, but ETI is ON in this build:

http://testing.sandia.gov/cdash/buildSummary.php?buildid=2293303

@mhoemmen
Copy link
Contributor Author

I'm looking at amesos2/src/Amesos2_MultiVecAdapter_decl.hpp, lines 315-317. It protects the include of Amesos2_EpetraMultiVecAdapter_decl.hpp with #ifdef HAVE_AMESOS2_EPETRA ... #endif. I'll ask the customer in question to send their automatically generated Amesos2_config.h header file.

@mhoemmen
Copy link
Contributor Author

I was able to build MueLu tests and examples with GO=int OFF, ETI OFF, and static libraries. Amesos2_config.h says that HAVE_AMESOS2_EPETRA is defined.

@mhoemmen
Copy link
Contributor Author

I fixed a minor build error in @trilinos/xpetra (Xpetra::TpetraMultiVector had a missing default constructor). About to push.

mhoemmen pushed a commit that referenced this issue Jan 19, 2016
@trilinos/xpetra Xpetra::TpetraMultiVector was missing a default
constructor (takes no arguments).  This caused a build error when

  - Explicit template instantiation is OFF
  - Tpetra_INST_INT_INT=OFF (GlobalOrdinal=int is disabled)

This commit adds the missing default constructor.  Xpetra now builds.
This helps address Issue #74.

Build/Test Cases Summary
Enabled Packages: Xpetra
Disabled Packages: FEI,STK,PyTrilinos,NOX,Intrepid,Intrepid2,ROL,Teko,Piro
0) MPI_DEBUG => Test case MPI_DEBUG was not run! => Does not affect push readiness! (-1.00 min)
1) SERIAL_RELEASE => Test case SERIAL_RELEASE was not run! => Does not affect push readiness! (-1.00 min)
2) MPI_DEBUG_COMPLEX => passed: passed=16,notpassed=0 (1.63 min)
3) SERIAL_RELEASE => passed: passed=16,notpassed=0 (2.12 min)
@jdbooth
Copy link
Contributor

jdbooth commented Jan 19, 2016

Hi Mark. @mhoemmen @trilinos/amesos2 @trilinos/muelu

Amesos2 does not support Epetra with GO=int OFF.

First, It is hard to change all the mappings because of how they get their GO type and we check at every stage that the GO type matches (Note, we use tpetra maps once we get the Epetra matrix.... I didn't make it that way just).
Second, It does not make sense to me at least why someone would be using Epetra (GO=int) if they turn ETI GO=int off. Maybe I don't see the case.

I even throw a configure warning to this extent.
IF (NOT Tpetra_INST_INT_INT)
MESSAGE(WARNING "Amesos2 will not provide Epetra Matrix Support with Tp
etra_INST_INT_INT OFF.**
**")
MESSAGE(WARNING "
Amesos2 will not provide MUMPS Support with Tpetra_INS
T_INT_INT OFF.**
**")
ENDIF ()

I did miss the HAVE_AMESOS2_EPETRA in the Amesos2_config.h, but that is derived from the CMAKE options. Also, made more sense to me to not include files we are not using than to include a bunch of empty header files.

If there is a a needed for either the empty header files or support of Epetra in Amesos2 with GO=int off, then please contact me offline and we can have a talk about it.

@mhoemmen
Copy link
Contributor Author

Hi @jdbooth -- just wanted to make clear that I wasn't asking for Amesos2 to support Epetra with GO=int off :-) I was merely giving the current status. In particular, I wanted to make sure Mike understood the consequences of requiring GO=int to be off by default.

I wasn't actually able to replicate the customer's build issue. They have a different build process, and forgetting to update that might be the cause. That was why I asked for their Amesos2_config.h file settings.

@maherou
Copy link
Member

maherou commented Jan 20, 2016

Understood. Thanks.

On Jan 20, 2016, at 1:58 PM, Mark Hoemmen [email protected] wrote:

Hi @jdbooth -- just wanted to make clear that I wasn't asking for Amesos2 to support Epetra with GO=int off :-) I was merely giving the current status. In particular, I wanted to make sure Mike understood the consequences of requiring GO=int to be off by default.


Reply to this email directly or view it on GitHub.

@mwglass
Copy link
Contributor

mwglass commented Jan 20, 2016

Just to clarify. Our build process for this issue is not different. I am using the Trilinos cmake build system to build and then install the appropriate packages for Sierra. We then build Sierra using this installed version of Trilinos. What I trying to do here is test a build of Trilinos with ETI turned on and minimizing the number of template instantiations. Mark had been trying to eleminate having to have GO=INT and I was trying to test that configuration. By the sounds of things we (SIERRA) will still have to specify GO=INT when turning on ETI as long as we are still using epetra.

@mhoemmen
Copy link
Contributor Author

@mwglass My hope was that it would be possible to build Epetra and its stack, build Tpetra and its stack, yet turn off GO=int. This might possibly require turning off Epetra support in Amesos2 and Xpetra (and packages that use Xpetra, like MueLu and Zoltan2). If this is not the case, then that's a bug and we should fix it. I'm sorry that I've been away and haven't been on the job full time to help with this. I will experiment with installation testing on my own and see if I can replicate this issue.

@mhoemmen
Copy link
Contributor Author

@bmpersc @tawiesn @aprokop Any thoughts on this btw?

@aprokop
Copy link
Contributor

aprokop commented Jan 22, 2016

MueLu's ETI is done the following way:

  • if Epetra in MueLu is disabled, follow Tpetra's instantiations
  • if Epetra in MueLu is enabled, we instantiate on Tpetra's instantiations + <double,int,int,EpetraNode> where Epetra node is either serial or OpenMp depending on whether it is enabled in Epetra.

One thing to notice, though, is that I'm using "Epetra in MueLu". It is possible to run the configuration with Epetra in Trilinos enabled but Epetra in MueLu disabled (through MueLu_ENABLE_Epetra=OFF).

Regarding Xpetra, it does not instantiate, but may provide extra stubs or partial specializations depending on the Epetra/Tpetra configurations.

@tawiesn
Copy link
Contributor

tawiesn commented Jan 22, 2016

Here is what Xpetra is doing: You can build Epetra (or Epetra64) and Tpetra (but with GO=int turned off). Xpetra will then be able to run both with Epetra (or Epetra64) and Tpetra. That is you can create, e.g. an Xpetra::Map using Xpetra::MapFactory < LO,GO,Node > ::Build(Xpetra::UseEpetra,...) (for GO=int if Epetra and GO=long long if Epetra64, all other will throw an exception), as well as, e.g., Xpetra::MapFactory < LO,GO,Node > ::Build(Xpetra::UseTpetra,...). The latter will throw an exception for GO=int (as GO=int is disabled in Tpetra). It will return a valid Map for all enabled GO's that are enabled for Tpetra.

@jhux2
Copy link
Member

jhux2 commented Jan 22, 2016

Just to clarify. Our build process for this issue is not different. I am using the Trilinos cmake build system to build and then install the appropriate packages for Sierra. We then build Sierra using this installed version of Trilinos. What I trying to do here is test a build of Trilinos with ETI turned on and minimizing the number of template instantiations. Mark had been trying to eleminate having to have GO=INT and I was trying to test that configuration. By the sounds of things we (SIERRA) will still have to specify GO=INT when turning on ETI as long as we are still using epetra.

@mwglass Sierra should now be able to do the following now: enable MueLu, Epetra and Tpetra, use ETI, and disable Tpetra GO=int. I just want to mention that Andrey and Tobias have put in a great deal of effort to get this working in Xpetra and MueLu.

@mhoemmen
Copy link
Contributor Author

I just want to mention that Andrey and Tobias have put in a great deal of effort to get this working in Xpetra and MueLu.

Let me emphasize this again :-D I'm super grateful for all the work the MueLu and Amesos2 devs have put into this. I know it's unfun and it takes time away from other work, but hopefully this will help Sierra and other apps quite a bit.

@mhoemmen
Copy link
Contributor Author

Sierra should now be able to do the following now: enable MueLu, Epetra and Tpetra, use ETI, and disable Tpetra GO=int.

Just to clarify: Does Sierra have to set MueLu_ENABLE_Epetra=OFF or Amesos2_ENABLE_Epetra=OFF in order to do this?

@mhoemmen mhoemmen added pkg: Tpetra Framework tasks Framework tasks (used internally by Framework team) labels Jan 24, 2016
@bartlettroscoe
Copy link
Member

It would be really great to turn of the 'int' GO instantiation. I think this would allow just one instanitation for every piece of code. Any more progress on this?

@tawiesn
Copy link
Contributor

tawiesn commented Mar 15, 2016

MueLu compiles and the most important tests run when GO=int is turned off in Tpetra. I'm currently preparing a couple of more patches for the remaining tests/examples which are currently not compiling. They are all guarded by the MUELU_BROKEN_TESTS flag. So, you should not be affected by compilation errors. You can see them in the MueLu dashboard in the Experimental section.

@mhoemmen
Copy link
Contributor Author

MueLu compiles and the most important tests run when GO=int is turned off in Tpetra.

@tawiesn Awesome!!! :-D

@bartlettroscoe
Copy link
Member

Can we change:

GLOBAL_SET(HAVE_TPETRA_INST_INT_INT_DEFAULT ON)

to

IF (${PROJECT_NAME}_ENABLE_Epetra)
  GLOBAL_SET(HAVE_TPETRA_INST_INT_INT_DEFAULT ON)
ELSE()
  GLOBAL_SET(HAVE_TPETRA_INST_INT_INT_DEFAULT OFF)
ENDIF()

in tpetra/CMakeLists.txt? If you don't enabled Epetra, you don't need an 'int' GO right?

That would result in only one instantiation (i.e. GO=long long int, LO=int, SCALAR=double) when you disabled float and complex.

@mhoemmen
Copy link
Contributor Author

@bartlettroscoe I'm in favor of GO = long long int being the one default GO option, but the downstream packages also need to make sure that most of their tests work for that case. Otherwise, setting this as the default will disable a lot of tests, hindering detection of regressions.

@jhux2
Copy link
Member

jhux2 commented Mar 18, 2016

I would like to hear from @spdomin and @mwglass.

@jhux2
Copy link
Member

jhux2 commented Mar 18, 2016

I would like to hear from @spdomin and @mwglass.

Oops, didn't realize mwglass had already chimed in.

@mhoemmen
Copy link
Contributor Author

mhoemmen commented Aug 7, 2017

I think we've done pretty well with this issue, so I'm closing it. Thanks to all who helped!

@mhoemmen mhoemmen closed this as completed Aug 7, 2017
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra tpetra/core/test/BugTests/BugTests.cpp only built its
tests if GlobalOrdinal=int was enabled.  This was unnecessarily
restrictive, given that the bugs in question (Bugs 5129 and 5072 in
the Mozilla bug tracking system, before Trilinos switched to using
GitHub issue tracking) didn't appear to have anything to do with a
specific GlobalOrdinal type.  Furthermore, we want to have full
testing for the case where GlobalOrdinal=int is off.  See e.g.,

trilinos#74

This commit generalizes the tests to build and run for the default
LocalOrdinal and GlobalOrdinal types.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra Tpetra::Map's "ProblematicLookup" test depended
arbitrarily on LocalOrdinal=int and GlobalOrdinal=int.  This commit
changes the test to use the default LocalOrdinal and GlobalOrdinal
types.  See trilinos#74.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra Tpetra's "Block" test depended arbitrarily on
LocalOrdinal=int and GlobalOrdinal=int.  This commit changes the test
to use the default LocalOrdinal and GlobalOrdinal types.  See trilinos#74.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra tpetra/core/test/BugTests/BugTests.cpp only built its
tests if GlobalOrdinal=int was enabled.  This was unnecessarily
restrictive, given that the bugs in question (Bugs 5129 and 5072 in
the Mozilla bug tracking system, before Trilinos switched to using
GitHub issue tracking) didn't appear to have anything to do with a
specific GlobalOrdinal type.  Furthermore, we want to have full
testing for the case where GlobalOrdinal=int is off.  See e.g.,

trilinos#74

This commit generalizes the tests to build and run for the default
LocalOrdinal and GlobalOrdinal types.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra Tpetra::Map's "ProblematicLookup" test depended
arbitrarily on LocalOrdinal=int and GlobalOrdinal=int.  This commit
changes the test to use the default LocalOrdinal and GlobalOrdinal
types.  See trilinos#74.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra Tpetra's "Block" test depended arbitrarily on
LocalOrdinal=int and GlobalOrdinal=int.  This commit changes the test
to use the default LocalOrdinal and GlobalOrdinal types.  See trilinos#74.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 7, 2017
@trilinos/tpetra Make Tpetra's RowMatrixTransposer test not depend on
LocalOrdinal=int or GlobalOrdinal=int.  It now uses the default
LocalOrdinal and GlobalOrdinal types.  See trilinos#74.
lxmota pushed a commit that referenced this issue Aug 8, 2017
@trilinos/tpetra tpetra/core/test/BugTests/BugTests.cpp only built its
tests if GlobalOrdinal=int was enabled.  This was unnecessarily
restrictive, given that the bugs in question (Bugs 5129 and 5072 in
the Mozilla bug tracking system, before Trilinos switched to using
GitHub issue tracking) didn't appear to have anything to do with a
specific GlobalOrdinal type.  Furthermore, we want to have full
testing for the case where GlobalOrdinal=int is off.  See e.g.,

#74

This commit generalizes the tests to build and run for the default
LocalOrdinal and GlobalOrdinal types.
lxmota pushed a commit that referenced this issue Aug 8, 2017
@trilinos/tpetra Tpetra::Map's "ProblematicLookup" test depended
arbitrarily on LocalOrdinal=int and GlobalOrdinal=int.  This commit
changes the test to use the default LocalOrdinal and GlobalOrdinal
types.  See #74.
lxmota pushed a commit that referenced this issue Aug 8, 2017
@trilinos/tpetra Tpetra's "Block" test depended arbitrarily on
LocalOrdinal=int and GlobalOrdinal=int.  This commit changes the test
to use the default LocalOrdinal and GlobalOrdinal types.  See #74.
lxmota pushed a commit that referenced this issue Aug 8, 2017
@trilinos/tpetra Make Tpetra's RowMatrixTransposer test not depend on
LocalOrdinal=int or GlobalOrdinal=int.  It now uses the default
LocalOrdinal and GlobalOrdinal types.  See #74.
mhoemmen pushed a commit to mhoemmen/Trilinos that referenced this issue Aug 9, 2017
@trilinos/tpetra Make trilinos#114 and Bug 5882 ("Bug" refers to issues in the
nonpublic Mozilla tracking system) regression tests no longer require
GlobalOrdinal=int and LocalOrdinal=int to be enabled.  Now, the two
tests just use the default GlobalOrdinal and LocalOrdinal types.

See trilinos#74 for why this is a good idea.
crtrott pushed a commit to mndevec/Trilinos that referenced this issue Aug 17, 2017
@trilinos/tpetra tpetra/core/test/BugTests/BugTests.cpp only built its
tests if GlobalOrdinal=int was enabled.  This was unnecessarily
restrictive, given that the bugs in question (Bugs 5129 and 5072 in
the Mozilla bug tracking system, before Trilinos switched to using
GitHub issue tracking) didn't appear to have anything to do with a
specific GlobalOrdinal type.  Furthermore, we want to have full
testing for the case where GlobalOrdinal=int is off.  See e.g.,

trilinos#74

This commit generalizes the tests to build and run for the default
LocalOrdinal and GlobalOrdinal types.
crtrott pushed a commit to mndevec/Trilinos that referenced this issue Aug 17, 2017
@trilinos/tpetra Tpetra::Map's "ProblematicLookup" test depended
arbitrarily on LocalOrdinal=int and GlobalOrdinal=int.  This commit
changes the test to use the default LocalOrdinal and GlobalOrdinal
types.  See trilinos#74.
crtrott pushed a commit to mndevec/Trilinos that referenced this issue Aug 17, 2017
@trilinos/tpetra Tpetra's "Block" test depended arbitrarily on
LocalOrdinal=int and GlobalOrdinal=int.  This commit changes the test
to use the default LocalOrdinal and GlobalOrdinal types.  See trilinos#74.
crtrott pushed a commit to mndevec/Trilinos that referenced this issue Aug 17, 2017
@trilinos/tpetra Make Tpetra's RowMatrixTransposer test not depend on
LocalOrdinal=int or GlobalOrdinal=int.  It now uses the default
LocalOrdinal and GlobalOrdinal types.  See trilinos#74.
crtrott pushed a commit to mndevec/Trilinos that referenced this issue Aug 17, 2017
@trilinos/tpetra Make trilinos#114 and Bug 5882 ("Bug" refers to issues in the
nonpublic Mozilla tracking system) regression tests no longer require
GlobalOrdinal=int and LocalOrdinal=int to be enabled.  Now, the two
tests just use the default GlobalOrdinal and LocalOrdinal types.

See trilinos#74 for why this is a good idea.
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) pkg: Tpetra
Projects
None yet
Development

No branches or pull requests

8 participants