Deal with depracation of DynamicProfile option for Tpetra::CrsGraph constructor

[ikalash]

I received the following email from @kddevin about an upcoming change to Tpetra::CrsGraph, which will deprecate the DynamicProfile option. I am posting it here to allow all Albany users to chime in, in particular, this change may be of interest to @bartgol and @ibaned .

Roger, Irina and Mauro:

As you know, the Tpetra team is deprecating the DynamicProfile option for constructing CrsGraph and CrsMatrix. We hope to have deprecation notices in Tpetra within the next few weeks. We'd like your opinion on the best approach (from an application perspective) on carrying out the deprecation.

The challenge arises because we are both changing the default behavior of some constructors and removing constructors specifying dynamic-vs-static profile.

Currently, there exist constructors with the default behavior specified to be DynamicProfile; e.g.,

    CrsGraph(blah, blah, profileType = DynamicProfile)

Users calling

    CrsGraph(blah, blah)

get DynamicProfile currently.

After DynamicProfile is removed, the constructor will simply be

    CrsGraph(blah, blah);

but the behavior will be StaticProfile. It isn't clear how to tell users of the current constructor with the default value that their behavior is changing.

The Tpetra team thought of a couple of approaches for the deprecation: a two-phase approach and a one-step "rip off the bandage" approach. They are detailed below. For both, there will be a CMake option that allows users to enable/disable deprecated code during the transition period.

Please share your thoughts on these two approaches. Do you have a preferred approach? Do you see any pitfalls or advantages?

Thanks for your help.

Karen (for the Tpetra team)

Two-phase deprecation: first default type (which warns that behavior is
changing) and then the interface with profileType argument

Phase one: Behavior deprecation

  #ifdef ENABLE_DEPRECATED
    const profileType defaultProfileType = DynamicProfile;
    deprecated DynamicProfile enum value
  #else
    const profileType defaultProfileType = StaticProfile;
    Remove DynamicProfile enum value
  #endif
  Constructor CrsGraph(blah blah, profileType = defaultProfileType);

At next release point:

remove all the DynamicProfile code, including DynamicProfile enum value

Phase two: Interface deprecation

  #ifdef ENABLE_DEPRECATED
    deprecated Constructor CrsGraph(blah blah, profileType)
       (where this constructor ignores profileType because only StaticProfile remains)
    deprecated ProfileType enum
  #else
    Constructor CrsGraph(blah blah)
    remove ProfileType enum
  #endif

At next release point:

Remove the enum and the constructor with profileType argument

Rip-off-the-bandage deprecation: take away the interface and behavior

  #ifdef ENABLE_DEPRECATED
      Constructor CrsGraph (blah blah, profileType = DynamicProfile)
      deprecated Constructor CrsGraph(blah blah, profileType)
  #else
      Constructor CrsGraph(blah blah) with StaticProfile behavior
  #endif

[ikalash]

Grepping through Albany, here are the instances of the Tpetra::CrsGraph constructor:

grep "new Tpetra_CrsGraph" -r
Petra_Converters_64.cpp:  Teuchos::RCP<Tpetra_CrsGraph> tpetraCrsGraph_ = Teuchos::rcp(new Tpetra_CrsGraph(tpetraRowMap_, tpetraColMap_, maxEntries));
responses/Albany_SolutionResponseFunction.cpp:    Teuchos::rcp(new Tpetra_CrsGraph(culled_mapT, 1));
disc/pumi/Albany_APFDiscretization.cpp:  overlap_graphT = Teuchos::rcp(new Tpetra_CrsGraph(
disc/pumi/Albany_APFDiscretization.cpp:  graphT = Teuchos::rcp(new Tpetra_CrsGraph(mapT, nonzeroesPerRow(neq)));
disc/stk/Aeras_SpectralDiscretization.cpp:  Teuchos::RCP<Tpetra_CrsGraph> Graph = Teuchos::rcp(new Tpetra_CrsGraph(overlap_mapT,
disc/stk/Aeras_SpectralDiscretization.cpp:  Teuchos::RCP<Tpetra_CrsGraph> OwnedGraph = Teuchos::rcp(new Tpetra_CrsGraph(mapT, nonzeroesPerRow(neq)));
disc/stk/Aeras_SpectralDiscretization.cpp:  overlap_graphT = Teuchos::rcp(new Tpetra_CrsGraph(overlap_mapT, 1));
disc/stk/Aeras_SpectralDiscretization.cpp:  graphT = Teuchos::rcp(new Tpetra_CrsGraph(mapT, 1));
disc/stk/Albany_STKDiscretization.cpp:      Teuchos::rcp(new Tpetra_CrsGraph(overlap_mapT, neq * nodes_per_element));
disc/stk/Albany_STKDiscretization.cpp:  graphT = Teuchos::rcp(new Tpetra_CrsGraph(mapT, nonzeroesPerRow(neq)));
disc/stk/Albany_STKDiscretization.cpp:  nodalGraph = Teuchos::rcp(new Tpetra_CrsGraph(overlap_node_mapT, 27));
disc/stk/Albany_STKDiscretization.cpp:        Teuchos::rcp(new Tpetra_CrsGraph(ss_ov_mapT, 1, Tpetra::StaticProfile));
disc/stk/Albany_STKDiscretization.cpp:        Teuchos::rcp(new Tpetra_CrsGraph(ss_mapT, 1, Tpetra::StaticProfile));
disc/stk/Albany_STKDiscretizationStokesH.cpp:  overlap_graphT = Teuchos::rcp(new Tpetra_CrsGraph(overlap_mapT, neq*nodes_per_element));
disc/stk/Albany_STKDiscretizationStokesH.cpp:  graphT = Teuchos::rcp(new Tpetra_CrsGraph(mapT, nonzeroesPerRow(neq)));

Only a couple of these explicitly specify the StaticProfile, meaning we're using the default DynamicProfile most of the time. I doubt that we really need the DynamicProfile, but perhaps somehow has an instance of a case where we do.

[kddevin]

Note: these changes will affect the CrsMatrix constructor as well.

[bartgol]

So without the dynamic profile, the user is responsible to provide a valid upper bound (or exact count) of the nnz in each row, right? I don't think this will be problematic for Albany. We will have to change a bit our filling of the graph (put all indices in temp arrays when looping on the mesh, count, create the graph with exact count, then insert in the graph).

We never build matrices from scratch, always from a pre-filled graph, so no issue there.

[kddevin]

That's right; with StaticProfile, you need to provide an upper bound or exact count. Let us know if you run into any difficulties; we'll be happy to address them right away.

[mperego]

@kddevin To answer directly your question, the two-step deprecation strategy you propose makes sense to me, because you warn the users calling CrsGraph(blah blah) that the default behavior is changing.
As for Albany, I would think we are fine with either deprecation approach. As @bartgol pointed out it should not be too hard for us to comply with the new implementation.

[ikalash]

Regarding @kddevin 's comment that CrsMatrix is affected to - yes, I see. About 1/2 of our CrsMatrix constructors take in the graph, so there I imagine whatever the graph profile is will be the matrix one. It seems there are only a handful of other CrsMatrix constructors.

If we wanted to test things in advance, we could probably switch all the instances of CrsMatrix and CrsGraph to the static profile and see what happens. The change to the filling will take some work as Luca points out above.

I guess I would vote for the more graded/conservative approach rather than the bandaid ripping one, if there is an option.

[ikalash]

Since this will need to be addressed very soon and no one has volunteered, I'll take a look at this, once I'm done getting all the Tpetra out of LCM in the unpetrify_branch of Albany. It turns out the usage of static vs. dynamic graph is relevant to one of the responses I've been working on converting to Thyra, namely ProjectIPtoNodalField. We should also do the switch from dynamic to static profile with an eye towards how it will fix into the unpetrify_branch of Albany. It should be easier there since the allocation of Tpetra matrices/graphs is contained to a few utilities functions, rather than pervasive throughout the code.

[ikalash]

Forgot to mention, help is welcome, if anyone is interested/available.

[ikalash]

Update from today's SART meeting on this:

• The dynamic profile in Tpetra will be deprecated as part of the Trilinos 12.16 release, currently scheduled for April 30.
• One will still be able to use the deprecated code for ~1 month. The deprecated code will be on by default, and a warning will be printed. After ~1 month, Tpetra team members will begin removing the deprecated code from the develop branch.
• The approach selected by the Tpetra team on how to go about the deprecation was the "rip off the bandaid" approach (see above discussion of this in the issue).

[ikalash]

Update from today's SART meeting: the removal of the dynamic profile from Tpetra is being deferred to end of June. So we have more time.

[ikalash]

@kddevin : is the timeframe for removing the dynamic profile still June 30?

[kddevin]

@ikalash probably not; we do not yet have the complete path for running without dynamic profile. Mid-July is more likely.

[bartgol]

Is it just a matter of time or is there the possibility that dynamic profile will be "un-deprecated"?

[ikalash]

@bartgol : I think it's a matter of time - see the comment by @kddevin . I think we should prioritize changing the code to use the static profile, otherwise it'll get broken at some point and we will be scrambling to do the conversion to keep it compiling / running. Maybe we should meet next week while I'm in town to talk about how to do it / start working on it?

[bartgol]

I agree. I was just making sure that when Karen said "we do not yet have the complete path for running without dynamic profile" she meant that they know how to do it, they just haven't finished it.

You can go ahead and start it if you want. I don't have much time to work/help on it for the next 3-4 weeks... But I got plenty of time to talk, if you need me.

[ikalash]

@kddevin : I just tried to build Trilinos with -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF towards switching Albany to the static graph. It seems there is an issue if you build with Xpetra ON (through MueLu for us) and the flag -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON . I get the following compilation error:

Processing enabled package: Xpetra (Libs)
-- Xpetra: Enabling deprecated code
-- Xpetra support for 32 bit Epetra enabled.
--    Xpetra_Epetra_NO_32BIT_GLOBAL_INDICES=OFF
CMake Error at packages/xpetra/CMakeLists.txt:146 (MESSAGE):
  If Xpetra Epetra support is enabled and Epetra 32-bit global indices are
  enabled, Xpetra requires that you enable Tpetra_INST_INT_INT


-- Configuring incomplete, errors occurred!

Is this expected? I actually can't twitch to Tpetra_INST_INT_INT b/c Albany requires the Tpetra_INST_INT_UNSIGNED_LONG to be on. I also wanted to keep MueLu on, which requires Xpetra. Perhaps there are some workarounds?

[tjfulle]

@ikalash, yep that is expected right now. Only today were we able to build Trilinos with Tpetra's downstream packages enabled with -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF , but we have to disable Epetra.

We are currently working with Xpetra, Panzer, and MueLu to be able to build/run tests with deprecated code disabled with any supported GO type. We're getting close, you can follow at trilinos/Trilinos#5602

[tjfulle]

A current workaround is to disable Epetra. Long term, packages need to be careful to disassociate the Epetra GO from the Tpetra GO since Tpetra will only be able to instantiate one GO type per build.

[mperego]

@ikalash you can try this:
-D Xpetra_ENABLE_Epetra:BOOL=OFF
-D MueLu_ENABLE_Epetra:BOOL=OFF
Not sure it works..

[tjfulle]

There is a configuration posted to trilinos/Trilinos#5602 that works.

[ikalash]

Thanks @tjfulle . I was able to create a no-epetra and a no-muelu build of Trilinos that we can use to build Albany as we switch over to the static graph. The configure scripts for Trilinos are attached here, for future reference. It seems we need to turn off Rythmos in both builds for things to compile. There are still some tests in Albany that use Rythmos, although it's not critical anymore.
do-cmake-trilinos-mpi-camobap-no-epetra.txt
do-cmake-trilinos-mpi-camobap-no-muelu.txt

[tjfulle]

@ikalash I’m actively working on this now. Trilinos will soon build with Tpetra’s deprecated code disabled, without having to disable other packages and/or know implementation details of downstream (from Tpetra) packages. Rythmos is the next in my sights. The MueLu developers are actively working on this too.

[ikalash]

@tjfulle : ah ok, great. We are way behind in switching Albany to the static graph profile (I'm just starting to look at it) so you're certainly not holding us up... the more time we have to deal with the deprecation, the better!

[tjfulle]

Also, MueLu will build with Tpetra’s deprecated code turned off, but only without Epetra support. Unfortunately, there are still quite a few MueLu test failures with this configuration, so it’s not quite done, but close.

[bartgol]

Piggy-backing on this issue, we should take care of other deprecated stuff in Trilinos. For instance, I'm getting a deprecation warning from stk_mesh and one from Thyra.

[ikalash]

I think we can consider this done. Thank you in particular to @mperego for doing the bulk of the work on the Albany side! The algol/proxima nightlies need to be updated to have the right Trilinos configuration options, and also to point to the right version of DTK, but this is mentioned in a different issue (#544).