-
Notifications
You must be signed in to change notification settings - Fork 573
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
Comments
@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, 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. |
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. |
@rmmilewi, forgot to tag you on this earlier. Sorry about that. |
@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? |
@trilinos/framework, if you're not the right folks to evaluate this, please let me know. |
@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. |
@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? |
@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. |
…s#6934) The ATDM APPs are not using this package so no reason to test this as part of ATDM Trilinos testing.
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). |
…le-compadre Automatically Merged using Trilinos Pull Request AutoTester PR Title: ATDM: Remove new package Compadre from ATDM Trilinos testing (#6934) PR Author: bartlettroscoe
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 |
…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" ...
…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 Have you guys checked if the Doxygen documentation builds correctly? I just tried to generate it but it looks empty. |
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. |
Thanks. No rush. Paul is looking into it, so he will probably fix it. |
…s#6934) The ATDM APPs are not using this package so no reason to test this as part of ATDM Trilinos testing.
This issue is to track the insertion of Compadre into Trilinos, and is a part of the overall issue Compadre#164.
CC: @mperego, @kuberry
The text was updated successfully, but these errors were encountered: