search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.21k stars 565 forks source link

Discuss and document workflow for directly snapshotting SEACAS into Trilinos to better serve ATDM #1751

Closed bartlettroscoe closed 3 years ago

bartlettroscoe commented 7 years ago

Next Action Status:

@gdsjaar to implement agreed to workflow on SEACAS github site then start doing new workflow ...

CC: @trilinos/framework, @gsjaardema

Description:

This story is to document a discussion and the final workflow for allowing @gsjaardema to directly SEACAS directly into Trilinos/packages/seacas/ in addition to shapshotting SEACAS into Sierra.base/seacas/ and then having that snapshotted from ther into Trilinos/packages/seacas/. The nature of the snapshotting process (i.e. never a merge) eliminates the potential for merge conflicts that might make the snapshotting from Sierra.base/seacas/ to Trilinos/packages/seacas/ more difficult. And this would allow @gsjaardema control to address issues for ATDM customers very quickly and simplify the version control and configuration of Trilinos for ATDM customers. For more background and discussion, see:

bartlettroscoe commented 7 years ago

So the basics of the workflow are pretty simple. When an ATDM or other customer urgently needs an update of SEACAS in Trilinos, they just ask @gsjaardema (or someone else) to snapshot SEACAS from seacas/packages/seacas/ directly into Trilinos/packages/seacas/ using the same snapshot script he uses currently to snapshot into Sierra.base/seacas/. @gsjaardema reports that he almost never needs to update anything in STK in order to upgrade SEACAS in Sierra so it is likely that most of these snapshots will not require any other changes in Trilinos. But if changes are needed, @gsjaardema (or someone else) can make changes in his local branch and then post a PR to this Trilinos repo and see if there are any concerns (but that might not be necessary in may cases if the changes are straightforward). Once ready to push, then one would just push to the main Trilinos 'develop' branch using ./checkin-test-sems.sh --do-all --push (rebasing is fine with a pure snapshot like this).

If changes to STK are needed, then they could be made in Trilinos/packages/stk/ and pushed to the Trilinos 'develop' branch. Then the commits could be pulled off and applied to Sierra.base/stk/ (I have a very robust procedure for doing this using temp branches and merge commits that I can document) and the snapshot of SEACAS can be updated in Sierra.base/seacas/ at the same time. @gsjaardema or anyone with push access to Sierra.base/ and some git know-how could do this.

Note that a great option for this workflow is that we could snapshot SEACAS directly into the ATDM branch of Trilinos and then that could be automatically synced back to the Trilinos 'develop' (with the same automated process syncing all changes to the ATDM copy of Trilinos back).

That is the basic outline for the updated process. I can write this process up in detail in a Google Doc and that way people can comment on it there as we refine it quickly to a form that we can publish (as a wiki page on this site) and then implement.

NOTE: In case there is concern that we are mentioning "Sierra.base", it is not exactly a secret that SEACAS and STK are being snapshotted from Sierra.base to Trilinos given Trilinos commits like:

commit e15b6358a6aa24d080ef9840816d1c3c47df5fd8
Author: Brent Perschbacher <bmpersc@sandia.gov>
Date:   Tue Aug 15 15:28:37 2017 -0600

    Snapshot of sierra.base.git from commit 888d2b5321002c5ca3e595ad8ccf14a9c9b4addc

    From repository at sierra-git.sandia.gov:/git/sierra.base.git

    At commit:
    commit 888d2b5321002c5ca3e595ad8ccf14a9c9b4addc
    Author: Greg Sjaardema <gdsjaar@sandia.gov>
    Date:   Mon Aug 14 10:35:14 2017 -0600

        APREPRO: Fix so will compile with intel-14

   6.2% packages/seacas/applications/blot/
  14.9% packages/seacas/applications/fastq/
   4.8% packages/seacas/applications/numbers/
  16.5% packages/seacas/applications/
   6.1% packages/seacas/libraries/chaco/
   4.6% packages/seacas/libraries/exodus/src/
   3.6% packages/seacas/libraries/ioss/src/main/test/
   3.5% packages/seacas/libraries/ioss/src/visualization/ParaViewCatalystIossAdapter/parser/jsoncpp-master/dist/json/
   8.1% packages/seacas/libraries/ioss/src/visualization/ParaViewCatalystIossAdapter/parser/jsoncpp-master/dist/
   5.0% packages/seacas/libraries/ioss/src/visualization/ParaViewCatalystIossAdapter/
   6.5% packages/seacas/libraries/ioss/src/
   3.7% packages/seacas/libraries/plt/
   3.0% packages/seacas/libraries/svdi/
  11.6% packages/seacas/libraries/

and 

commit 12804627c8587991061e8b81bdcc54c77b6c99c4
Author: Brent Perschbacher <bmpersc@sandia.gov>
Date:   Tue Aug 22 13:10:32 2017 -0600

    Snapshot of sierra.base.git from commit 0323b5dd555ef3d48801f4159f4a533cd6db88ad

    From repository at sierra-git.sandia.gov:/git/sierra.base.git

    At commit:
    commit 0323b5dd555ef3d48801f4159f4a533cd6db88ad
    Author: Kendall Hugh Pierson <khpiers@sandia.gov>
    Date:   Sat Aug 19 14:27:38 2017 -0600

        stk: Remove unused header file

  10.6% packages/stk/stk_balance/stk_balance/internal/
   4.7% packages/stk/stk_balance/
   4.7% packages/stk/stk_learning/ngp/
   5.3% packages/stk/stk_learning/
   9.9% packages/stk/stk_mesh/stk_mesh/base/
   8.8% packages/stk/stk_mesh/stk_mesh/baseImpl/elementGraph/
   3.5% packages/stk/stk_mesh/stk_mesh/baseImpl/
   8.8% packages/stk/stk_search/stk_search/
   3.5% packages/stk/stk_simd/stk_simd/avx/
   3.5% packages/stk/stk_simd/stk_simd/avx512/
   3.5% packages/stk/stk_simd/stk_simd/sse/
   3.5% packages/stk/stk_simd/stk_simd_view/
  10.6% packages/stk/stk_unit_test_utils/
   4.9% packages/stk/stk_util/stk_util/
  13.4% packages/stk/

so hopefully there should be no concern about mentioning "Sierra.base" in an open process. And as much info as is in these commits and what I have said above about "Sierra.base" is all that will be mentioned in this open documented process.

Any serious concerns about moving forward documenting the details in a Google Doc page?

jwillenbring commented 7 years ago

@bartlettroscoe One concern is that the revised SIERRA/Trilinos integration process worked out this FY (which includes snapshotting now and avoids merge conflicts) also looks to test NALU and Albany prior to updating SEACAS and STK in Trilinos. This is meant to reduce the issues that these applications were facing with updates.

bartlettroscoe commented 7 years ago

One concern is that the revised SIERRA/Trilinos integration process worked out this FY (which includes snapshotting now and avoids merge conflicts) also looks to test NALU and Albany prior to updating SEACAS and STK in Trilinos. This is meant to reduce the issues that these applications were facing with updates.

That is interesting. I guess the justification for extra testing that is that it is very hard to get things fixed if you expect them to be fixed in the Sierra.base/ repo and then snapshotted back to Trilinos. I think that might be more of an issue for STK than SEACAS. With SEACAS being directly snapshotted into Trilinos, then it could be fixed quickly or backed out (just a revert). Therefore, I don't think the we should have any higher requirements for snapshotting SEACAS directly into Trilinos than we would for pushing changes to say Ifpack2 or MueLu. If those developers don't have to test against NALU or Albany before they push to 'develop', then neither should snapshots of SEACAS.

So the assumption is that whoever snapshots SEACAS into Triilinos should have the same expectation to quickly fix problems for important internal customers as other Trilinos packages like Tpetra or MueLu (or back out the commit). If that is the case (and we can make sure that it is), then it should be okay to just run the checkin-test-sems.sh script for now, right? That seems about as fair and fair can be and it solves an important problem for ATDM (who is a very important internal Trilinos customer).

bartlettroscoe commented 7 years ago

A little more background on this (which is all given in SPAR-277) ...

The core of the problem is the indirect snapshotting of SEACAS into Sierra.base, then the snapshot of SEACAS AND STK to Trilinos and then the update of Trilinos to SPARC (and other ATDM APPs and developers). The problem here is always going to be breakages caused by STK (breaking the CMake build of STK) and breaking downstream Trilinos packages preventing the update of SEACAS in Trilinos from Sierra.base! Based on Greg’s feedback, upgrades of SEACAS almost never require changes in STK or downstream Trilinos packages. That fact is what makes a better workflow possible.

jwillenbring commented 7 years ago

@bartlettroscoe @gsjaardema My Albany/NaLu comment was incomplete and rushed. A primary concern from the Trilinos Framework perspective is that over the last year a snapshotting capability was developed and is in use now for SEACAS and STK. Greg could potentially just use that capability and @bmpersc would be happy to demonstrate it if he wants to evaluate that solution also.

If Greg chooses to use the above proposed solution I want to be up front that this isn't a Trilinos Framework supported solution. If there are issues with using it in the future and those issues are reported to the Trilinos Framework, we would more than likely again present the solution Brent could demonstrate if you are willing to evaluate it. I don't think there is necessarily an expectation of Trilinos Framework support for this, but I want to explicitly set expectations. For this kind of reason, I think it is important that the Trilinos Framework is involved in tool adoption discussions for Trilinos packages.

maherou commented 7 years ago

The recent reorg of Trilinos to a product focus (framework, data services, linear solvers, non-linear and analysis, and discretizations), and the related product leadership roles mean that this kind of change must go through and be approved by the Trilinos framework product team. It represents a fundamental change in how SEACAS is integrated into Trilinos and the framework team (in particular the framework product lead) has to approve of what is going on.

bartlettroscoe commented 6 years ago

Issue like #2039 demonstrate the motivation for allowing @gdsjaar to directly snapshot into Trilinos as well as into SIERRA, and do so in a way so that it does not interfere with the snapshotting of STK from SIERRA into Trilinos that @bmpersc does (requiring some communication and coordination between @gdsjaar and @bmpersc). I will try to set up a short meeting to get us all on the same page.

bartlettroscoe commented 6 years ago

We had a meeting today between myself, @jwillenbring, @bmpersc and @gdsjaar to discuss this topic. The workflow we agreed we would pursue is:

According to @gdsjaar, most of the changes to SEACAS don't require any changes to STK. That is what will allow the above simple process to work smoothly in most cases.

We also discussed what would happen if an update to SEACAS required changes to STK. One workflow we discussed is when very small and targeted changes to STK are needed. In this case, one option is to make those changes to STK in the Trilinos develop branch, then pull the STK commits off with git format-patch ... and then apply them back in the Sierra.base master branch using git am ... at the same time that the same version of SEACAS was snapshotted there as well. The exact workflow steps and commands for doing that take a little explaining but are not too bad.

The other use case is where non-trivial changes to STK are made when SEACAS is updated. @gdsjaar said that this primarily happens when STK needs new features from SEACAS and therefore new STK code is written to use these features. But he said that even in that case, the updated version of SEACAS will usually work with the older version of STK. In cases like that, SEACAS can be snaphotted into both Sierra.base and Trilinos as per the above typical simple process. But in cases where non-trivial changes to older versions of STK are required to work with the updated SEACAS, the best workflow would likely be for @gdsjaar to snapshot SEACAS into just Sierra.base master, have STK updated there, and then ask @bmpersc to snapshot both STK AND SEACAS from Sierra.base master to Trilinos develop. But again, this should be some what rare according to @gdsjaar.

We discussed the following plan for moving forward with this:

  1. Greg will document this SEACAS integration workflow for Trilinos and Sierra.base and the relationship to STK. (This would likely be done in the main SEACAS repo wiki https://github.com/gsjaardema/seacas/wiki but any place would be fine.) (An example of a more complex detailed snapshotting process for TriBITS is given at http://trac.trilinos.org/wiki/TriBITSTrilinosDev.)

  2. A wiki page for package integrations will be created in this Trilinos GitHub wiki that will point to the integration page for SEACAS that @gdsjaar will write.

  3. The process will start when @gdsjaar first communicates with @bmpersc and then does the first snapshot of SEACAS into both Sierra.base and Trilinos. From that point on, @bmpersc will only snapshot STK from Sierra.base to Trilinos (unless @gdsjaar and @bmpersc decide otherwise in some rare cases as described above).

But until @gdsjaar is ready to start up this process, @bmpersc will continue with the current snapshotting process he is currently doing. @gdsjaar said this would not likely happen until Jan. 2018.

bartlettroscoe commented 6 years ago

FYI: Kokkos keeps their instructions in a version-controlled file in their repo:

Since general users would not access this info frequently, that seems like a place where it might go. But that is up to @gdsjaar.

ndellingwood commented 6 years ago

A more detailed and updated set of instructions is also available in this file: https://github.com/kokkos/kokkos/blob/develop/config/kokkos-promotion.txt

gsjaardema commented 6 years ago

https://github.com/kokkos/kokkos/blob/develop/doc/kokkos-promotion.txt The instructions referenced above have been moved. Updating location.

github-actions[bot] commented 3 years ago

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.

github-actions[bot] commented 3 years ago

This issue was closed due to inactivity for 395 days.