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

Compadre: Add New Package to Trilinos #6934

Closed
jmgate opened this issue Mar 2, 2020 · 15 comments
Closed

Compadre: Add New Package to Trilinos #6934

jmgate opened this issue Mar 2, 2020 · 15 comments
Assignees

Comments

@jmgate
Copy link
Contributor

jmgate commented Mar 2, 2020

This issue is to track the insertion of Compadre into Trilinos, and is a part of the overall issue Compadre#164.

CC: @mperego, @kuberry

@jmgate jmgate self-assigned this Mar 2, 2020
@jmgate
Copy link
Contributor Author

jmgate commented Mar 2, 2020

@bartlettroscoe, I'm trying to figure out what all is needed if I have a package that currently builds with CMake that we want to snapshot into Trilinos. Compadre will continue to be developed in its own repository. When we snapshot it into Trilinos, we'll need to omit two directories from its source (Kokkos, plus another one). It should build via its own CMake build system when built on its own, but via TriBITS when built as part of Trilinos. It currently builds Kokkos for you before building itself against it.

I'm thinking we need something along the lines of

for modifications to the build system, plus

to snapshot it into Trilinos. Am I missing anything here? Do you have a guess as to how long it would take to integrate a package like this into Trilinos?

@jmgate jmgate changed the title Compadre: Add new package to Trilinos Compadre: Add New Package to Trilinos Mar 2, 2020
@bartlettroscoe
Copy link
Member

Do you have a guess as to how long it would take to integrate a package like this into Trilinos?

@jmgate, how long integrating in an existing package into Trilinos depends on several factors.

Note that you would not both snapshot the package source into the Trilinos git repo and also deal with it as an inserted package. You would do either one or the other. The choice depends on the controlling use cases. But it sounds like your use case is very similar to SEACAS and therefore if you set up Compadre like SEACAS, the version control parts will be easy.

We should likely discuss this briefly offline over Skype.

@jmgate
Copy link
Contributor Author

jmgate commented Mar 2, 2020

It sounds like "inserted package" probably means something in TriBITS I didn't realize. Yes, I think we're talking about snapshotting it in similar to SEACAS, KOKKOS, etc. I'll see if I can find a time for a quick Skype call.

@jmgate
Copy link
Contributor Author

jmgate commented Mar 3, 2020

@rmmilewi, forgot to tag you on this earlier. Sorry about that.

@jmgate
Copy link
Contributor Author

jmgate commented Mar 5, 2020

@trilinos/framework, I'm not sure what the current policies and procedures are for adding a new package to Trilinos. Can you please review this new package checklist and let me know if we can proceed?

@jmgate
Copy link
Contributor Author

jmgate commented Mar 11, 2020

@trilinos/framework, if you're not the right folks to evaluate this, please let me know.

@mperego
Copy link
Contributor

mperego commented Mar 11, 2020

@jmgate I had already asked for approval for this package, both to product leads and Trilinos framework, and I got their approval

@mperego
Copy link
Contributor

mperego commented Mar 11, 2020

@jmgate I had already asked for approval for this package, both to product leads and Trilinos framework, and I got their approval

I also sent an email to Trlinos developers. So, please proceed.

@jwillenbring
Copy link
Member

@bartlettroscoe Would this be a good first use case for the refactor that would make TPLs indistinguishable from packages? Then Compadre would not have to implement a second build system and it might give the effort a shot in the arm?

@bartlettroscoe
Copy link
Member

Would this be a good first use case for the refactor that would make TPLs indistinguishable from packages? Then Compadre would not have to implement a second build system and it might give the effort a shot in the arm?

@jwillenbring, how would the refactorings/extensions in TriBITSPub/TriBITS#63 and TriBITSPub/TriBITS#299 help here? If you treat Compadre as a TPL w.r.t. Trilinos, someone else still has to build and install it. I think the goal here is to have Compadre built as part of the Trilinos CMake project and installed along with all of the other Trilinos packages.

bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Apr 27, 2020
…s#6934)

The ATDM APPs are not using this package so no reason to test this as part of
ATDM Trilinos testing.
@bartlettroscoe
Copy link
Member

Just an FYI, but note that when #7241 got merged, it had the side-effect of enabling Compadre in all of the ATDM Trilinos builds. Therefore, those ran today and you can see how Compadre is doing on a number of different platforms in this query:

PR #7268 will disable Compadre in ATDM Trilinos testing going forward but I thought you might want to see what its portability looks like. (It looks to be failing to build in all of the CUDA builds as shown here).

trilinos-autotester added a commit that referenced this issue Apr 27, 2020
…le-compadre

Automatically Merged using Trilinos Pull Request AutoTester
PR Title: ATDM: Remove new package Compadre from ATDM Trilinos testing (#6934)
PR Author: bartlettroscoe
@kuberry
Copy link
Contributor

kuberry commented Apr 27, 2020

Just an FYI, but note that when #7241 got merged, it had the side-effect of enabling Compadre in all of the ATDM Trilinos builds. Therefore, those ran today and you can see how Compadre is doing on a number of different platforms in this query:

PR #7268 will disable Compadre in ATDM Trilinos testing going forward but I thought you might want to see what its portability looks like. (It looks to be failing to build in all of the CUDA builds as shown here).

Thanks for letting me know Ross. This is a result of those tests being run with TPL_ENABLE_CUDA set to ON, but with Compadre_USE_CUDA by default being set to OFF. I switched some CMake logic to resemble the suggestion you made in the original Compadre snapshot, and now these tests should pass if they were run again (no longer needing to set Compadre_USE_CUDA explicitly).

The fix is in PR #7271

jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue Apr 28, 2020
…s:develop' (2f9297e).

* trilinos-develop: (32 commits)
  Add alternative spelling of option
  Changed CMake logic so that if TPL_ENABLE_CUDA is set to "ON", then CUBLAS and CUSOLVER will become required TPLs, otherwise LAPACK and BLAS will become required TPLs.
  Condition is checked at level above, not needed
  Remove accidentally committed change
  SEACAS: More parmetis fixes * support 32-bit real parmetis / metis libraries * show parmetis version and int/real size in config output * show supported decomp options in config output
  Fixes for the PR
  Fix as recommended in review.
  ATDM: Remove new package Compadre from ATDM Trilinos testing (trilinos#6934)
  CONFIG: Update NetCDF and HDF5 TPL version (trilinos#7197)
  Panzer: partial rollback/amendment of 7225 due to EMPIRE reliance on previous behavior.
  SEACAS: Fix enabling of ParMETIS; clean up
  Tpetra: disable MPI based DistObject BlockedView test if no MPI enabled
  Tpetra: Adding Teuchos_Comm.hpp include to BugTests.cpp
  Kokkos: Fix span calculation of View for corner case
  Added unit tests for appAction related classes to Subcycling
  Fixing accidental cmake change
  Commiting OperatorSplit passes tests
  Tpetra Distributor: Add optional argument to getReverse
  tpetra:  removed unused code and comments that no longer are relevant The unused code sometimes caused compilation problems (that's why I noticed it)
  Revert "Tpetra Distributor: add method to get reverse distributor"
  ...
jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue Apr 28, 2020
…s:develop' (2f9297e).

* trilinos-develop: (32 commits)
  Add alternative spelling of option
  Changed CMake logic so that if TPL_ENABLE_CUDA is set to "ON", then CUBLAS and CUSOLVER will become required TPLs, otherwise LAPACK and BLAS will become required TPLs.
  Condition is checked at level above, not needed
  Remove accidentally committed change
  SEACAS: More parmetis fixes * support 32-bit real parmetis / metis libraries * show parmetis version and int/real size in config output * show supported decomp options in config output
  Fixes for the PR
  Fix as recommended in review.
  ATDM: Remove new package Compadre from ATDM Trilinos testing (trilinos#6934)
  CONFIG: Update NetCDF and HDF5 TPL version (trilinos#7197)
  Panzer: partial rollback/amendment of 7225 due to EMPIRE reliance on previous behavior.
  SEACAS: Fix enabling of ParMETIS; clean up
  Tpetra: disable MPI based DistObject BlockedView test if no MPI enabled
  Tpetra: Adding Teuchos_Comm.hpp include to BugTests.cpp
  Kokkos: Fix span calculation of View for corner case
  Added unit tests for appAction related classes to Subcycling
  Fixing accidental cmake change
  Commiting OperatorSplit passes tests
  Tpetra Distributor: Add optional argument to getReverse
  tpetra:  removed unused code and comments that no longer are relevant The unused code sometimes caused compilation problems (that's why I noticed it)
  Revert "Tpetra Distributor: add method to get reverse distributor"
  ...
@jmgate jmgate closed this as completed Apr 28, 2020
@mperego
Copy link
Contributor

mperego commented May 28, 2020

@jmgate Have you guys checked if the Doxygen documentation builds correctly? I just tried to generate it but it looks empty.

@jmgate
Copy link
Contributor Author

jmgate commented May 28, 2020

You know, I don't think we did anything about the Doxygen when we added Compadre to Trilinos. I can look into it, but it likely won't be till Monday.

@mperego
Copy link
Contributor

mperego commented May 28, 2020

Thanks. No rush. Paul is looking into it, so he will probably fix it.

kliegeois pushed a commit to kliegeois/Trilinos that referenced this issue May 29, 2020
…s#6934)

The ATDM APPs are not using this package so no reason to test this as part of
ATDM Trilinos testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants