-
Notifications
You must be signed in to change notification settings - Fork 45
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
NextGen Analytics Ramping up on TriBITS Refactor work #434
Comments
Hello @mperrinel and @marcinwrobel1986, Here is the Issue with a set of tasks to start ramping up and provide some initial feedback. Please let me know if you have any questions about any of the tasks listed above. For the review of specific files listed above, I think you can review the files by adding references like this: TriBITS/tribits/core/package_arch/TribitsExternalPackageWriteConfigFile.cmake Lines 66 to 69 in fd77678
in a comment in this issue (or some other issue just for that set of files) and then saying something about it that comment. Another option to do a review, which is more work to set up, would be to create a dummy branch for an older version of 'master', then copy files to be reviewed into another dummy branch taken off of that older version of the code, then set up a PR with the updated dummy branch against the older reference dummy branch. That way, that PR would should show just the files to be reviewed in their current state and would allow doing a review just like any other PR. Only you would not bother merging the PR at the end of the review. If you are interested in that approach, I can set up a demo and we can try it out. Just let me know. |
Given the creation of #437, I can assume this has started? |
As for Spack builds of Trilinos, please:
|
Review of https://tribits.org/doc/TribitsUsersGuide.html#how-to-use-find-package-for-a-tribits-tpl (How to use Does that make sense ? Why can't we simply integrate the Config generated code directly inside the FindTPL Module file ? |
The glue between existing The basic requirements are:
That is about it.
The idea is that the
Note that a Again, how do this glue for external packages is up in the air right now and I don't see any really great solutions. We just need a well documented and solid approach that we know will work. That will evolve as we complete #299 and #63 and that is okay. One thing that would make it easier and more robust to implement these specialized
Basically, the same code that generates the IMPORTED targets for external clients gets used to create these targets for the internal CMake targets. That is idea. Again, all of this is a work in progress that will evolve and we complete #299 and #63. |
@mperrinel, thanks for your comments in Plan to merge TriBITS concepts and implementation of Packages and TPLs. I think I have responded to all of them and make a change based on one of them. The most critical use case driving Epic #367 is "Use Case 3: Configure/build pointing to a subset of already installed TriBITS packages in same repo". But given we can implement that, the other use cases will be easier to implement when needed. |
@mperrinel and @marcinwrobel1986, as discussed at the meeting today, please comment in this issue or in the associated PR on any reviews you have done and what your impressions are. |
@bartlettroscoe, concerning the issue (Checkbox 2: Clone Trilinos repo...). Spack has been used to get Trilinos and its dependencies quickly and with most of this TPLs. Trilinos has been installed using this Spack install command :
By default, the stage is kept on a temporary directory, so even if you keep the build, it is automatically removed. To fix this problem, I edited a line on the spack/etc/spack/defaults/config.yaml :
You can change the default <$where_you_want_to_keep_your_build> by a directory which is not temporary. The default Trilinos version is 13.0.1 so it is necessary to add @develop to specify the develop version. The different variants activated with the command permit to obtain these TPLs: We can compare with the requested TPLs: And we can see, Pthread, Scotch, SuperLU and BoostLib are missing when, METIS, CGNS, Pnetcdf and Matio are extra TPls.
This first error has been fixed by installing csh command with apt.
I was not able to fix this error so I decided to install SuperLU my self using the source used by Spack : Once the compilation is done, SuperLU installation can be used by Trilinos as a TPL with the correct options : TPL_ENABLED_SuperLU=ON, TPL_SuperLU_INCLUDE_DIRS and TPL_SuperLU_LIBRARIES. Only the libsuperlu_4.3.a has to be set to the TPL_SuperLU_LIBRARIES variable Finally with few manual installations, all the requested TPLs are enabled with Trilinos. |
@bartlettroscoe. Review of the checkbox 5.3 ( Using packages from the build tree...): |
@mperrinel, can you put in a PR with your suggested edits? |
@mperrinel, can you please push a branch to your fork of spack with this and any other changes to spack? This violates the Open-Closed Principle (OCP). |
@mperrinel and @keitat, Question, what do we about the build and test failures for the Spack Trilinos configuration shown here? We see a build error in It is important we can automate the building and testing of this configuration so we can use to test updates of TriBITS. And we need a 100% passing reference build of Trilinos for that purpose. |
@bartlettroscoe @keitat
|
@bartlettroscoe |
@bartlettroscoe Review of the checkbox 5.4 and 5.5
|
@mperrinel, as long as you can document it and it is repeatable, that seems like an acceptable solution. But it seems that you would need to reset |
@mperrinel, the instructions in TribitsExampleApp/README.md assume that you have installed
Would it help if this was changed to:
? |
@mperrinel, correct. To make this simpler, we should perhaps just set |
@bartlettroscoe Review of the checbox 5.6 |
@mperrinel, okay. Then let's update TribitsExampleProject/README.md to recommend explicitly setting:
and mentioning that this will require Fortran. We will also need to update the instructions for |
@bartlettroscoe
is correct. In addition, we should mention that the subpackage C of TribitsExampleProject require Fortran here. |
@mperrinel, If no one is keeping testing Trilinos with recent versions of SuperLU and if SuperLU is not maintaining backward compatibility, then we should not be surprised that updated versions of Trilinos and SuperLU don't work together. @keitat, what is the status of ECP-xSDK testing of Trilinos and SuperLU? |
Hello @bartlettroscoe @mperrinel , |
I confirmed the error with Amesos. Can you change back to [email protected]? I will report it to Amesos team.
|
Yes, please create a PR with suggested fixes
It is harmless to define dependencies if there really is none. To be safe, we just need to maintain the ordering of the libs provided on the link line in case that is significant. That is really all the code is doing since we can't actually link between libraries that are already created.
That is not true. You have to maintain the order provided and CMake will maintain the order of the include dirs that you give it with this list.
I don't think that is how it works. The include dirs set on the INTERFACE |
@bartlettroscoe
So I don't really understand why is it nos always visible on the tests. What is the condition to write the all_libs target ? Why is not visible in this test : function(unittest_tribits_external_package_process_libraries_list_incl_dirs_0_lib_files_3) |
@mperrinel, because that unit test just calls the function |
@bartlettroscoe ok, thanks, all make sense now. |
@bartlettroscoe I opened the PR for 6.1 spelling mistakes |
@bartlettroscoe @keitat Progress on 5.2: This is my final list of Enabled TPLs : The compilation ended with success, but not the ctest done by make dashboard : |
…eConfigFile (#434) Fixed spelling mistakes
@mperrinel, can you dig into these errors some and see if you can post Trilinos GitHub issues for that build on CDash? The reproducibility instructions should involve the branch of spack that you are using to generate these results. (That is the whole point of this exercise, to allow consistent reproductions of builds of Trilinos.) Do you know how to use the the "Test Output" filter with the CDash Initial analysis of Zoltan test failures (click to expand). First, let's start with all of the failing tests for this build: showing: With a little poking around and using the filter ["Test Output", "include", "Diff failed"] with the query: showing: you can see that 8 of the 9 failing Zoltan tests are diffs. If you click "Show Matching Output" on the upper right, you can see the details (or at least the context) of the diffs a little. And the query: showing: shows that that one Zoltan test The query: showing: shows that one test is failing due to "Missing output files". The problem with that latter test
It looks like that error is coming from OpenMPI I think you need the argument This is getting detailed enough that we should likely create a Trilinos GitHub issue for these failures and move the discussion there. |
package.py for Trilinos does not provide the equivalent option. Quick workaround is adding a few python statements for this option. Need a process to automate package.py generation. |
For now, you can just create a branch in the spack repo and make the changes right there. |
I will just add a few statements and do PR to stack team (or use my own Spack fork). @mperrinel I will let you know as soon as it is ready. |
Thanks @keitat |
With Spack we can install Trilinos without Amesos. However, I recommend building Trilinos without Spack. @bartlettroscoe , what Trilinos packages do you activate? |
Just about all of them (or at least all of them that are enabled in Trilinos PR testing). |
At this point I think we should consider abandoning the Spack build of TPLs for Trilinos to allow NGA to reproduce Trilinos builds and tests with updated TriBITS versions. Instead, we should descope and just have NGA run TriBITS test suite and review TriBITS PRs and Issues (and provide comments for related Trilinos PRs). Let's discuss at the meeting today. |
NOTE: Trilinos PR testing is using SuperLU 4.3. For example, see here showing:
But we should abandon this effort and just skip NGA testing with Trilinos.* |
@MikolajZuzek, please look at documentation and files listed above. |
@bartlettroscoe @keitat. Please find attached the documentation requested |
@mperrinel, there are some formatting problems with the rendered Markdown (as per the Markdown Viewer Chrome Extension). It shows: Can you fix the formatting? |
Ok @bartlettroscoe, it is updated |
Let's call this done |
Parent Issue:
Description
This issue is an outline for ramping up to help out with the TriBITS Refactor work being tracked in the TriBITS Refactor board and the EPIC #367.
Tasks
<tplName>Config.cmake
files:The text was updated successfully, but these errors were encountered: